Fix history nav to a script-injected about:blank frame.

Fix history nav to a script-injected about:blank frame.

This CL tracks whether the browser process has an about:blank history
item for subframes during a history navigation.  If the renderer would
be staying on about:blank, it can skip asking the browser and commit
the initial about:blank page synchronously.

BUG=657896, 639842
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
R=alexmos@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/37c954965f2f221aec294a0a28dfbba1c7543f2a

[Charlie Reis, 2016-10-21 04:50:46]

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

[commit-bot: I haz the power, 2016-10-21 04:50:59]

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-10-21 06:11:35]

Description was changed from

==========
Fix going back to a script-injected about:blank frame.

This CL tracks whether the browser process has an about:blank history
item for subframes during a history navigation.  If the renderer would
be staying on about:blank, it can skip asking the browser and commit
the initial about:blank page synchronously.

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

to

==========
Fix going back to a script-injected about:blank frame.

This CL tracks whether the browser process has an about:blank history
item for subframes during a history navigation.  If the renderer would
be staying on about:blank, it can skip asking the browser and commit
the initial about:blank page synchronously.

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

[Charlie Reis, 2016-10-21 06:11:35]

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

[Charlie Reis, 2016-10-21 06:28:33]

Description was changed from

==========
Fix going back to a script-injected about:blank frame.

This CL tracks whether the browser process has an about:blank history
item for subframes during a history navigation.  If the renderer would
be staying on about:blank, it can skip asking the browser and commit
the initial about:blank page synchronously.

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

to

==========
Fix history nav to a script-injected about:blank frame.

This CL tracks whether the browser process has an about:blank history
item for subframes during a history navigation.  If the renderer would
be staying on about:blank, it can skip asking the browser and commit
the initial about:blank page synchronously.

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

[Charlie Reis, 2016-10-21 06:28:59]

Here's the somewhat bigger version of
https://codereview.chromium.org/2437173002/.  For each child frame, it plumbs
the |is_about_blank| bit to the renderer, who can decide whether we're staying
at an initial about:blank URL or whether about:blank is a history item that
differs from the actual initial URL.  Thus, it's more correct than the smaller
fix, and it's another step toward our plan to ultimately fix
https://crbug.com/639842.

This CL also has the advantage that it removes the whole staying_at_about_blank
logic in RenderFrameImpl, which was unfortunate to begin with.  (That was why we
ended up with blank frames that never generated a commit.)

The main downside is that it's a bit bigger, and thus carries some additional
risk to merge.

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

https://codereview.chromium.org/2438743005/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:871: std::map<std::string,
bool> names;
I was hoping to use a std::unordered_map for this, since the sort order isn't
needed and a constant lookup would have been nice.  That doesn't appear to be
supported for IPC message transport, though.  Shouldn't be a big problem, since
these maps won't typically have more than 100 entries even on enormous pages.

I'll also note that I considered going further and just making this map hold
PageStates rather than bools, but there's a reason I'm considering that too
risky to merge to M54-- it requires sending the whole subtree and not just
immediate children, and that carries some questions about how to store it in the
renderer (flat on one RenderFrame with recursive lookups, or pushed down to
children from a tree structure?).  I'd like that part of the change to bake all
the way through M56 Canary/Dev/Beta, so I'm sticking with the simpler bool for
now.

[commit-bot: I haz the power, 2016-10-21 06:36:18]

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

[commit-bot: I haz the power, 2016-10-21 06:36:19]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-10-21 07:36:16]

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

[commit-bot: I haz the power, 2016-10-21 07:36:39]

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-21 09:00:08]

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

[commit-bot: I haz the power, 2016-10-21 09:00:08]

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

[alexmos, 2016-10-21 17:20:50]

Looks good, just a couple of nits and sanity checking questions below.

I actually don't think this is that much more complicated than the other CL
(mostly just a set->map conversion and a couple of additional small checks), and
I really like that it eliminates the fake stopLoading and the regression with
the subframe at a real URL being navigated to about:blank.  I'd vote for this CL
as the preferred fix, even if it's a bit riskier for the merge. (If we go with
the first fix, there's a chance we'll still need to merge this one later on, if
it turns out someone cares about the regression case.)

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

