Fix Blink commit type for subframes after initial about:blank load.

Fix Blink commit type for subframes after initial about:blank load.

This ensures that we do not create a new history item for whatever
page replaces the initial about:blank page in an iframe.

BUG=464014, 310347, 178380
TEST=Click a link in an initially about:blank iframe. No history item created.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196964

[Charlie Reis, 2015-06-09 07:11:20]

creis@chromium.org changed reviewers:
+ japhet@chromium.org

[Charlie Reis, 2015-06-09 07:14:19]

Nate, can I get your advice on this?  It seems a bit awkward to me, but I think
I can justify each of the parts.

This gets the commit type to be correct for OOPIFs that replace the initial
about:blank page, but it also fixes a long-standing bug I discovered:
same-process link clicks in the initial about:blank page of an iframe are
creating a new entry when they shouldn't.  (That's because link clicks don't use
NavigationScheduler, while other things like buttons do.)

I'll file a separate bug for that, just to have a record of it.

https://codereview.chromium.org/1173513002/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames.html
(left):

https://codereview.chromium.org/1173513002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames.html:12:
<iframe name="frame1" src="about:blank"></iframe>
This test (which ironically I wrote in 2011) was broken, since it expected to
navigate from about:blank to a real page and back.  The real page should replace
the initial about:blank page in history, so it's not possible to go back.

https://codereview.chromium.org/1173513002/diff/40001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1173513002/diff/40001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:678: // From the HTML5 spec for
location.assign():
I'm moving this rule from NavigationScheduler::mustLockBackForwardList and not
the rest, since the other parts don't work at this point in the timeline.  (They
depend on whether a load is in progress, which is always true here for this
request, regardless of what the state was at the start of the request.)

