From c317a8803f1245c2f5d7e8e50e4b54745945c273 Mon Sep 17 00:00:00 2001 From: brave-builds <45370463+brave-builds@users.noreply.github.com> Date: Wed, 13 Dec 2023 07:35:54 +0100 Subject: [PATCH] Fixed crash when running extension item from app menu in private window (uplift to 1.62.x) (#21339) Uplift of #21331 (squashed) to beta --- app/brave_main_delegate_browsertest.cc | 1 - browser/ui/toolbar/app_menu_icons.cc | 1 + browser/ui/toolbar/brave_app_menu_model.cc | 37 ++++++++++++------- browser/ui/toolbar/brave_app_menu_model.h | 1 + .../brave_app_menu_model_browsertest.cc | 20 ++++++++-- chromium_src/chrome/browser/ui/ui_features.cc | 1 - 6 files changed, 41 insertions(+), 20 deletions(-) diff --git a/app/brave_main_delegate_browsertest.cc b/app/brave_main_delegate_browsertest.cc index f005361f5172..f1dde63deb5a 100644 --- a/app/brave_main_delegate_browsertest.cc +++ b/app/brave_main_delegate_browsertest.cc @@ -192,7 +192,6 @@ IN_PROC_BROWSER_TEST_F(BraveMainDelegateBrowserTest, DisabledFeatures) { #endif &features::kDigitalGoodsApi, &features::kDIPS, - &features::kExtensionsMenuInAppMenu, &features::kFedCm, &features::kFedCmWithoutThirdPartyCookies, &features::kFirstPartySets, diff --git a/browser/ui/toolbar/app_menu_icons.cc b/browser/ui/toolbar/app_menu_icons.cc index 08fa5f4886f4..7d8bd867c819 100644 --- a/browser/ui/toolbar/app_menu_icons.cc +++ b/browser/ui/toolbar/app_menu_icons.cc @@ -37,6 +37,7 @@ const std::map& GetCommandIcons() { {IDC_VIEW_PASSWORDS, kLeoKeyIcon}, {IDC_SHOW_DOWNLOADS, kLeoDownloadIcon}, {IDC_MANAGE_EXTENSIONS, kLeoBrowserExtensionsIcon}, + {IDC_EXTENSIONS_SUBMENU_MANAGE_EXTENSIONS, kLeoBrowserExtensionsIcon}, {IDC_ZOOM_MENU, kLeoSearchZoomInIcon}, {IDC_PRINT, kLeoPrintIcon}, {IDC_MORE_TOOLS_MENU, kLeoWindowScrewdriverIcon}, diff --git a/browser/ui/toolbar/brave_app_menu_model.cc b/browser/ui/toolbar/brave_app_menu_model.cc index 99a452a3414b..115566ab3b87 100644 --- a/browser/ui/toolbar/brave_app_menu_model.cc +++ b/browser/ui/toolbar/brave_app_menu_model.cc @@ -296,10 +296,22 @@ void BraveAppMenuModel::BuildBrowserSection() { IDS_SHOW_DOWNLOADS); } + // Use this command's enabled state to not having it in guest window. + // It's disabled in guest window. Upstream's guest window has extensions + // menu in app menu, but we hide it. if (IsCommandIdEnabled(IDC_MANAGE_EXTENSIONS)) { + // Upstream enabled extensions submenu by default. + CHECK(features::IsExtensionMenuInRootAppMenu()); + + // Use IDC_EXTENSIONS_SUBMENU_MANAGE_EXTENSIONS instead of + // IDC_MANAGE_EXTENSIONS because executing it from private(tor) window + // causes crash as LogSafetyHubInteractionMetrics() tries to refer + // SafetyHubMenuNotificationService. But it's not instantiated in private + // window. Upstream also has this crash if ExtensionsMenuInAppMenu feature + // is disabled. InsertItemWithStringIdAt( GetIndexOfCommandId(IDC_SHOW_DOWNLOADS).value() + 1, - IDC_MANAGE_EXTENSIONS, IDS_SHOW_EXTENSIONS); + IDC_EXTENSIONS_SUBMENU_MANAGE_EXTENSIONS, IDS_SHOW_EXTENSIONS); } } @@ -384,19 +396,11 @@ void BraveAppMenuModel::RemoveUpstreamMenus() { DCHECK(more_tools_model); // Remove upstream's extensions item. It'll be added into top level third - // section. - if (base::FeatureList::IsEnabled(features::kExtensionsMenuInAppMenu) || - features::IsChromeRefresh2023()) { - // Hide extensions sub menu. - DCHECK(GetIndexOfCommandId(IDC_EXTENSIONS_SUBMENU).has_value()); - RemoveItemAt(GetIndexOfCommandId(IDC_EXTENSIONS_SUBMENU).value()); - } else { - // Hide extensions item from more tools sub menu. - DCHECK(more_tools_model->GetIndexOfCommandId(IDC_MANAGE_EXTENSIONS) - .has_value()); - more_tools_model->RemoveItemAt( - more_tools_model->GetIndexOfCommandId(IDC_MANAGE_EXTENSIONS).value()); - } + // section. Upstream enabled extensions submenu by default. + CHECK(features::IsExtensionMenuInRootAppMenu()); + // Hide extensions sub menu. + DCHECK(GetIndexOfCommandId(IDC_EXTENSIONS_SUBMENU).has_value()); + RemoveItemAt(GetIndexOfCommandId(IDC_EXTENSIONS_SUBMENU).value()); // Remove upstream's cast item. It'll be added into more tools sub menu. if (media_router::MediaRouterEnabled(browser()->profile())) { @@ -494,6 +498,11 @@ bool BraveAppMenuModel::IsCommandIdEnabled(int id) const { IsIpfsServiceLaunched(browser_context); } #endif + if (id == IDC_EXTENSIONS_SUBMENU_MANAGE_EXTENSIONS) { + // Always returns true as this command id is only added when it could be + // used. + return true; + } return AppMenuModel::IsCommandIdEnabled(id); } diff --git a/browser/ui/toolbar/brave_app_menu_model.h b/browser/ui/toolbar/brave_app_menu_model.h index aaa56c2a8908..b1865812b45e 100644 --- a/browser/ui/toolbar/brave_app_menu_model.h +++ b/browser/ui/toolbar/brave_app_menu_model.h @@ -37,6 +37,7 @@ class BraveAppMenuModel : public AppMenuModel { private: FRIEND_TEST_ALL_PREFIXES(BraveAppMenuModelBrowserTest, BraveIpfsMenuTest); + friend class BraveAppMenuModelBrowserTest; // AppMenuModel overrides: void Build() override; diff --git a/browser/ui/toolbar/brave_app_menu_model_browsertest.cc b/browser/ui/toolbar/brave_app_menu_model_browsertest.cc index 962d16c672d1..d0d761e2297f 100644 --- a/browser/ui/toolbar/brave_app_menu_model_browsertest.cc +++ b/browser/ui/toolbar/brave_app_menu_model_browsertest.cc @@ -90,6 +90,13 @@ class BraveAppMenuModelBrowserTest : public InProcessBrowserTest { ipfs_service_->GetIpnsKeysManager()->SetKeysForTest(keys); } #endif + + void RunCommandFromAppMenuModel(Browser* browser, int command_id) { + auto* browser_view = BrowserView::GetBrowserViewForBrowser(browser); + BraveAppMenuModel model(browser_view->toolbar(), browser); + model.Init(); + model.ExecuteCommand(command_id, /*event_flags=*/0); + } }; void CheckCommandsAreDisabledInMenuModel( @@ -190,6 +197,11 @@ void CheckHistoryCommandsAreInOrderInMenuModel( CheckCommandsAreInOrderInMenuModel(history_model, history_commands_in_order); } +IN_PROC_BROWSER_TEST_F(BraveAppMenuModelBrowserTest, CommandsExecutionTest) { + RunCommandFromAppMenuModel(CreateIncognitoBrowser(), + IDC_EXTENSIONS_SUBMENU_MANAGE_EXTENSIONS); +} + // Test brave menu order test. // Brave menu is inserted based on corresponding commands enable status. // So, this doesn't test for each profiles(normal, private, tor and guest). @@ -209,7 +221,7 @@ IN_PROC_BROWSER_TEST_F(BraveAppMenuModelBrowserTest, MenuOrderTest) { IDC_RECENT_TABS_MENU, IDC_BOOKMARKS_MENU, IDC_SHOW_DOWNLOADS, - IDC_MANAGE_EXTENSIONS, + IDC_EXTENSIONS_SUBMENU_MANAGE_EXTENSIONS, IDC_ZOOM_MENU, IDC_PRINT, IDC_FIND, @@ -263,7 +275,7 @@ IN_PROC_BROWSER_TEST_F(BraveAppMenuModelBrowserTest, MenuOrderTest) { IDC_SHOW_BRAVE_WALLET, IDC_BOOKMARKS_MENU, IDC_SHOW_DOWNLOADS, - IDC_MANAGE_EXTENSIONS, + IDC_EXTENSIONS_SUBMENU_MANAGE_EXTENSIONS, IDC_ZOOM_MENU, IDC_PRINT, IDC_FIND, @@ -316,7 +328,7 @@ IN_PROC_BROWSER_TEST_F(BraveAppMenuModelBrowserTest, MenuOrderTest) { #endif IDC_RECENT_TABS_MENU, IDC_BOOKMARKS_MENU, - IDC_MANAGE_EXTENSIONS, + IDC_EXTENSIONS_SUBMENU_MANAGE_EXTENSIONS, }; CheckCommandsAreDisabledInMenuModel(guest_browser, @@ -355,7 +367,7 @@ IN_PROC_BROWSER_TEST_F(BraveAppMenuModelBrowserTest, MenuOrderTest) { IDC_SHOW_BRAVE_WALLET, IDC_BOOKMARKS_MENU, IDC_SHOW_DOWNLOADS, - IDC_MANAGE_EXTENSIONS, + IDC_EXTENSIONS_SUBMENU_MANAGE_EXTENSIONS, IDC_ZOOM_MENU, IDC_PRINT, IDC_FIND, diff --git a/chromium_src/chrome/browser/ui/ui_features.cc b/chromium_src/chrome/browser/ui/ui_features.cc index c5cdbde485b5..995f7adf1c98 100644 --- a/chromium_src/chrome/browser/ui/ui_features.cc +++ b/chromium_src/chrome/browser/ui/ui_features.cc @@ -16,7 +16,6 @@ OVERRIDE_FEATURE_DEFAULT_STATES({{ {kHaTSWebUI, base::FEATURE_DISABLED_BY_DEFAULT}, #endif {kTabHoverCardImages, base::FEATURE_DISABLED_BY_DEFAULT}, - {kExtensionsMenuInAppMenu, base::FEATURE_DISABLED_BY_DEFAULT}, }}); } // namespace features