cros: Fix startup page focus

- Move RestoreFocus from BrowserView::Show to OnWidgetActivationChanged
  to do it when the browser window is activated;
- Remove SetInitialFocus in SessionRestoreImpl::ShowBrowser since
  BrowserView change should do that as part of RestoreFocus;
- Also remove the debugging code for SetInitialFocus crash
  under SessionRestoreImpl::RestoreTab since ShowBrowser no longer
  calls SetInitialFocus;

Bug: 859257, 850626, 908524
Change-Id: Ia24e4d68e1386de84765f914bb75319c9887dfd1
Reviewed-on: https://chromium-review.googlesource.com/c/1313728
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614364}

chrome/browser/sessions/session_restore.cc

@@ -45,7 +45,6 @@
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #include "chrome/common/extensions/extension_metrics.h"
 #include "chrome/common/url_constants.h"
 #include "components/keep_alive_registry/keep_alive_types.h"
@@ -62,7 +61,6 @@
 #include "content/public/browser/session_storage_namespace.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
-#include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/page_state.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension_set.h"
@@ -87,83 +85,6 @@ bool HasSingleNewTabPage(Browser* browser) {
          search::IsInstantNTP(active_tab);
 }
 
-// WebContentsDestructionChecker crashes if the WebContents that it's observing
-// gets destroyed.
-// TODO(crbug.com/850626): Remove after bug is fixed.
-class WebContentsDestructionChecker : public content::WebContentsObserver {
- public:
-  explicit WebContentsDestructionChecker(content::WebContents* contents)
-      : WebContentsObserver(contents) {}
-  ~WebContentsDestructionChecker() override = default;
-
-  const WebContents* contents() const { return web_contents(); }
-
-  // content::WebContentsObserver:
-  void WebContentsDestroyed() override {
-    LOG(FATAL) << "Restored WebContents " << web_contents() << " destroyed";
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(WebContentsDestructionChecker);
-};
-
-// TabStripRestoreObserver watches the next WebContents that's added to a
-// TabStripModel and crashes if it's closed or detached.
-// TODO(crbug.com/850626): Remove after bug is fixed.
-class TabStripRestoreObserver : public TabStripModelObserver {
- public:
-  explicit TabStripRestoreObserver(TabStripModel* tab_strip)
-      : tab_strip_(tab_strip) {
-    DCHECK(tab_strip_);
-    tab_strip_->AddObserver(this);
-  }
-  ~TabStripRestoreObserver() override { tab_strip_->RemoveObserver(this); }
-
-  // TabStripModelObserver:
-  void OnTabStripModelChanged(
-      TabStripModel* tab_strip_model,
-      const TabStripModelChange& change,
-      const TabStripSelectionChange& selection) override {
-    if (change.type() == TabStripModelChange::kInserted) {
-      for (const auto& delta : change.deltas())
-        OnTabInserted(delta.insert.contents);
-      return;
-    }
-
-    if (change.type() == TabStripModelChange::kRemoved) {
-      for (const auto& delta : change.deltas())
-        OnTabRemoved(delta.remove.contents, delta.remove.will_be_deleted);
-      return;
-    }
-  }
-
- private:
-  // Creates checker if needed.
-  void OnTabInserted(content::WebContents* contents) {
-    if (destruction_checker_)
-      return;
-
-    destruction_checker_ =
-        std::make_unique<WebContentsDestructionChecker>(contents);
-  }
-
-  void OnTabRemoved(content::WebContents* contents, bool will_be_deleted) {
-    if (!destruction_checker_)
-      return;
-
-    if (contents == destruction_checker_->contents())
-      LOG(FATAL) << "Restored WebContents " << contents
-                 << (will_be_deleted ? " closing" : " detached");
-  }
-
-  TabStripModel* tab_strip_;  // owned by caller
-
-  // Initialized after a WebContents is inserted into the strip.
-  std::unique_ptr<WebContentsDestructionChecker> destruction_checker_;
-
-  DISALLOW_COPY_AND_ASSIGN(TabStripRestoreObserver);
-};
-
 // Pointers to SessionRestoreImpls which are currently restoring the session.
 std::set<SessionRestoreImpl*>* active_session_restorers = nullptr;
 
@@ -661,9 +582,6 @@ class SessionRestoreImpl : public content::NotificationObserver {
               ->RecreateSessionStorage(tab.session_storage_persistent_id);
     }
 
-    // TODO(crbug.com/850626): Remove this after bug is fixed.
-    TabStripRestoreObserver tab_strip_observer(browser->tab_strip_model());
-
     WebContents* web_contents = chrome::AddRestoredTab(
         browser, tab.navigations, tab_index, selected_index,
         tab.extension_app_id, is_selected_tab, tab.pinned, true,
@@ -715,8 +633,6 @@ class SessionRestoreImpl : public content::NotificationObserver {
 
     browser->window()->Show();
     browser->set_is_session_restore(false);
-
-    browser->tab_strip_model()->GetActiveWebContents()->SetInitialFocus();
   }
 
   // Appends the urls in |urls| to |browser|.

chrome/browser/ui/startup/startup_browser_creator_interactive_uitest.cc

@@ -10,6 +10,8 @@
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/chrome_browser_main.h"
+#include "chrome/browser/chrome_browser_main_extra_parts.h"
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/sessions/session_restore.h"
@@ -17,12 +19,17 @@
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
-typedef InProcessBrowserTest StartupBrowserCreatorTest;
+#if defined(USE_AURA)
+#include "ui/aura/window.h"
+#endif
+
+using StartupBrowserCreatorTest = InProcessBrowserTest;
 
 // Chrome OS doesn't support multiprofile.
 // And BrowserWindow::IsActive() always returns false in tests on MAC.
@@ -100,3 +107,60 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTest, LastUsedProfileActivated) {
   EXPECT_FALSE(new_browser->window()->IsActive());
 }
 #endif  // !OS_MACOSX && !OS_CHROMEOS
+
+#if defined(USE_AURA)
+class StartupPagePrefSetterMainExtraParts : public ChromeBrowserMainExtraParts {
+ public:
+  explicit StartupPagePrefSetterMainExtraParts(const std::vector<GURL>& urls)
+      : urls_(urls) {}
+
+  // ChromeBrowserMainExtraParts:
+  void PreBrowserStart() override {
+    Profile* profile =
+        g_browser_process->profile_manager()->GetActiveUserProfile();
+
+    SessionStartupPref pref_urls(SessionStartupPref::URLS);
+    pref_urls.urls = std::move(urls_);
+    SessionStartupPref::SetStartupPref(profile, pref_urls);
+  }
+
+ private:
+  std::vector<GURL> urls_;
+  DISALLOW_COPY_AND_ASSIGN(StartupPagePrefSetterMainExtraParts);
+};
+
+class StartupPageTest : public InProcessBrowserTest {
+ public:
+  StartupPageTest() {
+    // Don't open about:blank since we want to test startup urls.
+    set_open_about_blank_on_browser_launch(false);
+  }
+  ~StartupPageTest() override = default;
+
+  // InProcessBrowserTest:
+  void CreatedBrowserMainParts(
+      content::BrowserMainParts* browser_main_parts) override {
+    const std::vector<GURL> urls = {ui_test_utils::GetTestUrl(
+        base::FilePath(FILE_PATH_LITERAL("focus")),
+        base::FilePath(FILE_PATH_LITERAL("page_with_focus.html")))};
+
+    ChromeBrowserMainParts* chrome_browser_main_parts =
+        static_cast<ChromeBrowserMainParts*>(browser_main_parts);
+    chrome_browser_main_parts->AddParts(
+        new StartupPagePrefSetterMainExtraParts(urls));
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(StartupPageTest);
+};
+
+IN_PROC_BROWSER_TEST_F(StartupPageTest, StartupPageFocus) {
+  // Browser window should be active.
+  EXPECT_TRUE(browser()->window()->IsActive());
+
+  // Focus should land in the content area.
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(contents->GetContentNativeView()->HasFocus());
+}
+#endif  // defined(USE_AURA)

chrome/browser/ui/views/frame/browser_view.cc

@@ -127,6 +127,7 @@
 #include "components/signin/core/browser/account_consistency_method.h"
 #include "components/translate/core/browser/language_state.h"
 #include "components/version_info/channel.h"
+#include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/keyboard_event_processing_result.h"
 #include "content/public/browser/notification_service.h"
@@ -305,6 +306,15 @@ bool GetGestureCommand(ui::GestureEvent* event, int* command) {
   return false;
 }
 
+bool IsShowingWebContentsModalDialog(content::WebContents* web_contents) {
+  if (!web_contents)
+    return false;
+
+  const web_modal::WebContentsModalDialogManager* manager =
+      web_modal::WebContentsModalDialogManager::FromWebContents(web_contents);
+  return manager && manager->IsDialogActive();
+}
+
 // A view targeter for the overlay view, which makes sure the overlay view
 // itself is never a target for events, but its children (i.e. top_container)
 // may be.
@@ -671,28 +681,13 @@ void BrowserView::Show() {
     return;
   }
 
-  // Showing the window doesn't make the browser window active right away.
-  // This can cause SetFocusToLocationBar() to skip setting focus to the
-  // location bar. To avoid this we explicilty let SetFocusToLocationBar()
-  // know that it's ok to steal focus.
-  force_location_bar_focus_ = true;
-
-  // Setting the focus doesn't work when the window is invisible, so any focus
-  // initialization that happened before this will be lost.
-  //
-  // We really "should" restore the focus whenever the window becomes unhidden,
-  // but I think initializing is the only time where this can happen where
-  // there is some focus change we need to pick up, and this is easier than
-  // plumbing through an un-hide message all the way from the frame.
-  //
-  // If we do find there are cases where we need to restore the focus on show,
-  // that should be added and this should be removed.
-  RestoreFocus();
+  // Only set |restore_focus_on_activation_| when it is not set so that restore
+  // focus on activation only happen once for the very first Show() call.
+  if (!restore_focus_on_activation_.has_value())
+    restore_focus_on_activation_ = true;
 
   frame_->Show();
 
-  force_location_bar_focus_ = false;
-
   browser()->OnWindowDidShow();
 
   MaybeShowInvertBubbleView(this);
@@ -1109,7 +1104,7 @@ void BrowserView::SetFocusToLocationBar(bool select_all) {
   // even if the widget doens't have a focus. Either cases, we need to ignore
   // this when the browser window isn't active.
 #if defined(OS_WIN) || defined(OS_CHROMEOS)
-  if (!force_location_bar_focus_ && !IsActive())
+  if (!IsActive())
     return;
 #endif
 
@@ -2115,10 +2110,21 @@ void BrowserView::OnWidgetDestroying(views::Widget* widget) {
 void BrowserView::OnWidgetActivationChanged(views::Widget* widget,
                                             bool active) {
   if (browser_->window()) {
-    if (active)
+    if (active) {
+      if (restore_focus_on_activation_.has_value() &&
+          restore_focus_on_activation_.value()) {
+        restore_focus_on_activation_ = false;
+
+        // Set initial focus change on the first activation if there is no
+        // tab modal dialog.
+        if (!IsShowingWebContentsModalDialog(GetActiveWebContents()))
+          RestoreFocus();
+      }
+
       BrowserList::SetLastActive(browser_.get());
-    else
+    } else {
       BrowserList::NotifyBrowserNoLongerActive(browser_.get());
+    }
   }
 
   if (!extension_keybinding_registry_ &&

chrome/browser/ui/views/frame/browser_view.h

@@ -13,6 +13,7 @@
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "build/build_config.h"
@@ -796,9 +797,11 @@ class BrowserView : public BrowserWindow,
 
   views::UnhandledKeyboardEventHandler unhandled_keyboard_event_handler_;
 
-  // If this flag is set then SetFocusToLocationBar() will set focus to the
-  // location bar even if the browser window is not active.
-  bool force_location_bar_focus_ = false;
+  // Whether OnWidgetActivationChanged should RestoreFocus. If this is set and
+  // is true, OnWidgetActivationChanged will call RestoreFocus. This is set
+  // to true when not set in Show() so that RestoreFocus on activation only
+  // happens for very first Show() calls.
+  base::Optional<bool> restore_focus_on_activation_;
 
   // This is non-null on Chrome OS only.
   std::unique_ptr<TopControlsSlideController> top_controls_slide_controller_;

chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc

@@ -17,6 +17,7 @@
 
 #if defined(OS_MACOSX)
 #include "chrome/browser/ui/browser_commands_mac.h"
+#include "chrome/test/base/interactive_test_utils.h"
 #endif
 
 using views::FocusManager;
@@ -28,12 +29,17 @@ class BrowserViewTest : public InProcessBrowserTest {
   BrowserViewTest() = default;
   ~BrowserViewTest() override = default;
 
-  void InitPrefSettings() {
+  void SetUpOnMainThread() override {
 #if defined(OS_MACOSX)
     // Set the preference to true so we expect to see the top view in
     // fullscreen mode.
     PrefService* prefs = browser()->profile()->GetPrefs();
     prefs->SetBoolean(prefs::kShowFullscreenToolbar, true);
+
+    // Ensure that the browser window is activated. BrowserView::Show calls
+    // into BridgedNativeWidgetImpl::SetVisibilityState and makeKeyAndOrderFront
+    // there somehow does not change the window's key status on bot.
+    ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
 #endif
   }
 
@@ -45,7 +51,6 @@ class BrowserViewTest : public InProcessBrowserTest {
 
 IN_PROC_BROWSER_TEST_F(BrowserViewTest, FullscreenClearsFocus) {
   BrowserView* browser_view = static_cast<BrowserView*>(browser()->window());
-  InitPrefSettings();
   LocationBarView* location_bar_view = browser_view->GetLocationBarView();
   FocusManager* focus_manager = browser_view->GetFocusManager();
 
@@ -64,7 +69,6 @@ IN_PROC_BROWSER_TEST_F(BrowserViewTest, FullscreenClearsFocus) {
 // correctly in browser fullscreen mode.
 IN_PROC_BROWSER_TEST_F(BrowserViewTest, BrowserFullscreenShowTopView) {
   BrowserView* browser_view = static_cast<BrowserView*>(browser()->window());
-  InitPrefSettings();
 
   // The top view should always show up in regular mode.
   EXPECT_FALSE(browser_view->IsFullscreen());

chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc

@@ -2592,11 +2592,15 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTestTouch,
   TabStrip* tab_strip = GetTabStripForBrowser(browser());
   EXPECT_EQ("0 1", IDString(browser()->tab_strip_model()));
 
-  // Move to the first tab and drag it enough so that it detaches.
+  // Move to the first tab and drag it enough so that it detaches. Drag it
+  // slightly more horizontally so that it does not generate a swipe down
+  // gesture that minimizes the detached browser window.
   gfx::Point tab_0_center(GetCenterInScreenCoordinates(tab_strip->tab_at(0)));
   ASSERT_TRUE(PressInput(tab_0_center));
+  const int touch_move_delta = GetDetachY(tab_strip);
   ASSERT_TRUE(DragInputToNotifyWhenDone(
-      tab_0_center.x(), tab_0_center.y() + GetDetachY(tab_strip),
+      tab_0_center.x() + touch_move_delta + 5,
+      tab_0_center.y() + touch_move_delta,
       base::Bind(&PressSecondFingerWhileDetachedStep2, this,
                  gfx::Point(tab_0_center.x(),
                             tab_0_center.y() + 2 * GetDetachY(tab_strip)))));

chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc

@@ -199,17 +199,18 @@ class ToolbarViewTest : public InProcessBrowserTest {
 void ToolbarViewTest::RunToolbarCycleFocusTest(Browser* browser) {
   gfx::NativeWindow window = browser->window()->GetNativeWindow();
   views::Widget* widget = views::Widget::GetWidgetForNativeWindow(window);
-  views::FocusManager* focus_manager = widget->GetFocusManager();
-  CommandUpdater* updater = browser->command_controller();
-
-  // Send focus to the toolbar as if the user pressed Alt+Shift+T.
-  updater->ExecuteCommand(IDC_FOCUS_TOOLBAR);
 
   // Test relies on browser window activation, while platform such as Linux's
   // window activation is asynchronous.
   views::test::WidgetActivationWaiter waiter(widget, true);
   waiter.Wait();
 
+  // Send focus to the toolbar as if the user pressed Alt+Shift+T. This should
+  // happen after the browser window activation.
+  CommandUpdater* updater = browser->command_controller();
+  updater->ExecuteCommand(IDC_FOCUS_TOOLBAR);
+
+  views::FocusManager* focus_manager = widget->GetFocusManager();
   views::View* first_view = focus_manager->GetFocusedView();
   std::vector<int> ids;
 

content/browser/web_contents/web_contents_impl.cc

@@ -4100,28 +4100,9 @@ void WebContentsImpl::ResumeLoadingCreatedWebContents() {
 }
 
 bool WebContentsImpl::FocusLocationBarByDefault() {
-  // When the browser is started with about:blank as the startup URL, focus
-  // the location bar (which will also select its contents) so people can
-  // simply begin typing to navigate elsewhere.
-  //
-  // We need to be careful not to trigger this for anything other than the
-  // startup navigation. In particular, if we allow an attacker to open a
-  // popup to about:blank, then navigate, focusing the Omnibox will cause the
-  // end of the new URL to be scrolled into view instead of the start,
-  // allowing the attacker to spoof other URLs. The conditions checked here
-  // are all aimed at ensuring no such attacker-controlled navigation can
-  // trigger this.
-  //
-  // Note that we check the pending entry instead of the visible one; for the
-  // startup URL case these are the same, but for the attacker-controlled
-  // navigation case the visible entry is the committed "about:blank" URL and
-  // the pending entry is the problematic navigation elsewhere.
-  NavigationEntryImpl* entry = controller_.GetPendingEntry();
-  if (controller_.IsInitialNavigation() && entry &&
-      !entry->is_renderer_initiated() &&
-      entry->GetURL() == url::kAboutBlankURL) {
+  if (should_focus_location_bar_by_default_)
     return true;
-  }
+
   return delegate_ && delegate_->ShouldFocusLocationBarByDefault(this);
 }
 
@@ -4138,6 +4119,24 @@ void WebContentsImpl::DidStartNavigation(NavigationHandle* navigation_handle) {
 
   if (display_cutout_host_impl_)
     display_cutout_host_impl_->DidStartNavigation(navigation_handle);
+
+  if (navigation_handle->IsInMainFrame()) {
+    // When the browser is started with about:blank as the startup URL, focus
+    // the location bar (which will also select its contents) so people can
+    // simply begin typing to navigate elsewhere.
+    //
+    // We need to be careful not to trigger this for anything other than the
+    // startup navigation. In particular, if we allow an attacker to open a
+    // popup to about:blank, then navigate, focusing the Omnibox will cause the
+    // end of the new URL to be scrolled into view instead of the start,
+    // allowing the attacker to spoof other URLs. The conditions checked here
+    // are all aimed at ensuring no such attacker-controlled navigation can
+    // trigger this.
+    should_focus_location_bar_by_default_ =
+        controller_.IsInitialNavigation() &&
+        !navigation_handle->IsRendererInitiated() &&
+        navigation_handle->GetURL() == url::kAboutBlankURL;
+  }
 }
 
 void WebContentsImpl::DidRedirectNavigation(
@@ -4233,6 +4232,13 @@ void WebContentsImpl::DidFinishNavigation(NavigationHandle* navigation_handle) {
                                  ukm::SourceIdType::NAVIGATION_ID);
     }
   }
+
+  // If we didn't end up on about:blank after setting this in DidStartNavigation
+  // then don't focus the location bar.
+  if (should_focus_location_bar_by_default_ &&
+      navigation_handle->GetURL() != url::kAboutBlankURL) {
+    should_focus_location_bar_by_default_ = false;
+  }
 }
 
 void WebContentsImpl::DidFailLoadWithError(
@@ -5614,6 +5620,10 @@ void WebContentsImpl::DidStartLoading(FrameTreeNode* frame_tree_node,
   LoadingStateChanged(frame_tree_node->IsMainFrame() && to_different_document,
                       false, nullptr);
 
+  // Reset the focus state from DidStartNavigation to false if a new load starts
+  // afterward, in case loading logic triggers a FocusLocationBarByDefault call.
+  should_focus_location_bar_by_default_ = false;
+
   // Notify accessibility that the user is navigating away from the
   // current document.
   //

content/browser/web_contents/web_contents_impl.h

@@ -1810,6 +1810,11 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
   // Store the frame that is currently fullscreen, nullptr if there is none.
   RenderFrameHostImpl* current_fullscreen_frame_ = nullptr;
 
+  // Whether location bar should be focused by default. This is computed in
+  // DidStartNavigation/DidFinishNavigation and only set for an initial
+  // navigation triggered by the browser going to about:blank.
+  bool should_focus_location_bar_by_default_ = false;
+
   base::WeakPtrFactory<WebContentsImpl> loading_weak_factory_;
   base::WeakPtrFactory<WebContentsImpl> weak_factory_;