https://codereview.chromium.org/1173513002/diff/40001/Source/core/loader/Navi...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1173513002/diff/40001/Source/core/loader/Navi...
Source/core/loader/NavigationScheduler.cpp:299: // if
(!targetFrame->loader().stateMachine()->committedMultipleRealLoads()
I'll remove this if you think this approach is sane.

https://codereview.chromium.org/1173513002/diff/40001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1173513002/diff/40001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1953:
frameLoader.stateMachine()->advanceTo(FrameLoaderStateMachine::CommittedMultipleRealLoads);
After looking at how Blink uses these states, I think we might want to always
use CommittedMultipleRealLoads here.

Confusingly, Blink uses CommittedFirstRealLoad for the initial about:blank
document in an iframe.  The check that I've moved from NavigationScheduler
(which dictates whether the next commit creates a new history item or not) uses
CommittedMultipleRealLoads, which is the state you're in after leaving
about:blank, even if you only have 1 item in history (i.e., the real page that
replaced about:blank).

[Nate Chapin, 2015-06-09 18:25:33]

This seems reasonable in general.

https://codereview.chromium.org/1173513002/diff/40001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1173513002/diff/40001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:682: if
(!m_stateMachine.committedMultipleRealLoads()
Should this be a case in determineFrameLoadType()?

https://codereview.chromium.org/1173513002/diff/40001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1173513002/diff/40001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1953:
frameLoader.stateMachine()->advanceTo(FrameLoaderStateMachine::CommittedMultipleRealLoads);
On 2015/06/09 07:14:18, Charlie Reis wrote:
> After looking at how Blink uses these states, I think we might want to always
> use CommittedMultipleRealLoads here.
> 
> Confusingly, Blink uses CommittedFirstRealLoad for the initial about:blank
> document in an iframe.  The check that I've moved from NavigationScheduler
> (which dictates whether the next commit creates a new history item or not)
uses
> CommittedMultipleRealLoads, which is the state you're in after leaving
> about:blank, even if you only have 1 item in history (i.e., the real page that
> replaced about:blank).

CommittedMultipleRealLoads isn't used for a whole lot, if you have a better way
to do the location.assign() special case, I'm happy to remove it.

[Charlie Reis, 2015-06-11 00:25:34]

Patchset #4 (id:60001) has been deleted

[Charlie Reis, 2015-06-11 16:43:49]

[+nasko]

Ok!  I've done some more digging and found how this relates to (and replaces) an
earlier fix from Nasko.  I've updated his old test to cover it and hopefully be
less flaky, and it might help with the user comments on https://crbug.com/178380
as well.

PTAL!

https://codereview.chromium.org/1173513002/diff/40001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1173513002/diff/40001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:682: if
(!m_stateMachine.committedMultipleRealLoads()
On 2015/06/09 18:25:33, Nate Chapin wrote:
> Should this be a case in determineFrameLoadType()?

Yes, that seems reasonable.  Done.

https://codereview.chromium.org/1173513002/diff/40001/Source/core/loader/Navi...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1173513002/diff/40001/Source/core/loader/Navi...
Source/core/loader/NavigationScheduler.cpp:299: // if
(!targetFrame->loader().stateMachine()->committedMultipleRealLoads()
On 2015/06/09 07:14:18, Charlie Reis wrote:
> I'll remove this if you think this approach is sane.

Done.

https://codereview.chromium.org/1173513002/diff/40001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1173513002/diff/40001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1953:
frameLoader.stateMachine()->advanceTo(FrameLoaderStateMachine::CommittedMultipleRealLoads);
On 2015/06/09 18:25:33, Nate Chapin wrote:
> On 2015/06/09 07:14:18, Charlie Reis wrote:
> > After looking at how Blink uses these states, I think we might want to
always
> > use CommittedMultipleRealLoads here.
> > 
> > Confusingly, Blink uses CommittedFirstRealLoad for the initial about:blank
> > document in an iframe.  The check that I've moved from NavigationScheduler
> > (which dictates whether the next commit creates a new history item or not)
> uses
> > CommittedMultipleRealLoads, which is the state you're in after leaving
> > about:blank, even if you only have 1 item in history (i.e., the real page
that
> > replaced about:blank).
> 
> CommittedMultipleRealLoads isn't used for a whole lot, if you have a better
way
> to do the location.assign() special case, I'm happy to remove it.

Oh, I see; it was introduced in https://codereview.chromium.org/76653002/ in
November 2013.

The part that I found confusing was that the about:blank page counted as
"CommittedFirstRealLoad" (as opposed to something like
DisplayingInitialEmptyDocumentPostCommit, which used to exist but was removed in
https://codereview.chromium.org/22891017 in August 2013).  From the name, I
would have expected the first non-about:blank page to count as the
"CommittedFirstRealLoad."

I'm not terribly concerned about fixing that here, though, now that we seem to
have come to a mostly sane situation.  Ok if we fix this and later consider
improvements to CommittedMultipleRealLoads?

https://codereview.chromium.org/1173513002/diff/100001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/1173513002/diff/100001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:1214: // The current load should replace the
history item if it is the first real
This was Nasko's original fix from https://codereview.chromium.org/25269006/ in
October 2013.

It probably worked at the time, but Nate's
https://codereview.chromium.org/76653002 landed in November 2013 and did mostly
the same thing in NavigationScheduler::mustLockBackForwardList.  As far as I can
tell, that mostly made this check not helpful, since we needed to start checking
CommittedMultipleRealLoads instead.

In this CL, I've moved the NavigationScheduler check to FrameLoader so that it
also applies to navigations that don't use NavigationScheduler, and we don't
seem to need this old fix anymore.  (The tests are happy without it.)

https://codereview.chromium.org/1173513002/diff/100001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (left):

https://codereview.chromium.org/1173513002/diff/100001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:5997: TEST_P(ParameterizedWebFrameTest,
DISABLED_FirstFrameNavigationReplacesHistory)
This was Nasko's test from https://codereview.chromium.org/25269006/,
accompanying the fix I mentioned earlier.  Unfortunately, it was disabled 20
days later in https://crbug.com/310347 for being flaky (without much
notification to any of us or a record of how it was failing).

I think I've fixed the flakiness by using loadFrame on the subframe instead of
navigating to a javascript URL.  I've also split it into two tests just in case
the javascript URL in the second test is still flaky.

It now covers the cases for this bug, and I have additional test coverage for
the subframe commit lined up in the Chromium side as well.

[Nate Chapin, 2015-06-11 16:56:41]

lgtm

[Charlie Reis, 2015-06-11 17:55:32]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-06-11 17:56:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1173513002/100001

[commit-bot: I haz the power, 2015-06-11 17:59:38]

Committed patchset #5 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196964

[dglazkov, 2015-06-11 20:15:39]

On 2015/06/11 at 17:59:38, commit-bot wrote:
> Committed patchset #5 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196964

This seems to have broken
https://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Te...

SitePerProcessBrowserTest.NavigateRemoteFrame
SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

[dglazkov, 2015-06-11 20:18:42]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/1177313002/ by dglazkov@chromium.org.

The reason for reverting is: Broke SitePerProcessBrowserTest.NavigateRemoteFrame
and SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs..