CHECK that ui::Windows being destroyed have been hidden if necessary

There have been a bunch of crashes due to the GestureRecognizer targeting
touches to windows that have been destroyed. When a window is hidden 
(in WindowEventDispatcher::OnWindowHidden), all references to that window 
are removed from the GestureRecognizer.

It looks like some windows with touches targeted to them are being 
destroyed without being hidden first. This change CHECKs that this
never occurs.

BUG=342040

Review URL: https://codereview.chromium.org/224063002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@261531 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 53 insertions, 30 deletions

diff --git a/ui/aura/window.cc b/ui/aura/window.cc
index 86c9046..2c9aab8 100644
--- a/ui/aura/window.cc
+++ b/ui/aura/window.cc
@@ -235,6 +235,15 @@ Window::~Window() {
   if (host)
     host->dispatcher()->OnPostNotifiedWindowDestroying(this);
 
+  // The window should have already had its state cleaned up in
+  // WindowEventDispatcher::OnWindowHidden(), but there have been some crashes
+  // involving windows being destroyed without being hidden first. See
+  // crbug.com/342040. This should help us debug the issue. TODO(tdresser):
+  // remove this once we determine why we have windows that are destroyed
+  // without being hidden.
+  bool window_incorrectly_cleaned_up = CleanupGestureState();
+  CHECK(!window_incorrectly_cleaned_up);
+
   // Then destroy the children.
   RemoveOrDestroyChildren();
 
@@ -1324,6 +1333,19 @@ void Window::OnWindowBoundsChanged(const gfx::Rect& old_bounds) {
                     OnWindowBoundsChanged(this, old_bounds, bounds()));
 }
 
+bool Window::CleanupGestureState() {
+  bool state_modified = false;
+  state_modified |= ui::GestureRecognizer::Get()->CancelActiveTouches(this);
+  state_modified |=
+      ui::GestureRecognizer::Get()->CleanupStateForConsumer(this);
+  for (Window::Windows::iterator iter = children_.begin();
+       iter != children_.end();
+       ++iter) {
+    state_modified |= (*iter)->CleanupGestureState();
+  }
+  return state_modified;
+}
+
 void Window::OnPaintLayer(gfx::Canvas* canvas) {
   Paint(canvas);
 }
diff --git a/ui/aura/window.h b/ui/aura/window.h
index 2324533..6fbea36 100644
--- a/ui/aura/window.h
+++ b/ui/aura/window.h
@@ -326,6 +326,9 @@ class AURA_EXPORT Window : public ui::LayerDelegate,
   void PrintWindowHierarchy(int depth) const;
 #endif
 
+  // Returns true if there was state needing to be cleaned up.
+  bool CleanupGestureState();
+
  protected:
   // Deletes (or removes if not owned by parent) all child windows. Intended for
   // use from the destructor.
diff --git a/ui/aura/window_event_dispatcher.cc b/ui/aura/window_event_dispatcher.cc
index 5489596..36d8e6c 100644
--- a/ui/aura/window_event_dispatcher.cc
+++ b/ui/aura/window_event_dispatcher.cc
@@ -307,7 +307,7 @@ void WindowEventDispatcher::OnWindowHidden(Window* invisible,
   if (invisible->Contains(old_dispatch_target_))
     old_dispatch_target_ = NULL;
 
-  CleanupGestureState(invisible);
+  invisible->CleanupGestureState();
 
   // Do not clear the capture, and the |event_dispatch_target_| if the
   // window is moving across hosts, because the target itself is actually still
@@ -333,17 +333,6 @@ void WindowEventDispatcher::OnWindowHidden(Window* invisible,
   }
 }
 
