commit 712b2013aa586275c6c3b827979c4fee6da1adce
author Jinsuk Kim <jinsukkim@chromium.org> Mon Jun 18 17:52:43 2018
committer Commit Bot <commit-bot@chromium.org> Mon Jun 18 17:52:43 2018
tree 703054d486e1656b1ffed5fd01da563b0e386815
parent 9e073a82516fb034f311904cd8a07718dcc9cb86

Android: Ensure |OnDetachedFromWindow| is invoked upon destruction

https://crrev.com/c/1020942 meant to make sure |DetachedFromWindow| is
invoked upon ViewAndroid destruction. This had a bug when
WindowAndroid is destroyed first, which can happen for Chromecast
where WebContents destruction is delayed by design.

WindowAndroid dtor invokes |GetWindowAndroid| indirectly via its
base dtor (through RemoveAllChildren), but it didn't work as intended
because |GetWindowAndroid| is a virtual function. So it ended up
calling |ViewAndroid::GetWindowAndroid|, not
|WindowAndroid::GetWindowAndroid|.

This CL fixes it by pulling out the task of invoking
|OnDetachedFromWindow| so that the destructor won't use the virtual
function. WindowAndroid can call |OnDetachedFromWindow| in its dtor
since it doesn't need |GetWindowAndroid| - it knows for sure
WindowAndroid (itself) is present.


Bug: b/78251221
Change-Id: I754e45ca3ea61ceb86101ede3b529e75e206689f
Reviewed-on: https://chromium-review.googlesource.com/1102823
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568064}

ui/android/view_android.cc

diff --git a/ui/android/view_android.cc b/ui/android/view_android.cc
index 7a17c28..ec031a8 100644
--- a/ui/android/view_android.cc
+++ b/ui/android/view_android.cc
@@ -88,7 +88,7 @@
 ViewAndroid::ViewAndroid() : ViewAndroid(LayoutType::NORMAL) {}
 
 ViewAndroid::~ViewAndroid() {
-  RemoveAllChildren();
+  RemoveAllChildren(GetWindowAndroid() != nullptr);
   observer_list_.Clear();
   RemoveFromParent();
 }
@@ -252,11 +252,10 @@
   return gfx::PointF(x + loc_x, y + loc_y);
 }
 
-void ViewAndroid::RemoveAllChildren() {
-  bool has_window = GetWindowAndroid();
+void ViewAndroid::RemoveAllChildren(bool attached_to_window) {
   auto it = children_.begin();
   while (it != children_.end()) {
-    if (has_window)
+    if (attached_to_window)
       (*it)->OnDetachedFromWindow();
     (*it)->parent_ = nullptr;
     // erase returns a new iterator for the element following the ereased one.

ui/android/view_android.h

diff --git a/ui/android/view_android.h b/ui/android/view_android.h
index c723346..5d2d2a2 100644
--- a/ui/android/view_android.h
+++ b/ui/android/view_android.h
@@ -179,6 +179,8 @@
   ViewAndroid* parent() const { return parent_; }
 
  protected:
+  void RemoveAllChildren(bool attached_to_window);
+
   ViewAndroid* parent_;
 
  private:
@@ -204,7 +206,6 @@
   bool ScrollTo(float x, float y);
 
   void RemoveChild(ViewAndroid* child);
-  void RemoveAllChildren();
 
   void OnAttachedToWindow();
   void OnDetachedFromWindow();

ui/android/window_android.cc

diff --git a/ui/android/window_android.cc b/ui/android/window_android.cc
index 9b28a66e..f099ea08 100644
--- a/ui/android/window_android.cc
+++ b/ui/android/window_android.cc
@@ -152,6 +152,7 @@
 WindowAndroid::~WindowAndroid() {
   DCHECK(parent_ == nullptr) << "WindowAndroid must be a root view.";
   DCHECK(!compositor_);
+  RemoveAllChildren(true);
   Java_WindowAndroid_clearNativePointer(AttachCurrentThread(), GetJavaObject());
 }