https://codereview.chromium.org/2438743005/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:871: std::map<std::string,
bool> names;
On 2016/10/21 06:28:59, Charlie Reis (OOO until 10-19) wrote:
> I was hoping to use a std::unordered_map for this, since the sort order isn't
> needed and a constant lookup would have been nice.  That doesn't appear to be
> supported for IPC message transport, though.  Shouldn't be a big problem,
since
> these maps won't typically have more than 100 entries even on enormous pages.
> 
> I'll also note that I considered going further and just making this map hold
> PageStates rather than bools, but there's a reason I'm considering that too
> risky to merge to M54-- it requires sending the whole subtree and not just
> immediate children, and that carries some questions about how to store it in
the
> renderer (flat on one RenderFrame with recursive lookups, or pushed down to
> children from a tree structure?).  I'd like that part of the change to bake
all
> the way through M56 Canary/Dev/Beta, so I'm sticking with the simpler bool for
> now.

Acknowledged.

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

https://codereview.chromium.org/2438743005/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:244: // no-op.)
nit: maybe add a reference to 657896 here as well, as motivation for
|is_about_blank|?

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

https://codereview.chromium.org/2438743005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5744: }
Happy to see this gone!

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

https://codereview.chromium.org/2438743005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5073: !history_item_is_about_blank || url
!= GURL(url::kAboutBlankURL);
Just to make sure I understand the case where |history_item_is_about_blank| is
true but |url| is not about:blank.  (I.e., the scenario from
FrameNavigationEntry_RecreatedSubframeToBlank and what breaks in the CL with the
simpler fix.)  In that case, this will force us to still ask the browser, which
will eventually result in an asynchronous commit to about:blank with the
corresponding PageState?  Is this the behavior this had on the old path?  (I.e.,
is it also asynchronous, and does it restore stuff from PageState?)

https://codereview.chromium.org/2438743005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5073: !history_item_is_about_blank || url
!= GURL(url::kAboutBlankURL);
Another question:  suppose we've got a page with a <iframe src="about:blank">,
then the subframe is navigated to some other URL, then back to "about:blank"
again.  Then we go forward/back on the whole page.  How should it behave?  Seems
like this will make it commit about:blank synchronously and lose the PageState;
is that right?

https://codereview.chromium.org/2438743005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5075:
parent->history_subframe_unique_names_.erase(frame_->uniqueName().utf8());
nit: seems like you could move this inside the previous if.

[Charlie Reis, 2016-10-21 17:45:03]

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

[commit-bot: I haz the power, 2016-10-21 17:45:33]

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-10-21 18:11:52]

Thanks!  Replies below.

On 2016/10/21 17:20:50, alexmos wrote:
> Looks good, just a couple of nits and sanity checking questions below.
> 
> I actually don't think this is that much more complicated than the other CL
> (mostly just a set->map conversion and a couple of additional small checks),
and
> I really like that it eliminates the fake stopLoading and the regression with
> the subframe at a real URL being navigated to about:blank.  I'd vote for this
CL
> as the preferred fix, even if it's a bit riskier for the merge. (If we go with
> the first fix, there's a chance we'll still need to merge this one later on,
if
> it turns out someone cares about the regression case.)

I agree.  If they both had the same behavior, then the smaller CL is less risky.
 Given that the smaller one has a known user-visible bug, it makes sense to do
this more correct one instead.

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

https://codereview.chromium.org/2438743005/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:244: // no-op.)
On 2016/10/21 17:20:49, alexmos wrote:
> nit: maybe add a reference to 657896 here as well, as motivation for
> |is_about_blank|?

Done.

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

https://codereview.chromium.org/2438743005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5744: }
On 2016/10/21 17:20:49, alexmos wrote:
> Happy to see this gone!

Me too!

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

