Fix Media Router context menu crashes

When the context menu is closed, destroy the menu model asynchronously,
so that its command gets executed before the menu model is destroyed.

Bug: 861652, 861655
Change-Id: Id0bf068e600f13ab187c0ad137ca15979363c61f
Reviewed-on: https://chromium-review.googlesource.com/1130657
Commit-Queue: Takumi Fujimoto <takumif@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575478}

chrome/browser/ui/toolbar/media_router_action.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/toolbar/media_router_action.h"
 
+#include "base/bind.h"
+#include "base/location.h"
 #include "base/metrics/user_metrics.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/media/router/media_router.h"
@@ -22,6 +24,7 @@
 #include "chrome/common/media_router/media_route.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/vector_icons/vector_icons.h"
+#include "content/public/browser/browser_thread.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/image/image_skia.h"
@@ -137,6 +140,9 @@ gfx::NativeView MediaRouterAction::GetPopupNativeView() {
 }
 
 ui::MenuModel* MediaRouterAction::GetContextMenu() {
+  // If there is an existing context menu, destroy it before we instantiate a
+  // new one.
+  DestroyContextMenu();
   MediaRouterActionController* controller =
       media_router::MediaRouterUIService::Get(browser_->profile())
           ->action_controller();
@@ -155,7 +161,14 @@ void MediaRouterAction::OnContextMenuClosed() {
       !GetMediaRouterDialogController()->IsShowingMediaRouterDialog()) {
     toolbar_actions_bar_->UndoPopOut();
   }
-  contextual_menu_.reset();
+  // We must destroy the context menu asynchronously to prevent it from being
+  // destroyed before the command execution.
+  // TODO(takumif): Using task sequence to order operations is fragile. Consider
+  // other ways to do so when we move the icon to the trusted area.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::BindOnce(&MediaRouterAction::DestroyContextMenu,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 bool MediaRouterAction::ExecuteAction(bool by_user) {
@@ -302,3 +315,7 @@ const gfx::VectorIcon& MediaRouterAction::GetCurrentIcon() const {
   return has_local_display_route_ ? vector_icons::kMediaRouterActiveIcon
                                   : vector_icons::kMediaRouterIdleIcon;
 }
+
+void MediaRouterAction::DestroyContextMenu() {
+  contextual_menu_.reset();
+}

chrome/browser/ui/toolbar/media_router_action.h

@@ -115,6 +115,8 @@ class MediaRouterAction : public ToolbarActionViewController,
 
   const gfx::VectorIcon& GetCurrentIcon() const;
 
+  void DestroyContextMenu();
+
   // The current icon to show. This is updated based on the current issues and
   // routes since |this| is an IssueObserver and MediaRoutesObserver.
   const gfx::VectorIcon* current_icon_;

chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc

@@ -97,10 +97,14 @@ class MediaRouterUIBrowserTest : public InProcessBrowserTest {
     nav_observer.StopWatchingNewWebContents();
   }
 
-  MediaRouterAction* GetMediaRouterAction() {
+  // Returns the dialog controller for the active WebContents.
+  MediaRouterDialogControllerImplBase* GetDialogController() {
     return MediaRouterDialogControllerImplBase::GetOrCreateForWebContents(
-               browser()->tab_strip_model()->GetActiveWebContents())
-        ->action();
+        browser()->tab_strip_model()->GetActiveWebContents());
+  }
+
+  MediaRouterAction* GetMediaRouterAction() {
+    return GetDialogController()->action();
   }
 
   ui::SimpleMenuModel* GetActionContextMenu() {
@@ -134,9 +138,7 @@ class MediaRouterUIBrowserTest : public InProcessBrowserTest {
 
     content::WebContents* web_contents =
         browser()->tab_strip_model()->GetActiveWebContents();
-    MediaRouterDialogController* dialog_controller =
-        MediaRouterDialogController::GetOrCreateForWebContents(
-            browser()->tab_strip_model()->GetActiveWebContents());
+    MediaRouterDialogController* dialog_controller = GetDialogController();
     content::ContextMenuParams params;
     params.page_url =
         web_contents->GetController().GetLastCommittedEntry()->GetURL();
@@ -164,9 +166,7 @@ class MediaRouterUIBrowserTest : public InProcessBrowserTest {
         test::AppMenuTestApi::Create(browser());
     app_menu_test_api->ShowMenu();
 
-    MediaRouterDialogController* dialog_controller =
-        MediaRouterDialogController::GetOrCreateForWebContents(
-            browser()->tab_strip_model()->GetActiveWebContents());
+    MediaRouterDialogController* dialog_controller = GetDialogController();
     ASSERT_FALSE(dialog_controller->IsShowingMediaRouterDialog());
     app_menu_test_api->ExecuteCommand(IDC_ROUTE_MEDIA);
     EXPECT_TRUE(dialog_controller->IsShowingMediaRouterDialog());
@@ -180,9 +180,7 @@ class MediaRouterUIBrowserTest : public InProcessBrowserTest {
   }
 
   void TestEphemeralToolbarIconForDialog() {
-    MediaRouterDialogController* dialog_controller =
-        MediaRouterDialogController::GetOrCreateForWebContents(
-            browser()->tab_strip_model()->GetActiveWebContents());
+    MediaRouterDialogController* dialog_controller = GetDialogController();
 
     EXPECT_FALSE(ActionExists());
     dialog_controller->ShowMediaRouterDialog();
@@ -211,6 +209,31 @@ class MediaRouterUIBrowserTest : public InProcessBrowserTest {
     EXPECT_FALSE(ActionExists());
   }
 
+  void TestPinAndUnpinToolbarIcon() {
+    GetDialogController()->ShowMediaRouterDialog();
+    EXPECT_TRUE(ActionExists());
+
+    // Pin the icon via its context menu.
+    ui::SimpleMenuModel* context_menu = GetActionContextMenu();
+    const int command_index = context_menu->GetIndexOfCommandId(
+        IDC_MEDIA_ROUTER_ALWAYS_SHOW_TOOLBAR_ACTION);
+    if (IsCocoaBrowser()) {
+      // With Cocoa, OnContextMenuClosed() gets called before command execution.
+      GetMediaRouterAction()->OnContextMenuClosed();
+      context_menu->ActivatedAt(command_index);
+    } else {
+      context_menu->ActivatedAt(command_index);
+      GetMediaRouterAction()->OnContextMenuClosed();
+    }
+    GetDialogController()->HideMediaRouterDialog();
+    EXPECT_TRUE(ActionExists());
+
+    // Unpin the icon via its context menu.
+    GetActionContextMenu()->ActivatedAt(command_index);
+    GetMediaRouterAction()->OnContextMenuClosed();
+    EXPECT_FALSE(ActionExists());
+  }
+
   ToolbarActionsBar* toolbar_actions_bar_ = nullptr;
 
   Issue issue_;
@@ -393,6 +416,10 @@ IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest, UpdateActionLocation) {
       toolbar_actions_bar_->IsActionVisibleOnMainBar(GetMediaRouterAction()));
 }
 
+IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest, PinAndUnpinToolbarIcon) {
+  TestPinAndUnpinToolbarIcon();
+}
+
 // Runs dialog-related tests with the Views Cast dialog.
 class MediaRouterViewsUIBrowserTest : public MediaRouterUIBrowserTest {
  protected:
@@ -424,4 +451,10 @@ IN_PROC_BROWSER_TEST_F(MediaRouterViewsUIBrowserTest,
   TestEphemeralToolbarIconForDialog();
 }
 
+IN_PROC_BROWSER_TEST_F(MediaRouterViewsUIBrowserTest, PinAndUnpinToolbarIcon) {
+  if (IsCocoaBrowser())
+    return;
+  TestPinAndUnpinToolbarIcon();
+}
+
 }  // namespace media_router