Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case with PlzNavigate enabled. Reason being we don't find a frame to navigate to. Thanks to clamy for pointing out
that the sequence numbers setting code in the frame navigation code caused that.

Fix is to add the item sequence number and document sequence number to the page state. Added a helper
function to the TestRenderFrameHost class called CreatePageStateForURL
which provides this functionality.

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7cf627a6bd6704018b6f2091c176d4762f080797
Cr-Commit-Position: refs/heads/master@{#417165}

[ananta, 2016-09-03 00:49:55]

Description was changed from

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case. With PlzNavigate enabled
the navigation is initiated in the browser. However the
NavigationControllerImpl::NavigateToPendingEntryInternal()
function is unable to find a frame to navigate and hence ends up navigating the
root. This
causes the test to crash as it is trying to deref a null navigation_request
pointer in
TestRenderFrameHost::PrepareForCommit().

Fix is to use the PrepareForCommitIfNecessary() function.

BUG=510836
==========

to

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case. With PlzNavigate enabled
the navigation is initiated in the browser. However the
NavigationControllerImpl::NavigateToPendingEntryInternal()
function is unable to find a frame to navigate and hence ends up navigating the
root. This
causes the test to crash as it is trying to deref a null navigation_request
pointer in
TestRenderFrameHost::PrepareForCommit().

Fix is to use the PrepareForCommitIfNecessary() function.

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-09-03 00:50:50]

ananta@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org

[ananta, 2016-09-03 00:51:01]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-03 00:51:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 02:23:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-03 02:23:03]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-09-06 12:32:08]

Thanks! As explained in the comment, I don't think the fix is the right one for
the unit tests.

@nasko: it seems this test started falling after your CL for PageState in
FrameNavigationEntry landed.

https://codereview.chromium.org/2309583002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2309583002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2572:
subframe->PrepareForCommitIfNecessary();
This test used to work with PlzNavigate enabled. We should create a
NavigationRequest when asked to go back so this should not be an issue. Instead,
we should investigate why we don't actually start a navigation in the subframe.

Considering when it started failing, I suspect this is due to
https://codereview.chromium.org/2294343002 landing. In particular, this CL
changes the way in which the item & document sequence number are set up in the
FrameNavigationEntry. I think this is why we no longer set them up properly in
the unit tests, and because of that the history code interpretes the go-back
command as being a same-document navigation instead of a cross-document one.

[ananta, 2016-09-07 02:43:37]

https://codereview.chromium.org/2309583002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2309583002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2572:
subframe->PrepareForCommitIfNecessary();
On 2016/09/06 12:32:08, clamy wrote:
> This test used to work with PlzNavigate enabled. We should create a
> NavigationRequest when asked to go back so this should not be an issue.
Instead,
> we should investigate why we don't actually start a navigation in the
subframe.
> 
> Considering when it started failing, I suspect this is due to
> https://codereview.chromium.org/2294343002 landing. In particular, this CL
> changes the way in which the item & document sequence number are set up in the
> FrameNavigationEntry. I think this is why we no longer set them up properly in
> the unit tests, and because of that the history code interpretes the go-back
> command as being a same-document navigation instead of a cross-document one.

Thanks for the tip. It does fail because of the change in the way the sequence
numbers are setup in the navigation entry. I added code in
FrameNavigationEntry::SetPageState to honor the existing values in the sequence
numbers if the exploded ones don't contain values greater than 0. That fixes the
problem.

For context in chrome, the page state is updated recursively using the Web
navigation state from the renderer and is sent to the browser via the
FrameHostMsg_UpdateState IPC message

Please check if this approach is reasonable for tests.

[ananta, 2016-09-07 02:46:52]

Description was changed from

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case. With PlzNavigate enabled
the navigation is initiated in the browser. However the
NavigationControllerImpl::NavigateToPendingEntryInternal()
function is unable to find a frame to navigate and hence ends up navigating the
root. This
causes the test to crash as it is trying to deref a null navigation_request
pointer in
TestRenderFrameHost::PrepareForCommit().

Fix is to use the PrepareForCommitIfNecessary() function.

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case with PlzNavigate enabled.
Reason being we don't find a frame to navigate to. Thanks to clamy for pointing
out
that the sequence numbers setting code in the frame navigation code caused that.

Proposed fix is to honor existing sequence number values
in the frame navigation class if the page state does not
contain valid values for them. Please note that this
problem does not happen in chrome, because the page state
is sent with all this information from the renderer.


BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-09-07 02:47:32]

Description was changed from

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case with PlzNavigate enabled.
Reason being we don't find a frame to navigate to. Thanks to clamy for pointing
out
that the sequence numbers setting code in the frame navigation code caused that.

