Ensure we don't crash if user navigates back from NTP to Chrome sign-in page before it has fully lo…

Ensure we don't crash if user navigates back from NTP to Chrome sign-in page before it has fully loaded.

This CL aims to be simple, so it can be merged back to M29. It changes from DidStopLoading, which is later than needed point in time, to DidNavigateMainFrame. It is safe if we have committed a navigation to delete the previous navigation entry.

I've also added checks to ensure that we don't try to remove a pending navigation entry, as it leads to bugs, as evidenced here.

A follow up CL can completely remove the CurrentHistoryCleaner object, as all it does is store a single bit of state - the history entry index, and monitors for the same navigation events as OneClickSigninHelper.

BUG=257277
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213324

[Charlie Reis, 2013-07-18 21:53:38]

Drive-by comments.  :)  Glad you found a way to repro it!

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:30: 
nit: Remove extra line.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:168: class CommitWebContentsObserver
: public content::WebContentsObserver {
Maybe BackOnNTPCommitObserver?  CommitWebContentsObserver doesn't really say
what it does.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:181: {
nit: Extra braces not needed.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:197:
IN_PROC_BROWSER_TEST_F(SigninBrowserTest, SkipForNowAndGoBack) {
nit: Add a comment referencing the bug.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:216: // Since the navigation to the
blank URL causes a "redirect" to NTP, we expect
How do they cause this "redirect"?  Is it started immediately when the skip_url
commits?

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:219: EXPECT_EQ(ntp_url,
web_contents->GetActiveURL());
It's GetVisibleURL now, right?  :)

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:223: // before the DidStopLoading
message of NTP gets delivered.
Nice!  That sounds like the right approach to me, since DidStopLoading happens
asynchronously.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:224: CommitWebContentsObserver
commit_observer(web_contents);
Are we guaranteed that the NTP navigation can't commit before here, or do we
need to put this declaration above the NavigateToURL call on line 214?

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:226: {
nit: Braces aren't needed here.  They're mainly used in tests where we repeat
this block several times with the same variable names.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:232: EXPECT_EQ(ntp_url,
web_contents->GetActiveURL());
I don't understand this part.  It looks like the state we were in on line 218. 
Has the NTP committed?  Have we gone back?  Does that trigger another NTP load? 
Could other DidStopLoading calls have happened along the way?

[nasko, 2013-07-19 23:03:41]

Fixed the drive-bys and actually fixed the bug : ).

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:30: 
On 2013/07/18 21:53:38, creis wrote:
> nit: Remove extra line.

Done.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:168: class CommitWebContentsObserver
: public content::WebContentsObserver {
On 2013/07/18 21:53:38, creis wrote:
> Maybe BackOnNTPCommitObserver?  CommitWebContentsObserver doesn't really say
> what it does.

Done.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:181: {
On 2013/07/18 21:53:38, creis wrote:
> nit: Extra braces not needed.

Yeah, a few things are left over from the mess that this code used to be : )

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:197:
IN_PROC_BROWSER_TEST_F(SigninBrowserTest, SkipForNowAndGoBack) {
On 2013/07/18 21:53:38, creis wrote:
> nit: Add a comment referencing the bug.

Done.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:216: // Since the navigation to the
blank URL causes a "redirect" to NTP, we expect
On 2013/07/18 21:53:38, creis wrote:
> How do they cause this "redirect"?  Is it started immediately when the
skip_url
> commits?

Done.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:219: EXPECT_EQ(ntp_url,
web_contents->GetActiveURL());
On 2013/07/18 21:53:38, creis wrote:
> It's GetVisibleURL now, right?  :)

Sadly, not yet : (. But I put the comment intentionally like this, so I don't
miss it when this changes : ).

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:223: // before the DidStopLoading
message of NTP gets delivered.
On 2013/07/18 21:53:38, creis wrote:
> Nice!  That sounds like the right approach to me, since DidStopLoading happens
> asynchronously.

Done.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:224: CommitWebContentsObserver
commit_observer(web_contents);
On 2013/07/18 21:53:38, creis wrote:
> Are we guaranteed that the NTP navigation can't commit before here, or do we
> need to put this declaration above the NavigateToURL call on line 214?

I can't guarantee anything in complex systems ;) so I'm moving the observer
declaration before the navigation, to be on the safer side.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:226: {
On 2013/07/18 21:53:38, creis wrote:
> nit: Braces aren't needed here.  They're mainly used in tests where we repeat
> this block several times with the same variable names.

Done.

https://codereview.chromium.org/19699007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_browsertest.cc:232: EXPECT_EQ(ntp_url,
web_contents->GetActiveURL());
On 2013/07/18 21:53:38, creis wrote:
> I don't understand this part.  It looks like the state we were in on line 218.

> Has the NTP committed?  Have we gone back?  Does that trigger another NTP
load? 
> Could other DidStopLoading calls have happened along the way?

Since we navigated back during the NTP commit, we are back in the same state
once we have the navigation complete. On DidStopLoading of the blank page, the
helper object will initiate another load of the NTP, so we are back in the same
point.

[nasko, 2013-07-19 23:14:05]

Adding OWNERS

rogerta@ - chrome/browser/signin/ and chrome/browser/ui/sync/
mattm@ - chrome/browser/safe_browsing/

[Charlie Reis, 2013-07-19 23:28:43]

I like it, except that the end of the test makes me question my understanding of
the fix.

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
chrome/browser/signin/signin_browsertest.cc:193: // that occur of the user
clicks on the "Skip for now" link at the signin page
nit: of -> if

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
chrome/browser/signin/signin_browsertest.cc:229: observer.Wait();
This looks potentially flaky to me.  We could get LOAD_STOP before getting here
and then we'd wait forever.

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
chrome/browser/signin/signin_browsertest.cc:231: EXPECT_EQ(ntp_url,
web_contents->GetActiveURL());
Sorry, this still doesn't make sense to me.

In the new code, when we commit the NTP, we synchronously delete the blank
entry, right?  That means going back would put us back on the start_url, not
skip_url.

https://codereview.chromium.org/19699007/diff/7001/content/public/browser/nav...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/19699007/diff/7001/content/public/browser/nav...
content/public/browser/navigation_controller.h:333: // Removes the entry at the
specified |index|.  This call dicards any
nit: Might as well fix "discards".  Funny that the comment already said it
returned false.

[nasko, 2013-07-22 16:18:28]

Fixing nits and adding Roger to review from sign-in perspective.

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
chrome/browser/signin/signin_browsertest.cc:193: // that occur of the user
clicks on the "Skip for now" link at the signin page
On 2013/07/19 23:28:43, creis wrote:
> nit: of -> if

Done.

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
chrome/browser/signin/signin_browsertest.cc:229: observer.Wait();
On 2013/07/19 23:28:43, creis wrote:
> This looks potentially flaky to me.  We could get LOAD_STOP before getting
here
> and then we'd wait forever.

I don't think we can, see the next comment for more details.

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
chrome/browser/signin/signin_browsertest.cc:231: EXPECT_EQ(ntp_url,
web_contents->GetActiveURL());
On 2013/07/19 23:28:43, creis wrote:
> Sorry, this still doesn't make sense to me.
> 
> In the new code, when we commit the NTP, we synchronously delete the blank
> entry, right?  That means going back would put us back on the start_url, not
> skip_url.

Here is the timeline of events:

Click on "Skip for now", which goes to blank.html
Commit blank.html
DidStopLoading blank.html - this creates the history cleaner and does OpenURL
for NTP. At this point we have last committed of blank.html and visible of NTP.
Commit NTP - at this point the observer I've added initiates a back navigation
to blank.html. The pending navigation entry and visible URL is for blank.html,
while last committed is NTP. Due to the changes in nav controller, we will not
delete the history entry here, since it is the pending one.
DidStopLoading NTP - nothing really done here
Commit blank.html
DidStopLoading blank.html - this creates another history cleaner and is what the
LOAD_STOP observer sees. At this point we are back to the last committed is
blank.html and since the signin helper does OpenURL on NTP, we get it as the
visible URL.

I hope this clears things up. I can remove the extra URL checks after the
observer, but to me it is a way to ensure we understand what is going on and if
something changes, we can adjust the test to match it.

https://codereview.chromium.org/19699007/diff/7001/content/public/browser/nav...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/19699007/diff/7001/content/public/browser/nav...
content/public/browser/navigation_controller.h:333: // Removes the entry at the
specified |index|.  This call dicards any
On 2013/07/19 23:28:43, creis wrote:
> nit: Might as well fix "discards".  Funny that the comment already said it
> returned false.

Done.

[Roger Tawa OOO till Jul 10th, 2013-07-22 18:35:00]

lgtm for chrome\browser\ui\sync

A few questions about the sign in test.

https://codereview.chromium.org/19699007/diff/18001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/19699007/diff/18001/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:184: observer.Wait();
For my own info, why do we need wait here?

https://codereview.chromium.org/19699007/diff/18001/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:218:
ui_test_utils::NavigateToURL(browser(), skip_url);
This call does a wait for the load to stop.  Why do we need another wait at line
229?

https://codereview.chromium.org/19699007/diff/18001/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:231: EXPECT_EQ(ntp_url,
web_contents->GetActiveURL());
I'm not sure why these two tests are the same as lines 223-224.

[nasko, 2013-07-22 18:44:31]

https://codereview.chromium.org/19699007/diff/18001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/19699007/diff/18001/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:184: observer.Wait();
On 2013/07/22 18:35:00, Roger Tawa wrote:
> For my own info, why do we need wait here?

This is to ensure that we don't continue execution until the navigation back to
the blank.html page has committed. If we don't wait here, DidStopLoading for NTP
will likely get delivered and if that happens, we can't simulate the race
condition.

https://codereview.chromium.org/19699007/diff/18001/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:218:
ui_test_utils::NavigateToURL(browser(), skip_url);
On 2013/07/22 18:35:00, Roger Tawa wrote:
> This call does a wait for the load to stop.  Why do we need another wait at
line
> 229?

The extra wait is to ensure the test has enough time to execute the back
navigation, since it waits for the second redirect to NTP.

https://codereview.chromium.org/19699007/diff/18001/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:231: EXPECT_EQ(ntp_url,
web_contents->GetActiveURL());
On 2013/07/22 18:35:00, Roger Tawa wrote:
> I'm not sure why these two tests are the same as lines 223-224.

Did you have a chance to read my timeline explanation I did earlier when Charlie
asked why this is the case?

[nasko, 2013-07-22 18:53:34]

Including mattm@ for OWNERS review of chrome/browser/safe_browsing/.

[Roger Tawa OOO till Jul 10th, 2013-07-22 20:06:14]

lgtm for chrome/browser/signin/signin_browsertest.cc

Sorry, I did not see your timeline, my bad.  I get it now, thanks for the
explanation.

[Charlie Reis, 2013-07-22 23:18:07]

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
chrome/browser/signin/signin_browsertest.cc:231: EXPECT_EQ(ntp_url,
web_contents->GetActiveURL());
On 2013/07/22 16:18:28, nasko wrote:
> On 2013/07/19 23:28:43, creis wrote:
> > Sorry, this still doesn't make sense to me.
> > 
> > In the new code, when we commit the NTP, we synchronously delete the blank
> > entry, right?  That means going back would put us back on the start_url, not
> > skip_url.
> 
> Here is the timeline of events:
> 
> Click on "Skip for now", which goes to blank.html
> Commit blank.html
> DidStopLoading blank.html - this creates the history cleaner and does OpenURL
> for NTP. At this point we have last committed of blank.html and visible of
NTP.
> Commit NTP - at this point the observer I've added initiates a back navigation
> to blank.html. The pending navigation entry and visible URL is for blank.html,
> while last committed is NTP. Due to the changes in nav controller, we will not
> delete the history entry here, since it is the pending one.

How is that possible?  It doesn't seem like the test should be able to do a back
navigation to blank.html before the CurrentHistoryCleaner deletes it.  Are you
saying the BackOnNTPCommitObserver fires before the CurrentHistoryCleaner?

> DidStopLoading NTP - nothing really done here
> Commit blank.html
> DidStopLoading blank.html - this creates another history cleaner and is what
the
> LOAD_STOP observer sees.

I was hoping to prevent the creation of another history cleaner by forcing the
blank.html entry to be immediately deleted when the NTP commits.  Is that not
happening?


> At this point we are back to the last committed is
> blank.html and since the signin helper does OpenURL on NTP, we get it as the
> visible URL.
> 
> I hope this clears things up. I can remove the extra URL checks after the
> observer, but to me it is a way to ensure we understand what is going on and
if
> something changes, we can adjust the test to match it.

[nasko, 2013-07-23 20:34:47]

Fixed in a different way. Please take another look.

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/19699007/diff/7001/chrome/browser/signin/sign...
chrome/browser/signin/signin_browsertest.cc:231: EXPECT_EQ(ntp_url,
web_contents->GetActiveURL());
On 2013/07/22 23:18:07, creis wrote:
> On 2013/07/22 16:18:28, nasko wrote:
> > On 2013/07/19 23:28:43, creis wrote:
> > > Sorry, this still doesn't make sense to me.
> > > 
> > > In the new code, when we commit the NTP, we synchronously delete the blank
> > > entry, right?  That means going back would put us back on the start_url,
not
> > > skip_url.
> > 
> > Here is the timeline of events:
> > 
> > Click on "Skip for now", which goes to blank.html
> > Commit blank.html
> > DidStopLoading blank.html - this creates the history cleaner and does
OpenURL
> > for NTP. At this point we have last committed of blank.html and visible of
> NTP.
> > Commit NTP - at this point the observer I've added initiates a back
navigation
> > to blank.html. The pending navigation entry and visible URL is for
blank.html,
> > while last committed is NTP. Due to the changes in nav controller, we will
not
> > delete the history entry here, since it is the pending one.
> 
> How is that possible?  It doesn't seem like the test should be able to do a
back
> navigation to blank.html before the CurrentHistoryCleaner deletes it.  Are you
> saying the BackOnNTPCommitObserver fires before the CurrentHistoryCleaner?
> 
> > DidStopLoading NTP - nothing really done here
> > Commit blank.html
> > DidStopLoading blank.html - this creates another history cleaner and is what
> the
> > LOAD_STOP observer sees.
> 
> I was hoping to prevent the creation of another history cleaner by forcing the
> blank.html entry to be immediately deleted when the NTP commits.  Is that not
> happening?
> 
> 
> > At this point we are back to the last committed is
> > blank.html and since the signin helper does OpenURL on NTP, we get it as the
> > visible URL.
> > 
> > I hope this clears things up. I can remove the extra URL checks after the
> > observer, but to me it is a way to ensure we understand what is going on and
> if
> > something changes, we can adjust the test to match it.
> 

I've modified the patch to use DidCommitProvisionalLoadForFrame in order to
ensure we don't create multiple history cleaners. If we do it in
DidNavigateMainFrame, there is a chance for someone observing the commit to
navigate back, just like my test was doing.

[mattm, 2013-07-23 21:12:45]

lgtm

[Charlie Reis, 2013-07-23 22:39:32]

LGTM, as long as the GetLastCommittedURL changes won't cause an issue with
merging.

https://codereview.chromium.org/19699007/diff/26001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/19699007/diff/26001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:520: delete this;  // Success.
nit: Wrong indent.  (Remove two spaces.)

https://codereview.chromium.org/19699007/diff/26001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:927:
web_contents->GetLastCommittedURL())) {
Good thinking.  One thought, though-- will you be able to merge this?  Fine if
so.

[nasko, 2013-07-24 00:01:40]

Fixed the nit. We are safe for merging, the method exists in M29.

https://codereview.chromium.org/19699007/diff/26001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/19699007/diff/26001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:520: delete this;  // Success.
On 2013/07/23 22:39:32, creis wrote:
> nit: Wrong indent.  (Remove two spaces.)

Done.

https://codereview.chromium.org/19699007/diff/26001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:927:
web_contents->GetLastCommittedURL())) {
On 2013/07/23 22:39:32, creis wrote:
> Good thinking.  One thought, though-- will you be able to merge this?  Fine if
> so.

Yes, the CL that added GetLastCommittedURL is 201317 and according to
omahaproxy, win beta is 208345. We should be safe.

[commit-bot: I haz the power, 2013-07-24 00:07:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/19699007/33001

[commit-bot: I haz the power, 2013-07-24 02:49:37]

Change committed as 213324