PlzNavigate: Transmit referrer into FrameNavigationEntry.

PlzNavigate: Transmit referrer into FrameNavigationEntry.

Referrer information was lost for a subframe navigation. Consequently,
it was not stored in a FrameNavigationEntry and not properly restored
when the user come back to the main page using the back button.

This CL **partially** fixes the tests:
 * http/tests/navigation/post-frame-goback1.html
 * virtual/stable/http/tests/navigation/post-frames-goback1.html
with --enable-browser-side-navigation flag.

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

Committed: https://crrev.com/4cb7d05bc2e30d147c38ba73e9697f4559e55c0f
Cr-Commit-Position: refs/heads/master@{#427314}

[arthursonzogni, 2016-10-19 11:41:02]

Description was changed from

==========
PlzNavigate: Add referrer for subframe navigation.

Referrer was not transmitted for subframe navigation.

This CL **partially** fix the bug with the test:
http/tests/navigation/post-frame-goback1.html
with --enable-browser-side-navigation flag.

BUG=648588
==========

to

==========
PlzNavigate: Add referrer for subframe navigation.

Referrer was not transmitted for subframe navigation.

This CL **partially** fixes the test:
http/tests/navigation/post-frame-goback1.html
with --enable-browser-side-navigation flag.

BUG=648588
==========

[arthursonzogni, 2016-10-19 11:41:50]

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

[commit-bot: I haz the power, 2016-10-19 11:42:27]

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-10-19 12:13:59]

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

[commit-bot: I haz the power, 2016-10-19 12:14:00]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[arthursonzogni, 2016-10-19 13:04:55]

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

[commit-bot: I haz the power, 2016-10-19 13:05:23]

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-10-19 14:01:22]

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

[commit-bot: I haz the power, 2016-10-19 14:01:23]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2016-10-19 14:05:24]

arthursonzogni@chromium.org changed reviewers:
+ clamy@chromium.org

[arthursonzogni, 2016-10-19 14:05:25]

Hi Camille,

This is a small CL. Referrer was not set for subframe navigation.

Could you please take a look ?

Thanks,
Arthur

[arthursonzogni, 2016-10-19 14:05:56]

Patchset #1 (id:1) has been deleted

[clamy, 2016-10-19 14:34:51]

https://codereview.chromium.org/2433743004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2433743004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4813: if (ds->isClientRedirect()) {
This is happening in the middle of the non-PlzNavigate code. Can you explain how
that helps with the PlzNavigate case?

[arthursonzogni, 2016-10-19 15:53:26]

Description was changed from

==========
PlzNavigate: Add referrer for subframe navigation.

Referrer was not transmitted for subframe navigation.

This CL **partially** fixes the test:
http/tests/navigation/post-frame-goback1.html
with --enable-browser-side-navigation flag.

BUG=648588
==========

to

==========
PlzNavigate: Add referrer for subframe navigation.

Referrer was not transmitted for subframe navigation.

This CL **partially** fixes the tests:
 * http/tests/navigation/post-frame-goback1.html
 * virtual/stable/http/tests/navigation/post-frames-goback1.html
with --enable-browser-side-navigation flag.

BUG=648588
==========

[arthursonzogni, 2016-10-20 12:48:39]