https://codereview.chromium.org/2438743005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5073: !history_item_is_about_blank || url
!= GURL(url::kAboutBlankURL);
On 2016/10/21 17:20:49, alexmos wrote:
> Just to make sure I understand the case where |history_item_is_about_blank| is
> true but |url| is not about:blank.  (I.e., the scenario from
> FrameNavigationEntry_RecreatedSubframeToBlank and what breaks in the CL with
the
> simpler fix.)  In that case, this will force us to still ask the browser,
which
> will eventually result in an asynchronous commit to about:blank with the
> corresponding PageState?

Exactly right.  In fact, it had no commit at all until I removed the
staying_at_about_blank logic from RenderFrameImpl.  The RecreatedSubframeToBlank
test caught that in PS2.

>  Is this the behavior this had on the old path?  (I.e.,
> is it also asynchronous, and does it restore stuff from PageState?)

No, good point.  In the old path, it would have committed synchronously.  (In
both paths, the PageState for about:blank frames seems to be ignored, so it
doesn't matter that we provide it.)

This will be fixed by sending down all the same-process PageStates.  However, I
don't think this is likely to matter much in practice-- the parent page would
have to try to script the iframe synchronously, knowing that the default src URL
*isn't* about:blank, and it's only about:blank in this case due to the history
item.  If it did that, you're right that we would clobber whatever was in there
when the about:blank history item arrived from the browser (for the time being).

https://codereview.chromium.org/2438743005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5073: !history_item_is_about_blank || url
!= GURL(url::kAboutBlankURL);
On 2016/10/21 17:20:49, alexmos wrote:
> Another question:  suppose we've got a page with a <iframe src="about:blank">,
> then the subframe is navigated to some other URL, then back to "about:blank"
> again.  Then we go forward/back on the whole page.  How should it behave? 
Seems
> like this will make it commit about:blank synchronously and lose the
PageState;
> is that right?

Good question.  Yes, this CL will make it commit the about:blank synchronously,
without the PageState.  The good news is that about:blank frames don't appear to
use their PageState (unlike srcdoc frames), so I don't expect it to be a
problem.

https://codereview.chromium.org/2438743005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5075:
parent->history_subframe_unique_names_.erase(frame_->uniqueName().utf8());
On 2016/10/21 17:20:49, alexmos wrote:
> nit: seems like you could move this inside the previous if.

Done.

[alexmos, 2016-10-21 18:22:10]

LGTM!

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

https://codereview.chromium.org/2438743005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5073: !history_item_is_about_blank || url
!= GURL(url::kAboutBlankURL);
On 2016/10/21 18:11:51, Charlie Reis (OOO until 10-19) wrote:
> On 2016/10/21 17:20:49, alexmos wrote:
> > Just to make sure I understand the case where |history_item_is_about_blank|
is
> > true but |url| is not about:blank.  (I.e., the scenario from
> > FrameNavigationEntry_RecreatedSubframeToBlank and what breaks in the CL with
> the
> > simpler fix.)  In that case, this will force us to still ask the browser,
> which
> > will eventually result in an asynchronous commit to about:blank with the
> > corresponding PageState?
> 
> Exactly right.  In fact, it had no commit at all until I removed the
> staying_at_about_blank logic from RenderFrameImpl.  The
RecreatedSubframeToBlank
> test caught that in PS2.
> 
> >  Is this the behavior this had on the old path?  (I.e.,
> > is it also asynchronous, and does it restore stuff from PageState?)
> 
> No, good point.  In the old path, it would have committed synchronously.  (In
> both paths, the PageState for about:blank frames seems to be ignored, so it
> doesn't matter that we provide it.)
> 
> This will be fixed by sending down all the same-process PageStates.  However,
I
> don't think this is likely to matter much in practice-- the parent page would
> have to try to script the iframe synchronously, knowing that the default src
URL
> *isn't* about:blank, and it's only about:blank in this case due to the history
> item.  If it did that, you're right that we would clobber whatever was in
there
> when the about:blank history item arrived from the browser (for the time
being).

Acknowledged.

https://codereview.chromium.org/2438743005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5073: !history_item_is_about_blank || url
!= GURL(url::kAboutBlankURL);
On 2016/10/21 18:11:51, Charlie Reis (OOO until 10-19) wrote:
> On 2016/10/21 17:20:49, alexmos wrote:
> > Another question:  suppose we've got a page with a <iframe
src="about:blank">,
> > then the subframe is navigated to some other URL, then back to "about:blank"
> > again.  Then we go forward/back on the whole page.  How should it behave? 
> Seems
> > like this will make it commit about:blank synchronously and lose the
> PageState;
> > is that right?
> 
> Good question.  Yes, this CL will make it commit the about:blank
synchronously,
> without the PageState.  The good news is that about:blank frames don't appear
to
> use their PageState (unlike srcdoc frames), so I don't expect it to be a
> problem.

Acknowledged.  It's so weird (and fortunate, I guess) that about:blank iframes
ignore the PageState but srcdoc frames don't.  I wonder if the spec says
anything about this behavior.

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

https://codereview.chromium.org/2438743005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:891: if
(UTF16ToUTF8(frame_state.url_string.string()) == url::kAboutBlankURL)
Just out of curiousity - what is the frame_state.url_string for srcdoc frames? 
Is it just empty?

[Charlie Reis, 2016-10-21 18:31:46]

Thanks!

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

https://codereview.chromium.org/2438743005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:891: if
(UTF16ToUTF8(frame_state.url_string.string()) == url::kAboutBlankURL)
On 2016/10/21 18:22:10, alexmos wrote:
> Just out of curiousity - what is the frame_state.url_string for srcdoc frames?

> Is it just empty?

about:srcdoc.

We rewrite all about: URLs to be about:blank in FilterURL, probably without
thinking about this case.

[Charlie Reis, 2016-10-21 18:44:58]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-10-21 18:45:03]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-10-21 18:45:56]

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-21 20:12:36]

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

