Tech Debt Repayment for StartupBrowserCreatorImpl Refactor

chrome/browser/ui/startup/startup_browser_creator_impl.cc

Index: chrome/browser/ui/startup/startup_browser_creator_impl.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator_impl.cc b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
index 5c8ae2881898f1a981237b2dc8184a6e5eb154b4..704e5f276bc898f75f88b6cb3728f5d0c1cf822c 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
@@ -750,7 +750,11 @@ Browser* StartupBrowserCreatorImpl::RestoreOrCreateBrowser(
       &StartupBrowserCreator::in_synchronous_profile_launch_, true);
 
   // OpenTabsInBrowser requires at least one tab be passed. As a fallback to
-  // prevent a crash, use the NTP if |tabs| is empty.
+  // prevent a crash, use the NTP if |tabs| is empty. This shouldn't happen,
+  // but could indicate an inconsistency where, e.g., we expected a session
+  // restore to happen but it did not occur/succeed. This has been observed when
+  // launching Chrome with the kRestoreLastSession switch, using a profile that
+  // does not normally restore, when a Chrome process is already running.

[Peter Kasting, 2016/11/02 17:54:51]

I'm concerned with "this shouldn't happen" (which to me reads like "DCHECK"/code
bug) in conjunction with "has been observed".

It seems like either the current behavior is a weird edge case but correct, in
which case we shouldn't say things like "fallback" and "shouldn't happen" and
should just handle this, or else this is a real bug (maybe in code elsewhere?),
in which case we should fix the bug so we don't get here with no tabs.

I would imagine that stepping through the callstack in the case you describe
might illuminate this.

[tmartino, 2016/11/02 18:04:40]

Hmm, I can certainly take a look at stepping through the callstack in more
detail and report back.

What sticks out to me here is that the documentation for the switch in question
only specifies behavior "at startup":

// Indicates the last session should be restored on startup. This overrides the
// preferences value and is primarily intended for testing. The value of this
// switch is the number of tabs to wait until loaded before 'load completed' is
// sent to the ui_test.

So we're sort of venturing into undefined behavior territory here--with the
caveat that even if the behavior is only defined at startup, someone absolutely
can still pass the switch later.

[Peter Kasting, 2016/11/02 18:05:47]

Right, I'm wondering if the fix is "detect that the switch is being passed at
not-startup and ignore it".  Dunno where that code would live.

[tmartino, 2017/02/03 23:44:11]

Spent about a day on this and wanted to close the loop. Here's what I verified:

- GetSessionStartupPref doesn't know whether or not this is process startup, so
it allows the switch to override when it should not. This seems like the
appropriate place to make such a change, but unfortunately it would be difficult
to add a |process_startup| bool--this is a static method, and there are a number
of callsites outside these files that wouldn't have visibility into whether this
is startup.

- Because of this, the New Tab Page is not added in GetNewTabPageTabs.

- We expect MaybeAsyncRestore to perform a restore and leave this codepath
entirely before the line in question, but none occurs.

For the time being, I don't want to make any changes. I tried several approaches
to resolve this particular inconsistency; the only place that could actually
have the needed visibility would be  GetNewTabPageTabs. I just don't think the
added complexity of special-casing that method is proportional to the problem it
would be solving. More generally, I think the possibility of inconsistency
between restore expectations and outcomes more broadly is one we should probably
treat as possible, and not particularly high-risk.

   browser = OpenTabsInBrowser(
       browser, process_startup,
       (tabs.empty()