Proposed fix is to honor existing sequence number values
in the frame navigation class if the page state does not
contain valid values for them. Please note that this
problem does not happen in chrome, because the page state
is sent with all this information from the renderer.


BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case with PlzNavigate enabled.
Reason being we don't find a frame to navigate to. Thanks to clamy for pointing
out
that the sequence numbers setting code in the frame navigation code caused that.

Proposed fix is to honor existing sequence number values in the frame navigation
class if the page state does not
contain valid values for them. Please note that this problem does not happen in
chrome, because the page state
is sent with all this information from the renderer.

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-09-07 02:49:22]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 02:49:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-09-07 02:53:50]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 02:54:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 03:43:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 03:43:58]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-09-07 14:22:41]

@nasko: could you take a look at this one since it modifies code introduced by
your patch? Also, I noticed that there is still a parameter for sequence id in
DidCommitProvisionalLoad. is it still useful since we're essentially going to
overwrite it when setting the PageState in the FrameNavigationEntry?

[nasko, 2016-09-07 20:56:02]

https://codereview.chromium.org/2309583002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_navigation_entry.cc (right):

https://codereview.chromium.org/2309583002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.cc:96: if
(exploded_state.top.item_sequence_number > 0 ||
I don't think this is the correct approach. The whole reason for this method is
to avoid mismatches between item_sequence_number_ and
exploded_state.top.item_sequence_number, where this CL is adding ways for this
to happen. I would prefer we fix the unit testing code to behave closer to
reality than to add the code here.

[nasko, 2016-09-07 20:57:07]

On 2016/09/07 14:22:41, clamy wrote:
> @nasko: could you take a look at this one since it modifies code introduced by
> your patch? Also, I noticed that there is still a parameter for sequence id in
> DidCommitProvisionalLoad. is it still useful since we're essentially going to
> overwrite it when setting the PageState in the FrameNavigationEntry?

Which version of DidCommitProvisionalLoad is that?

[ananta, 2016-09-07 22:48:36]

Description was changed from

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case with PlzNavigate enabled.
Reason being we don't find a frame to navigate to. Thanks to clamy for pointing
out
that the sequence numbers setting code in the frame navigation code caused that.

Proposed fix is to honor existing sequence number values in the frame navigation
class if the page state does not
contain valid values for them. Please note that this problem does not happen in
chrome, because the page state
is sent with all this information from the renderer.

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case with PlzNavigate enabled.
Reason being we don't find a frame to navigate to. Thanks to clamy for pointing
out
that the sequence numbers setting code in the frame navigation code caused that.

Fix is to add the item sequence number and document sequence number to the page
state. Added a helper
function to the TestRenderFrameHost class called CreatePageStateForURL
which provides this functionality.

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-09-07 22:50:07]

https://codereview.chromium.org/2309583002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_navigation_entry.cc (right):

https://codereview.chromium.org/2309583002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.cc:96: if
(exploded_state.top.item_sequence_number > 0 ||
On 2016/09/07 20:56:02, nasko (slow) wrote:
> I don't think this is the correct approach. The whole reason for this method
is
> to avoid mismatches between item_sequence_number_ and
> exploded_state.top.item_sequence_number, where this CL is adding ways for this
> to happen. I would prefer we fix the unit testing code to behave closer to
> reality than to add the code here.

Added the functionality to the test code. PTAL

[ananta, 2016-09-07 22:50:18]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 22:51:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2016-09-07 23:21:59]

Thanks, this is much better direction!

https://codereview.chromium.org/2309583002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2309583002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2506: url2,
item_sequence_number2, 0);
The document sequence number should be different than the previous navigation,
as it is a different document.

https://codereview.chromium.org/2309583002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2539: url3,
params.item_sequence_number, 0);
Same here. Document number cannot be the same.

https://codereview.chromium.org/2309583002/diff/60001/content/test/test_rende...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/2309583002/diff/60001/content/test/test_rende...
content/test/test_render_frame_host.cc:483: return
PageState::CreateFromEncodedData(encoded_page_state);
This code looks fine, but why can't this remain a method on PageState?
PageState::CreateForTesting looks much more targeted and clear.

[ananta, 2016-09-07 23:58:44]

https://codereview.chromium.org/2309583002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2309583002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2506: url2,
item_sequence_number2, 0);
On 2016/09/07 23:21:59, nasko (slow) wrote:
> The document sequence number should be different than the previous navigation,
> as it is a different document. 

Done.