-void WindowEventDispatcher::CleanupGestureState(Window* window) {
-  ui::GestureRecognizer::Get()->CancelActiveTouches(window);
-  ui::GestureRecognizer::Get()->CleanupStateForConsumer(window);
-  const Window::Windows& windows = window->children();
-  for (Window::Windows::const_iterator iter = windows.begin();
-      iter != windows.end();
-      ++iter) {
-    CleanupGestureState(*iter);
-  }
-}
-
 Window* WindowEventDispatcher::GetGestureTarget(ui::GestureEvent* event) {
   Window* target = NULL;
   if (!event->IsEndingEvent()) {
diff --git a/ui/aura/window_event_dispatcher.h b/ui/aura/window_event_dispatcher.h
index f0cb67f..6b476b7 100644
--- a/ui/aura/window_event_dispatcher.h
+++ b/ui/aura/window_event_dispatcher.h
@@ -162,10 +162,6 @@ class AURA_EXPORT WindowEventDispatcher : public ui::EventProcessor,
   // on capture (like DragDropTracker).
   void OnWindowHidden(Window* invisible, WindowHiddenReason reason);
 
-  // Cleans up the state of gestures for all windows in |window| (including
-  // |window| itself). This includes cancelling active touch points.
-  void CleanupGestureState(Window* window);
-
   // Returns a target window for the given gesture event.
   Window* GetGestureTarget(ui::GestureEvent* event);
 
diff --git a/ui/events/gestures/gesture_recognizer.h b/ui/events/gestures/gesture_recognizer.h
index f33a7a7..4b31e57 100644
--- a/ui/events/gestures/gesture_recognizer.h
+++ b/ui/events/gestures/gesture_recognizer.h
@@ -36,8 +36,9 @@ class EVENTS_EXPORT GestureRecognizer {
                                                 GestureConsumer* consumer) = 0;
 
   // This is called when the consumer is destroyed. So this should cleanup any
-  // internal state maintained for |consumer|.
-  virtual void CleanupStateForConsumer(GestureConsumer* consumer) = 0;
+  // internal state maintained for |consumer|. Returns true iff there was
+  // state relating to |consumer| to clean up.
+  virtual bool CleanupStateForConsumer(GestureConsumer* consumer) = 0;
 
   // Return the window which should handle this TouchEvent, in the case where
   // the touch is already associated with a target.
@@ -71,8 +72,9 @@ class EVENTS_EXPORT GestureRecognizer {
   virtual bool GetLastTouchPointForTarget(GestureConsumer* consumer,
                                           gfx::PointF* point) = 0;
 
-  // Sends a touch cancel event for every active touch.
-  virtual void CancelActiveTouches(GestureConsumer* consumer) = 0;
+  // Sends a touch cancel event for every active touch. Returns true iff any
+  // touch cancels were sent.
+  virtual bool CancelActiveTouches(GestureConsumer* consumer) = 0;
 
   // Subscribes |helper| for dispatching async gestures such as long press.
   // The Gesture Recognizer does NOT take ownership of |helper| and it is the
diff --git a/ui/events/gestures/gesture_recognizer_impl.cc b/ui/events/gestures/gesture_recognizer_impl.cc
index 5a70f51..7e04463 100644
--- a/ui/events/gestures/gesture_recognizer_impl.cc
+++ b/ui/events/gestures/gesture_recognizer_impl.cc
@@ -28,15 +28,19 @@ void TransferConsumer(GestureConsumer* current_consumer,
   }
 }
 
-void RemoveConsumerFromMap(GestureConsumer* consumer,
+bool RemoveConsumerFromMap(GestureConsumer* consumer,
                            GestureRecognizerImpl::TouchIdToConsumerMap* map) {
+  bool consumer_removed = false;
   for (GestureRecognizerImpl::TouchIdToConsumerMap::iterator i = map->begin();
        i != map->end();) {
-    if (i->second == consumer)
+    if (i->second == consumer) {
       map->erase(i++);
-    else
+      consumer_removed = true;
+    } else {
       ++i;
+    }
   }
+  return consumer_removed;
 }
 
 void TransferTouchIdToConsumerMap(
@@ -150,15 +154,16 @@ bool GestureRecognizerImpl::GetLastTouchPointForTarget(
   return true;
 }
 
-void GestureRecognizerImpl::CancelActiveTouches(
-    GestureConsumer* consumer) {
+bool GestureRecognizerImpl::CancelActiveTouches(GestureConsumer* consumer) {
   std::vector<std::pair<int, GestureConsumer*> > ids;
   for (TouchIdToConsumerMap::const_iterator i = touch_id_target_.begin();
        i != touch_id_target_.end(); ++i) {
     if (i->second == consumer)
       ids.push_back(std::make_pair(i->first, i->second));
   }
+  bool cancelled_touch = !ids.empty();
   CancelTouches(&ids);
+  return cancelled_touch;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -218,14 +223,19 @@ GestureSequence::Gestures* GestureRecognizerImpl::ProcessTouchEventForGesture(
   return gesture_sequence->ProcessTouchEventForGesture(event, result);
 }
 
-void GestureRecognizerImpl::CleanupStateForConsumer(GestureConsumer* consumer) {
+bool GestureRecognizerImpl::CleanupStateForConsumer(
+    GestureConsumer* consumer) {
+  bool state_cleaned_up = false;
   if (consumer_sequence_.count(consumer)) {
+    state_cleaned_up = true;
     delete consumer_sequence_[consumer];
     consumer_sequence_.erase(consumer);
   }
 
-  RemoveConsumerFromMap(consumer, &touch_id_target_);
-  RemoveConsumerFromMap(consumer, &touch_id_target_for_gestures_);
+  state_cleaned_up |= RemoveConsumerFromMap(consumer, &touch_id_target_);
+  state_cleaned_up |=
+      RemoveConsumerFromMap(consumer, &touch_id_target_for_gestures_);
+  return state_cleaned_up;
 }
 
 void GestureRecognizerImpl::AddGestureEventHelper(GestureEventHelper* helper) {
diff --git a/ui/events/gestures/gesture_recognizer_impl.h b/ui/events/gestures/gesture_recognizer_impl.h
index 5de287f..f136894 100644
--- a/ui/events/gestures/gesture_recognizer_impl.h
+++ b/ui/events/gestures/gesture_recognizer_impl.h
@@ -44,7 +44,7 @@ class EVENTS_EXPORT GestureRecognizerImpl : public GestureRecognizer,
                                 GestureConsumer* new_consumer) OVERRIDE;
   virtual bool GetLastTouchPointForTarget(GestureConsumer* consumer,
                                           gfx::PointF* point) OVERRIDE;
-  virtual void CancelActiveTouches(GestureConsumer* consumer) OVERRIDE;
+  virtual bool CancelActiveTouches(GestureConsumer* consumer) OVERRIDE;
 
  protected:
   virtual GestureSequence* CreateSequence(GestureSequenceDelegate* delegate);
@@ -60,7 +60,8 @@ class EVENTS_EXPORT GestureRecognizerImpl : public GestureRecognizer,
       const TouchEvent& event,
       ui::EventResult result,
       GestureConsumer* target) OVERRIDE;
-  virtual void CleanupStateForConsumer(GestureConsumer* consumer) OVERRIDE;
+  virtual bool CleanupStateForConsumer(GestureConsumer* consumer)
+      OVERRIDE;
   virtual void AddGestureEventHelper(GestureEventHelper* helper) OVERRIDE;
   virtual void RemoveGestureEventHelper(GestureEventHelper* helper) OVERRIDE;