Tell renderer which subframes have history items on back/forward.

Tell renderer which subframes have history items on back/forward.

This avoids making an IPC trip to the browser process for frames
that will not find a history item there.

BUG=649345, 639842
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/c8ca51e469c96ab4a3e1b0cd9772687077b38984
Cr-Commit-Position: refs/heads/master@{#421972}

[Charlie Reis, 2016-09-29 06:51:34]

Description was changed from

==========
Tell renderer which subframes have history items on back/forward.

This avoids making an IPC trip to the browser process for frames
that will not find a history item there.

BUG=649345
TEST=See bug for repro steps.
==========

to

==========
Tell renderer which subframes have history items on back/forward.

This avoids making an IPC trip to the browser process for frames
that will not find a history item there.

BUG=649345
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-09-29 06:51:45]

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

[commit-bot: I haz the power, 2016-09-29 06:51:57]

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-29 07:00:49]

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

[commit-bot: I haz the power, 2016-09-29 07:00:51]

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

[Charlie Reis, 2016-09-29 17:33:08]

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

[commit-bot: I haz the power, 2016-09-29 17:33:39]

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

[Charlie Reis, 2016-09-29 17:37:13]

Description was changed from

==========
Tell renderer which subframes have history items on back/forward.

This avoids making an IPC trip to the browser process for frames
that will not find a history item there.

BUG=649345
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Tell renderer which subframes have history items on back/forward.

This avoids making an IPC trip to the browser process for frames
that will not find a history item there.

BUG=649345, 639842
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-09-29 17:37:13]

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

[Charlie Reis, 2016-09-29 17:42:43]

Alex, can you start reviewing this?  I'm still trying to get a test to exercise
the bug, since it's interesting that it doesn't affect all fallback cases (e.g.,
the FrameNavigationEntry_SubframeHistoryFallback test).

Note that this builds on https://codereview.chromium.org/2316003002 from
https://crbug.com/638088, and partly resolves some of the TODOs.  Both CLs will
likely need to be merged to M54.

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

https://codereview.chromium.org/2380943002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:1173:
parent_frame->browser_has_subtree_history_items_;
We don't need to inherit anymore, because the child will check its parent's set
in decidePolicyForNavigation.  I don't think there's a need to send the whole
subtree, since we'll get the child's set of unique names when we ask for its
history item.

[commit-bot: I haz the power, 2016-09-29 18:38:48]

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

[commit-bot: I haz the power, 2016-09-29 18:38:49]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-09-29 20:47:41]

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

[commit-bot: I haz the power, 2016-09-29 20:48:08]

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

[Charlie Reis, 2016-09-29 20:51:50]

Ok, test added!  (I boiled it down to a dynamically generated iframe that is
sent to a data URL.  In the fallback case, we were breaking because the browser
told the renderer to go to about:blank after it went to the data URL.)

[commit-bot: I haz the power, 2016-09-29 21:50:11]

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

[commit-bot: I haz the power, 2016-09-29 21:50:12]

Dry run: This issue passed the CQ dry run.

[alexmos, 2016-09-29 21:55:53]

LGTM, thanks!  Nice improvement from just passing the bool flag, and seems
totally reasonable.  A couple of nits, but feel free to skip those to commit
faster and make today's canary.

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

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3083:
NavigateToURL(shell(), main_url_a);
nit: EXPECT_TRUE, also below

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3120: //
this safeguards against flakiness.)
Wow, I'm curious how the flakiness could possibly occur.  Was the resolution of
Date.now() not enough so that it was actually giving it the same name as before?
 performance.now() might be less prone to that.

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3142: //
name and the new FrameNavigationEntry for the fallback navigation.
Just curious, why do we need to keep the stale FrameNavigationEntry around?

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3147:
entry1->root_node()->children[1]->frame_entry->url().scheme());
Not that it matters, but you could also double-check that the frame_unique_name
is not equal to "wrong" at this point.

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

https://codereview.chromium.org/2380943002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:5600: // Keep track of which subframes
that the browser process has history
nit: s/Keep track of which subframes/Track subframes/  (or just remove "that")

[Charlie Reis, 2016-09-29 22:07:10]

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

[commit-bot: I haz the power, 2016-09-29 22:07:32]

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

[dcheng, 2016-09-29 22:08:15]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-09-29 22:08:16]

