Fix crash under ExclusiveAccessBubbleViews::AnimationProgressed().

chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/exclusive_access/fullscreen_controller_test.h"
+#include "chrome/browser/ui/views/exclusive_access_bubble_views.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_observer.h"
+
+class ExclusiveAccessBubbleViewsTest : public FullscreenControllerTest,
+                                       public views::WidgetObserver {
+ public:
+  ExclusiveAccessBubbleViewsTest() {}
+
+  ExclusiveAccessBubbleViews* bubble() {
+    BrowserView* browser_view =
+        BrowserView::GetBrowserViewForBrowser(browser());
+    return browser_view->exclusive_access_bubble();
+  }
+
+  // WidgetObserver:
+  void OnWidgetDestroying(views::Widget* widget) override {
+    was_observing_in_destroying_ = widget->HasObserver(bubble());
+    was_destroying_ = true;
+  }
+  void OnWidgetDestroyed(views::Widget* widget) override {
+    widget->RemoveObserver(this);

[msw, 2017/05/03 18:48:33]

optional nit: move this into OnWidgetDestroying?

[tapted, 2017/05/04 06:42:08]

Done.

+  }
+
+ protected:
+  bool was_destroying_ = false;
+  bool was_observing_in_destroying_ = false;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExclusiveAccessBubbleViewsTest);
+};
+
+// Simulate obscure codepaths resulting in the bubble Widget being Closed before

[msw, 2017/05/03 18:48:34]

nit: "closed"?

[tapted, 2017/05/04 06:42:08]

Done.

+// the ExclusiveAccessBubbleViews destructor will tear down the bubble and any

[msw, 2017/05/03 18:48:34]

nit: this sentence is a bit run-on and could be clearer

[tapted, 2017/05/04 06:42:07]

Done.

+// running animations that could trigger crashes.
+IN_PROC_BROWSER_TEST_F(ExclusiveAccessBubbleViewsTest, NativeClose) {

[msw, 2017/05/03 18:48:34]

nit: maybe make a test for the normal close codepath? (perhaps that exists)

[tapted, 2017/05/04 06:42:07]

Yup - most of the tests in FullscreenControllerInteractiveTest exercise the
regular codepaths. E.g. those doing
ASSERT_{TRUE,FALSE}(IsFullscreenBubbleDisplayed());
 
(and.. at least 4 of those tests are not disabled for being flaky... perhaps
this bug I'm fixing is even the cause of that flakiness...)

I've been checking up on them occasionally to ensure the disabled tests still
pass. E.g. http://crbug.com/651272 . (and I just ran them again and all 19 are
passing.. at least on Mac).

+  EXPECT_FALSE(bubble());
+  EnterActiveTabFullscreen();
+  EXPECT_TRUE(bubble());
+
+  bubble()->GetView()->GetWidget()->AddObserver(this);
+
+  // It's hard to nail down a codepath that results in the bubble Widget being

[msw, 2017/05/03 18:48:34]

Maybe this comment should just say "Simulate the bubble being closed out from
under its controller, which seems to happen in some odd corner cases, like
system log-off while the bubble is showing."

[tapted, 2017/05/04 06:42:07]

Done. Yeah is probably too much info.

+  // destroyed before the call to the ExclusiveAccessBubbleViewsTest destructor.

[msw, 2017/05/03 18:48:34]

q: Did you mean ExclusiveAccessBubbleViewsTest? (I'm not sure why the test dtor
is relevant here, it seems like you mean some other bubble/controller component)

[tapted, 2017/05/04 06:42:07]

Ahh yep - I meant the ExclusiveAccessBubbleViews destructor.

+  // CloseAllTabs() will destroy the bubble via the ExclusiveAccessBubbleViews
+  // destructor (via RenderViewTerminated -> ExitFullscreenMode).
+  // CloseAllBrowsers() does a graceful exit via Browser::OnWindowClosing()
+  // (which triggers CloseAllTabs()).
+  // However, when closing via BrowserWindow::DestroyBrowser(), BrowserView
+  // first stops observing its Widget, so never sees an OnWindowClosing().
+  // DestroyBrowser is triggered by chrome::SessionEnding() which ends up in
+  // RenderProcessHostImpl::FastShutdownIfPossible() and only if fast shutdown
+  // IS possible, does it call ProcessDied() which also results in graceful
+  // termination via RenderViewTerminated.
+  // If fast shutdown is NOT possible (e.g. unload handlers), then the browser
+  // window can be torn down before RenderViewTerminated is sent and since the
+  // bubble is a child window it will get town down as well.
+  // That's all a bit complicated for a test, but it boils down to the window
+  // being closed by the OS or by DesktopWindowTreeHost::OnHostClosed() rather
+  // than by Widget::Close, and that needs to be handled. Widget::CloseNow() is
+  // a close approximation of this.
+  bubble()->GetView()->GetWidget()->CloseNow();

[msw, 2017/05/03 18:48:34]

aside: you could also potentially delete the bubble's NativeWidget?

[tapted, 2017/05/04 06:42:07]

That works for NativeWidgetAura, and
views::PlatformTestHelper::SimulateNativeDestroy() does that for
NativeWidgetAura and does an equivalent thing for Mac. However,
DesktopNativeWidgetAura (i.e. DesktopWindowTreeHostWin) is a bit different. But
the way that CloseNow() is implemented turns out to do a reasonable thing for
all platforms.

+  EXPECT_FALSE(bubble());
+
+  // Verify that teardown is really happening via OnWidgetDestroyed() rather
+  // than the usual path via the ExclusiveAccessBubbleViews destructor. Since
+  // the destructor always first removes ExclusiveAccessBubbleViews as an
+  // observer before starting the close, checking in OnWidgetDestroyed that it's
+  // still observing achieves this.
+  EXPECT_TRUE(was_observing_in_destroying_);
+  EXPECT_TRUE(was_destroying_);
+}