PlzNavigate: Fix dynamic iframe back forward layout test failure

PlzNavigate: Fix dynamic iframe back forward layout test failure

This test creates an iframe and changes the src of the iframe to different URLs based on the
session storage property. Additionally it also moves back in history in the process. We get renderer initiated
navigations initially followed by a browser navigation for the history and then a subsequent navigation for the child
frame which is treated as a history navigation in Blink as the history navigation initated above is still incomplete.

For PlzNavigate the history navigation initiated for the frame waits for the unload handler on the iframe to complete.
This is a browser navigation. Before the unload ack comes in we receive a navigation IPC which is ignored as the pending
navigation is a browser navigation and the new navigation does not have user_gesture.

The attempted fix is to treat child frame history navigations as renderer navigations if they are not initiated by a user
action.

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

[ananta, 2017-02-14 02:57:02]

Description was changed from

==========
PlzNavigate: Fix dynamic iframe back forward layout test failure

This test creates an iframe and changes the src of the iframe to different URLs
based on the
session storage property. Additionally it also moves back in history in the
process. We get renderer initiated
navigations initially followed by a browser navigation for the history and then
a subsequent navigation for the child
frame which is treated as a history navigation in Blink as the history
navigation initated above is still incomplete.

For PlzNavigate the history navigation initiated for the frame waits for the
unload handler on the iframe to complete.
This is a browser navigation. Before the unload ack comes in we receive a
navigation IPC which is ignored as the pending
navigation is a browser navigation and the new navigation does not have
user_gesture.

The attempted fix is to treat child frame history navigations as renderer
navigations if they are not initiated by a user
action.

BUG=691168
==========

to

==========
PlzNavigate: Fix dynamic iframe back forward layout test failure

This test creates an iframe and changes the src of the iframe to different URLs
based on the
session storage property. Additionally it also moves back in history in the
process. We get renderer initiated
navigations initially followed by a browser navigation for the history and then
a subsequent navigation for the child
frame which is treated as a history navigation in Blink as the history
navigation initated above is still incomplete.

For PlzNavigate the history navigation initiated for the frame waits for the
unload handler on the iframe to complete.
This is a browser navigation. Before the unload ack comes in we receive a
navigation IPC which is ignored as the pending
navigation is a browser navigation and the new navigation does not have
user_gesture.

The attempted fix is to treat child frame history navigations as renderer
navigations if they are not initiated by a user
action.

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

[ananta, 2017-02-14 03:03:50]

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

[ananta, 2017-02-14 03:04:02]

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

[commit-bot: I haz the power, 2017-02-14 03:04:43]

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, 2017-02-14 03:17:05]

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

[commit-bot: I haz the power, 2017-02-14 03:17:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2017-02-14 17:52:04]

I defer to Camille or Nasko on this

[ananta, 2017-02-14 21:30:27]

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

[commit-bot: I haz the power, 2017-02-14 21:31:21]

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

[ananta, 2017-02-14 23:01:47]

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

[ananta, 2017-02-14 23:01:47]

+creis

[commit-bot: I haz the power, 2017-02-15 00:36:50]

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

[commit-bot: I haz the power, 2017-02-15 00:36:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2017-02-15 06:04:32]

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

[commit-bot: I haz the power, 2017-02-15 06:05:29]

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

[ananta, 2017-02-15 06:39:28]

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

[commit-bot: I haz the power, 2017-02-15 06:40:02]

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, 2017-02-15 09:46:51]

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

[commit-bot: I haz the power, 2017-02-15 09:46:52]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[clamy, 2017-02-15 16:27:55]