ipc lgtm, but one question: won't this set possibly be huge, given the current
bugs around unique name?

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:190: std::set<std::string>
subframe_unique_names,
Nit: const ref

[Charlie Reis, 2016-09-29 22:12:55]

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

[commit-bot: I haz the power, 2016-09-29 22:13:07]

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

[Charlie Reis, 2016-09-29 22:22:48]

On 2016/09/29 22:08:16, dcheng wrote:
> ipc lgtm, but one question: won't this set possibly be huge, given the current
> bugs around unique name?

Yes, that is a downside.  I suppose we could consider a hashing approach for
this IPC (and hope that we don't get collisions as we prune items from the set),
but I'm less inclined to add that to this CL (in terms of complexity).  Maybe in
a followup CL, if we think the general work to put a length limit on unique name
isn't coming soon enough?

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

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3083:
NavigateToURL(shell(), main_url_a);
On 2016/09/29 21:55:53, alexmos wrote:
> nit: EXPECT_TRUE, also below

D'oh!  Done.  One day I'll fix the rest of the occurrences in this file so I
stop copy/pasting them.  :)

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3120: //
this safeguards against flakiness.)
On 2016/09/29 21:55:53, alexmos wrote:
> Wow, I'm curious how the flakiness could possibly occur.  Was the resolution
of
> Date.now() not enough so that it was actually giving it the same name as
before?
>  performance.now() might be less prone to that.

It wasn't actually failing for me, but I was trying to eliminate other sources
of flakiness as I investigated it.  I agree that performance.now does seem to be
even less likely to flake here, so I've removed this line.

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3142: //
name and the new FrameNavigationEntry for the fallback navigation.
On 2016/09/29 21:55:53, alexmos wrote:
> Just curious, why do we need to keep the stale FrameNavigationEntry around?

In general, we don't remove subframe history items when frames are removed from
the page.  That way frames can be restored if they get deleted by some JS action
and then you leave and come back.

In this case, I suppose it's possible we could clear out any unused FNEs when
going back at DidStopLoading, but we don't really track which ones were used
during the navigation.  Plus it could be there if you leave and come back again
(in case the page was just broken for a bit).

Anyway, seems in line with the frame removal case, so I think it's safe to leave
as is.

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3147:
entry1->root_node()->children[1]->frame_entry->url().scheme());
On 2016/09/29 21:55:53, alexmos wrote:
> Not that it matters, but you could also double-check that the
frame_unique_name
> is not equal to "wrong" at this point.

I'll stick with the scheme check, now that I've removed the "wrong" name.

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/2380943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:190: std::set<std::string>
subframe_unique_names,
On 2016/09/29 22:08:16, dcheng wrote:
> Nit: const ref

Done.

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

https://codereview.chromium.org/2380943002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:5600: // Keep track of which subframes
that the browser process has history
On 2016/09/29 21:55:53, alexmos wrote:
> nit: s/Keep track of which subframes/Track subframes/  (or just remove "that")

Done.

[Charlie Reis, 2016-09-29 22:42:51]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-09-29 22:42:56]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2016-09-29 22:42:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexmos@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2380943002/#ps80001
(title: "Fix const ref.")

[commit-bot: I haz the power, 2016-09-29 22:43:16]

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-29 23:10:47]

Description was changed from

==========
Tell renderer which subframes have history items on back/forward.

This avoids making an IPC trip to the browser process for frames
that will not find a history item there.

BUG=649345, 639842
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Tell renderer which subframes have history items on back/forward.

This avoids making an IPC trip to the browser process for frames
that will not find a history item there.

BUG=649345, 639842
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-29 23:10:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-29 23:16:27]

Description was changed from

==========
Tell renderer which subframes have history items on back/forward.

This avoids making an IPC trip to the browser process for frames
that will not find a history item there.

BUG=649345, 639842
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Tell renderer which subframes have history items on back/forward.

This avoids making an IPC trip to the browser process for frames
that will not find a history item there.

BUG=649345, 639842
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/c8ca51e469c96ab4a3e1b0cd9772687077b38984
Cr-Commit-Position: refs/heads/master@{#421972}
==========

[commit-bot: I haz the power, 2016-09-29 23:16:28]

Patchset 5 (id:??) landed as
https://crrev.com/c8ca51e469c96ab4a3e1b0cd9772687077b38984
Cr-Commit-Position: refs/heads/master@{#421972}