https://codereview.chromium.org/2309583002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2539: url3,
params.item_sequence_number, 0);
On 2016/09/07 23:21:59, nasko (slow) wrote:
> Same here. Document number cannot be the same.

Done.

https://codereview.chromium.org/2309583002/diff/60001/content/test/test_rende...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/2309583002/diff/60001/content/test/test_rende...
content/test/test_render_frame_host.cc:483: return
PageState::CreateFromEncodedData(encoded_page_state);
On 2016/09/07 23:21:59, nasko (slow) wrote:
> This code looks fine, but why can't this remain a method on PageState?
> PageState::CreateForTesting looks much more targeted and clear.

Added a new method CreateForTestingWithSequenceNumbers to the PageState class.

[ananta, 2016-09-07 23:58:56]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 23:59:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 00:02:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 00:02:36]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[nasko, 2016-09-08 00:19:00]

LGTM with a couple of nits addressed. Thanks!

https://codereview.chromium.org/2309583002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2309583002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2463: int64_t
document_sequence_number1 = base::Time::Now().ToDoubleT() * 1000000;
Let's use the same strategy that Blink uses for those. It starts off with a
number based on time, but then increments it from there on. It guarantees that
we won't get two sequence numbers that match.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Hi...

https://codereview.chromium.org/2309583002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2541:
params.item_sequence_number = item_sequence_number3;
Let's be consistent and set ISN and DSN for each params struct in all of the
cases. This will bring the code closer to how it behaves in reality.

[ananta, 2016-09-08 00:41:39]

https://codereview.chromium.org/2309583002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2309583002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2463: int64_t
document_sequence_number1 = base::Time::Now().ToDoubleT() * 1000000;
On 2016/09/08 00:18:59, nasko (slow) wrote:
> Let's use the same strategy that Blink uses for those. It starts off with a
> number based on time, but then increments it from there on. It guarantees that
> we won't get two sequence numbers that match.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Hi...

Thanks. Added a function GenerateSequenceNumbers() which mimics that.

https://codereview.chromium.org/2309583002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2541:
params.item_sequence_number = item_sequence_number3;
On 2016/09/08 00:18:59, nasko (slow) wrote:
> Let's be consistent and set ISN and DSN for each params struct in all of the
> cases. This will bring the code closer to how it behaves in reality.

Done.

[ananta, 2016-09-08 00:48:22]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 00:48:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 02:06:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 02:06:50]

Dry run: This issue passed the CQ dry run.

[ananta, 2016-09-08 02:12:56]

The CQ bit was checked by ananta@chromium.org

[ananta, 2016-09-08 02:12:56]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2309583002/#ps140001
(title: "Set document_sequence_number correctly and use the same strategy like
Blink to generate these numbe…")

[commit-bot: I haz the power, 2016-09-08 02:13:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 02:17:10]

Description was changed from

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case with PlzNavigate enabled.
Reason being we don't find a frame to navigate to. Thanks to clamy for pointing
out
that the sequence numbers setting code in the frame navigation code caused that.

Fix is to add the item sequence number and document sequence number to the page
state. Added a helper
function to the TestRenderFrameHost class called CreatePageStateForURL
which provides this functionality.

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case with PlzNavigate enabled.
Reason being we don't find a frame to navigate to. Thanks to clamy for pointing
out
that the sequence numbers setting code in the frame navigation code caused that.

Fix is to add the item sequence number and document sequence number to the page
state. Added a helper
function to the TestRenderFrameHost class called CreatePageStateForURL
which provides this functionality.

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-08 02:17:12]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-08 02:19:27]

Description was changed from

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case with PlzNavigate enabled.
Reason being we don't find a frame to navigate to. Thanks to clamy for pointing
out
that the sequence numbers setting code in the frame navigation code caused that.

Fix is to add the item sequence number and document sequence number to the page
state. Added a helper
function to the TestRenderFrameHost class called CreatePageStateForURL
which provides this functionality.

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

The test was crashing in the controller.GoBack() case with PlzNavigate enabled.
Reason being we don't find a frame to navigate to. Thanks to clamy for pointing
out
that the sequence numbers setting code in the frame navigation code caused that.

Fix is to add the item sequence number and document sequence number to the page
state. Added a helper
function to the TestRenderFrameHost class called CreatePageStateForURL
which provides this functionality.

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7cf627a6bd6704018b6f2091c176d4762f080797
Cr-Commit-Position: refs/heads/master@{#417165}
==========

[commit-bot: I haz the power, 2016-09-08 02:19:28]

Patchset 8 (id:??) landed as
https://crrev.com/7cf627a6bd6704018b6f2091c176d4762f080797
Cr-Commit-Position: refs/heads/master@{#417165}