Thanks! A few questions and comments below.

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:211: bool browser_initiated =
!entry.is_renderer_initiated();
Here we set whether the navigation was browser initiated or not. If I understand
the issue correctly, shouldn't we modify this so that it accurately reflects
that the history navigation was in fact renderer-initiated?

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:103:
CreateRendererInitiatedHistoryNavigation(
I don't think creating a new way to create a NavigationRequest is warranted. I'd
rather we identify which parameter is badly initialized with the existing one
and ensure it is set correctly.

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:981: // A new navigation may have
occurred.
This seems wrong. If we started a new browser-initiated NavigationRequest, it
should have committed immediately or be waiting for the BeforeUnload ACK.
Similarly, a renderer-initiated navigation could not have started while we were
waiting for the BeforeUnload ACK. In what case are you hitting this issue?

[ananta, 2017-02-15 23:45:03]

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:211: bool browser_initiated =
!entry.is_renderer_initiated();
On 2017/02/15 16:27:55, clamy wrote:
> Here we set whether the navigation was browser initiated or not. If I
understand
> the issue correctly, shouldn't we modify this so that it accurately reflects
> that the history navigation was in fact renderer-initiated?

Thanks. Added code to set the flag for child frame navigations coming from the
renderer. Will look into doing this for all history navigations from the
renderer in a later patchset.

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:103:
CreateRendererInitiatedHistoryNavigation(
On 2017/02/15 16:27:55, clamy wrote:
> I don't think creating a new way to create a NavigationRequest is warranted.
I'd
> rather we identify which parameter is badly initialized with the existing one
> and ensure it is set correctly.

Done. I added a is_user_action parameter to this function which is passed from
the renderer.

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2697753002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:981: // A new navigation may have
occurred.
On 2017/02/15 16:27:55, clamy wrote:
> This seems wrong. If we started a new browser-initiated NavigationRequest, it
> should have committed immediately or be waiting for the BeforeUnload ACK.
> Similarly, a renderer-initiated navigation could not have started while we
were
> waiting for the BeforeUnload ACK. In what case are you hitting this issue?

The scenario is the following:-
1. A browser navigation was initiated via a renderer history back forward 
   mechanism. It is waiting for the unload ack from the renderer.
2. A new navigation from the renderer comes in. We allow this as the 
   pending navigation above is a renderer navigation.
3. Eventually the ack comes in. The navigation which is now underway is 
   the one initiated in step 2 and hence the state check fails.

[ananta, 2017-02-16 00:36:00]

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

[commit-bot: I haz the power, 2017-02-16 00:38:04]

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

[Charlie Reis, 2017-02-16 01:05:50]

Sorry for the delay-- I'll try to take a look tomorrow as well.  I think I had a
run-in with this test with the new session history logic, and I'd like to
understand the change needed.

One minor nit: I would suggest has_user_gesture rather than is_user_gesture,
since it can be a bit indirect.

[ananta, 2017-02-16 01:25:19]

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

[commit-bot: I haz the power, 2017-02-16 01:27:05]

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

[ananta, 2017-02-16 01:34:22]

On 2017/02/16 01:05:50, Charlie Reis wrote:
> Sorry for the delay-- I'll try to take a look tomorrow as well.  I think I had
a
> run-in with this test with the new session history logic, and I'd like to
> understand the change needed.
> 
> One minor nit: I would suggest has_user_gesture rather than is_user_gesture,
> since it can be a bit indirect.

Thanks. Renamed the parameter.

[commit-bot: I haz the power, 2017-02-16 03:31:00]

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

[commit-bot: I haz the power, 2017-02-16 03:31:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[ananta, 2017-02-16 20:25:37]

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

[commit-bot: I haz the power, 2017-02-16 20:27:03]

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, 2017-02-16 22:11:21]

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

[commit-bot: I haz the power, 2017-02-16 22:11:23]

Dry run: This issue passed the CQ dry run.

[clamy, 2017-02-21 13:46:36]

Thanks! The patch looks good, just one question below.

https://codereview.chromium.org/2697753002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2697753002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:998:
NavigationRequest::WAITING_FOR_RENDERER_RESPONSE) {
If the result of BeforeUnload is not proceed, shouldn't we still cancel the
navigation?

[Charlie Reis, 2017-02-21 17:40:00]

This one should be closed now that https://codereview.chromium.org/2701093002
has landed, right?

[ananta, 2017-02-21 19:05:51]

On 2017/02/21 17:40:00, Charlie Reis wrote:
> This one should be closed now that https://codereview.chromium.org/2701093002
> has landed, right?

Yes. Sorry I missed this.

[Charlie Reis, 2017-02-21 19:08:13]

Description was changed from

==========
PlzNavigate: Fix dynamic iframe back forward layout test failure

This test creates an iframe and changes the src of the iframe to different URLs
based on the
session storage property. Additionally it also moves back in history in the
process. We get renderer initiated
navigations initially followed by a browser navigation for the history and then
a subsequent navigation for the child
frame which is treated as a history navigation in Blink as the history
navigation initated above is still incomplete.

For PlzNavigate the history navigation initiated for the frame waits for the
unload handler on the iframe to complete.
This is a browser navigation. Before the unload ack comes in we receive a
navigation IPC which is ignored as the pending
navigation is a browser navigation and the new navigation does not have
user_gesture.

The attempted fix is to treat child frame history navigations as renderer
navigations if they are not initiated by a user
action.

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

to

==========
PlzNavigate: Fix dynamic iframe back forward layout test failure

This test creates an iframe and changes the src of the iframe to different URLs
based on the
session storage property. Additionally it also moves back in history in the
process. We get renderer initiated
navigations initially followed by a browser navigation for the history and then
a subsequent navigation for the child
frame which is treated as a history navigation in Blink as the history
navigation initated above is still incomplete.

For PlzNavigate the history navigation initiated for the frame waits for the
unload handler on the iframe to complete.
This is a browser navigation. Before the unload ack comes in we receive a
navigation IPC which is ignored as the pending
navigation is a browser navigation and the new navigation does not have
user_gesture.

The attempted fix is to treat child frame history navigations as renderer
navigations if they are not initiated by a user
action.

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