On 2016/10/19 14:34:51, clamy wrote:
>
https://codereview.chromium.org/2433743004/diff/20001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/2433743004/diff/20001/content/renderer/render...
> content/renderer/render_frame_impl.cc:4813: if (ds->isClientRedirect()) {
> This is happening in the middle of the non-PlzNavigate code. Can you explain
how
> that helps with the PlzNavigate case?

Good question!

When the iframe has committed its navigation from nothing (about:blank) to
"form-target.pl", the browser creates a FrameNavigationEntry representing the
navigation.
This navigation is restored later from it.

It translates to code by:
+-------------------------------------------------
| - RenderFrameImpl::SendDidCommitProvisionalLoad() <- params.referer is lost.
|..[IPC]...
| - RenderFrameHostImpl::OnDidCommitProvisionalLoad()
| - NavigatorImpl::DidNavigate()
| - NavigationControllerImpl::RendererDidNavigate()
| - NavigationControllerImpl::RendererDidNavigateAutoSubframe()
| - NavigationEntryImpl::AddOrUpdateFrameEntry()
| - FrameNavigationEntry::UpdateEntry()    <- referrer is normally set here.
+-------------------------------------------------

With non-plznavigate, it seems that the referrer data is not restored from a
FrameNavigationEntry.
Everything is not clear to me. Please tell me what do you think of it? I will
investigate in the meantime.

[clamy, 2016-10-20 12:51:28]

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

[clamy, 2016-10-20 12:53:08]

Ok I see the issue now.
+creis for the review since this concerns history. The issue is that we don't
report the referrer for subframe navigations in DidCommitProvisionalLoad, so the
FrameNavigationEntry is not updated. Therefore a subsequent history navigation
will not set up the referrer properly.

[arthursonzogni, 2016-10-21 09:39:49]

Description was changed from

==========
PlzNavigate: Add referrer for subframe navigation.

Referrer was not transmitted for subframe navigation.

This CL **partially** fixes the tests:
 * http/tests/navigation/post-frame-goback1.html
 * virtual/stable/http/tests/navigation/post-frames-goback1.html
with --enable-browser-side-navigation flag.

BUG=648588
==========

to

==========
PlzNavigate: Transmit referrer into FrameNavigationEntry.

Referrer information was lost for a subframe navigation. Consequently,
it was not stored in a FrameNavigationEntry and not properly restored
when the user come back to the main page using the back button.

This CL **partially** fixes the tests:
 * http/tests/navigation/post-frame-goback1.html
 * virtual/stable/http/tests/navigation/post-frames-goback1.html
with --enable-browser-side-navigation flag.

BUG=648588
==========

[Charlie Reis, 2016-10-21 20:15:58]

Looks right-- can you add a test showing the bug in the non-PlzNavigate case?

https://codereview.chromium.org/2433743004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2433743004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4813: if (ds->isClientRedirect()) {
On 2016/10/20 12:48:39, arthursonzogni wrote:
> On 2016/10/19 14:34:51, clamy wrote:
> >
>
https://codereview.chromium.org/2433743004/diff/20001/content/renderer/render...
> > File content/renderer/render_frame_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2433743004/diff/20001/content/renderer/render...
> > content/renderer/render_frame_impl.cc:4813: if (ds->isClientRedirect()) {
> > This is happening in the middle of the non-PlzNavigate code. Can you explain
> how
> > that helps with the PlzNavigate case?
> 
> Good question!
> 
> When the iframe has committed its navigation from nothing (about:blank) to
> "form-target.pl", the browser creates a FrameNavigationEntry representing the
> navigation.
> This navigation is restored later from it.
> 
> It translates to code by:
> +-------------------------------------------------
> | - RenderFrameImpl::SendDidCommitProvisionalLoad() <- params.referer is lost.
> |..[IPC]...
> | - RenderFrameHostImpl::OnDidCommitProvisionalLoad()
> | - NavigatorImpl::DidNavigate()
> | - NavigationControllerImpl::RendererDidNavigate()
> | - NavigationControllerImpl::RendererDidNavigateAutoSubframe()
> | - NavigationEntryImpl::AddOrUpdateFrameEntry()
> | - FrameNavigationEntry::UpdateEntry()    <- referrer is normally set here.
> +-------------------------------------------------
> 
> With non-plznavigate, it seems that the referrer data is not restored from a
> FrameNavigationEntry.
> Everything is not clear to me. Please tell me what do you think of it? I will
> investigate in the meantime.

Good catch!  It does look like we were missing the value on subframes, even in
non-PlzNavigate.

Can you add a check for this in a NavigationControllerBrowserTest?  If any
existing tests hit this case for subframes, you can just verify that the FNE's
referrer is correct afterward.  (If not, it shouldn't be too hard to add a new
test for it there.)

[arthursonzogni, 2016-10-24 14:56:34]

Description was changed from

==========
PlzNavigate: Transmit referrer into FrameNavigationEntry.

Referrer information was lost for a subframe navigation. Consequently,
it was not stored in a FrameNavigationEntry and not properly restored
when the user come back to the main page using the back button.

This CL **partially** fixes the tests:
 * http/tests/navigation/post-frame-goback1.html
 * virtual/stable/http/tests/navigation/post-frames-goback1.html
with --enable-browser-side-navigation flag.

BUG=648588
==========

to

==========
PlzNavigate: Transmit referrer into FrameNavigationEntry.

Referrer information was lost for a subframe navigation. Consequently,
it was not stored in a FrameNavigationEntry and not properly restored
when the user come back to the main page using the back button.

This CL **partially** fixes the tests:
 * http/tests/navigation/post-frame-goback1.html
 * virtual/stable/http/tests/navigation/post-frames-goback1.html
with --enable-browser-side-navigation flag.

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

[arthursonzogni, 2016-10-24 14:57:20]

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

[commit-bot: I haz the power, 2016-10-24 14:57:40]

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

[arthursonzogni, 2016-10-24 14:57:43]

The CQ bit was unchecked by arthursonzogni@chromium.org

[arthursonzogni, 2016-10-24 14:58:16]

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

[commit-bot: I haz the power, 2016-10-24 14:58:41]

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

[arthursonzogni, 2016-10-24 14:58:57]

The CQ bit was unchecked by arthursonzogni@chromium.org

[arthursonzogni, 2016-10-24 15:00:18]

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

[commit-bot: I haz the power, 2016-10-24 15:00:38]

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

[arthursonzogni, 2016-10-24 15:04:34]

Hi Charlie,

Thanks! I added tests. I confirm that the bug was present, even in
non-PlzNavigate.

[commit-bot: I haz the power, 2016-10-24 16:54:17]

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

[commit-bot: I haz the power, 2016-10-24 16:54:18]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Charlie Reis, 2016-10-24 17:26:44]

Thanks for the test!  LGTM with nits.

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

https://codereview.chromium.org/2433743004/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6780: //
Check that the FrameNavigationEntry's referrer.
nit: Drop "that"

https://codereview.chromium.org/2433743004/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6790:
RefererStoredForSubFrameWithRedirect) {
There's a fair amount of overhead to each browser test, with firing up a full
new instance of the browser.  Given how related these two cases are, I think
it's safe to combine them into the same test (one after the other).

https://codereview.chromium.org/2433743004/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6806: //
Check that the FrameNavigationEntry's referrer.
nit: Drop "that"

[arthursonzogni, 2016-10-25 08:43:04]

Patchset #3 (id:60001) has been deleted

[arthursonzogni, 2016-10-25 08:46:17]

Patchset #3 (id:80001) has been deleted

[arthursonzogni, 2016-10-25 09:02:40]

Thanks for your review!
I use a loop instead of concatenating the tests, I assume it's still okay to
commit the patch.

[arthursonzogni, 2016-10-25 09:03:28]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2016-10-25 09:03:28]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2433743004/#ps100001
(title: "Merge tests")