[commit-bot: I haz the power, 2016-10-21 20:12:37]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Charlie Reis, 2016-10-21 20:37:41]

I'll land this manually, since the linux_android_rel_ng failure is failing for
everyone.  This CL can't get past it because "retries without patch disabled."

[commit-bot: I haz the power, 2016-10-21 20:43:27]

Description was changed from

==========
Fix history nav to a script-injected about:blank frame.

This CL tracks whether the browser process has an about:blank history
item for subframes during a history navigation.  If the renderer would
be staying on about:blank, it can skip asking the browser and commit
the initial about:blank page synchronously.

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

to

==========
Fix history nav to a script-injected about:blank frame.

This CL tracks whether the browser process has an about:blank history
item for subframes during a history navigation.  If the renderer would
be staying on about:blank, it can skip asking the browser and commit
the initial about:blank page synchronously.

BUG=657896, 639842
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
R=alexmos@chromium.org

Committed: https://crrev.com/37c954965f2f221aec294a0a28dfbba1c7543f2a
Cr-Commit-Position: refs/heads/master@{#426880}
==========

[commit-bot: I haz the power, 2016-10-21 20:43:28]

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

[Charlie Reis, 2016-10-21 20:46:53]

Description was changed from

==========
Fix history nav to a script-injected about:blank frame.

This CL tracks whether the browser process has an about:blank history
item for subframes during a history navigation.  If the renderer would
be staying on about:blank, it can skip asking the browser and commit
the initial about:blank page synchronously.

BUG=657896, 639842
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
R=alexmos@chromium.org

Committed: https://crrev.com/37c954965f2f221aec294a0a28dfbba1c7543f2a
Cr-Commit-Position: refs/heads/master@{#426880}
==========

to

==========
Fix history nav to a script-injected about:blank frame.

This CL tracks whether the browser process has an about:blank history
item for subframes during a history navigation.  If the renderer would
be staying on about:blank, it can skip asking the browser and commit
the initial about:blank page synchronously.

BUG=657896, 639842
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
R=alexmos@chromium.org

Committed:
https://chromium.googlesource.com/chromium/src/+/37c954965f2f221aec294a0a28df...
==========

[Charlie Reis, 2016-10-21 20:46:54]

Committed patchset #5 (id:80001) manually as
37c954965f2f221aec294a0a28dfbba1c7543f2a (presubmit successful).