[commit-bot: I haz the power, 2016-10-25 09:03:47]

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-10-25 10:54:41]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-10-25 10:58:35]

Description was changed from

==========
PlzNavigate: Transmit referrer into FrameNavigationEntry.

Referrer information was lost for a subframe navigation. Consequently,
it was not stored in a FrameNavigationEntry and not properly restored
when the user come back to the main page using the back button.

This CL **partially** fixes the tests:
 * http/tests/navigation/post-frame-goback1.html
 * virtual/stable/http/tests/navigation/post-frames-goback1.html
with --enable-browser-side-navigation flag.

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

to

==========
PlzNavigate: Transmit referrer into FrameNavigationEntry.

Referrer information was lost for a subframe navigation. Consequently,
it was not stored in a FrameNavigationEntry and not properly restored
when the user come back to the main page using the back button.

This CL **partially** fixes the tests:
 * http/tests/navigation/post-frame-goback1.html
 * virtual/stable/http/tests/navigation/post-frames-goback1.html
with --enable-browser-side-navigation flag.

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

Committed: https://crrev.com/4cb7d05bc2e30d147c38ba73e9697f4559e55c0f
Cr-Commit-Position: refs/heads/master@{#427314}
==========

[commit-bot: I haz the power, 2016-10-25 10:58:36]

Patchset 3 (id:??) landed as
https://crrev.com/4cb7d05bc2e30d147c38ba73e9697f4559e55c0f
Cr-Commit-Position: refs/heads/master@{#427314}

[Charlie Reis, 2016-10-25 16:48:42]

On 2016/10/25 09:02:40, arthursonzogni wrote:
> Thanks for your review!
> I use a loop instead of concatenating the tests, I assume it's still okay to
> commit the patch.

This is ok, but I would have suggested avoiding that.  There wasn't much code to
duplicate here, and now the line numbers in a test failure won't easily reveal
which part was failing.  (In general, copy/paste in tests is less of a problem
than in real code, especially for blocks this small.)  Just makes it a bit
harder to diagnose when it fails.

[arthursonzogni, 2016-10-26 08:02:06]

On 2016/10/25 16:48:42, Charlie Reis wrote:
> On 2016/10/25 09:02:40, arthursonzogni wrote:
> > Thanks for your review!
> > I use a loop instead of concatenating the tests, I assume it's still okay to
> > commit the patch.
> 
> This is ok, but I would have suggested avoiding that.  There wasn't much code
to
> duplicate here, and now the line numbers in a test failure won't easily reveal
> which part was failing.  (In general, copy/paste in tests is less of a problem
> than in real code, especially for blocks this small.)  Just makes it a bit
> harder to diagnose when it fails.

Good point. I will remember that next time.