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

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

This is a minimal change that avoids having the renderer make IPCs to
the browser to get history items that would just turn out to be
about:blank.  The renderer can commit these synchronously.

This approach has one known issue: going back to a page whose
subframe had a real initial URL but was navigated to about:blank will
break (and load the initial URL rather than about:blank).

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

[Closed in favor of a more correct fix: https://codereview.chromium.org/2438743005/]

[Charlie Reis, 2016-10-20 19:08:11]

Description was changed from

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

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

to

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

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

[Charlie Reis, 2016-10-20 19:09:38]

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

[commit-bot: I haz the power, 2016-10-20 19:10: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, 2016-10-20 19:11:25]

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

[Charlie Reis, 2016-10-20 19:11:26]

Alex, what do you think about this as a followup to
https://codereview.chromium.org/2380943002?  It would break going back to a page
whose iframe started at a real URL and later went to about:blank, but I think I
can fix that in a followup and it seems much less severe for M54.

Also, any ideas on how to write a test page to repro this are welcome.  :)

[commit-bot: I haz the power, 2016-10-20 19:43:22]

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

[commit-bot: I haz the power, 2016-10-20 19:43:22]

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

[Charlie Reis, 2016-10-20 23:00:06]

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

[commit-bot: I haz the power, 2016-10-20 23:01:03]

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-20 23:18:02]

Alex, can you take a look at PS2?  It still breaks the case where we left a real
subframe URL to go to about:blank, but I think it should handle the main bug and
the srcdoc cases.

The alternative is moving further toward fixing https://crbug.com/639842. 
That's better behavior, but a larger/riskier fix to merge.  Could still be worth
it, though.

I'll try to write some tests in the meantime.

[Charlie Reis, 2016-10-21 01:13: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 01:14:12]

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 01:18:51]

Ok, new tests added, and this is ready for review.  They should show the bug is
fixed, but they also show that forms injected into about:blank pages don't get
restored (thus the PageState doesn't matter) while forms in srcdoc pages do get
restored (thus the PageState does matter).  This further justifies that we want
the renderer to request history items for srcdoc but not for about:blank.

I'll spin up a second CL to try out the larger fix that addresses the realURL ->
about:blank case, so that we have both options available.

[commit-bot: I haz the power, 2016-10-21 03:59:44]

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

[commit-bot: I haz the power, 2016-10-21 03:59:46]

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_...)

[Charlie Reis, 2016-10-21 04:39:18]

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

[Charlie Reis, 2016-10-21 04:39:25]

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

https://codereview.chromium.org/2437173002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3388:
ASSERT_EQ(1U, root->child_at(0)->child_count());
Not sure why this isn't working in PlzNavigate (even before my changes).  I've
had to disable the test in that mode for now.

[commit-bot: I haz the power, 2016-10-21 04:39:27]

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:21:56]

Description was changed from

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

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 is a minimal change that avoids having the renderer make IPCs to
the browser to get history items that would just turn out to be
about:blank.  The renderer can commit these synchronously.

This approach has one known issue: going back to a page whose
subframe had a real initial URL but was navigated to about:blank will
break (and load the initial URL rather than about:blank).

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

[alexmos, 2016-10-21 06:34:16]

LGTM with nits.  Thanks for the thorough tests!  I'll take a look at the second
approach next.

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

https://codereview.chromium.org/2437173002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3311:
std::string form_value;
nit: maybe initialize to something other than ""?  Though I guess the
EXPECT_TRUE would catch any ExecuteScript... failures.

https://codereview.chromium.org/2437173002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3388:
ASSERT_EQ(1U, root->child_at(0)->child_count());
On 2016/10/21 04:39:25, Charlie Reis (OOO until 10-19) wrote:
> Not sure why this isn't working in PlzNavigate (even before my changes).  I've
> had to disable the test in that mode for now.

Acknowledged.  I tried it out locally, and it seems it alternates between
loading and not loading the iframes (i.e., going forward and back again would
work the second time).  Anyway, something to investigate separately.

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

https://codereview.chromium.org/2437173002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:883: // about:blank, but we
can detect in the PageState's URL.
nit: missing "it" after can detect?

https://codereview.chromium.org/2437173002/diff/60001/content/test/data/navig...
File
content/test/data/navigation_controller/inject_iframe_srcdoc_with_nested_frame.html
(right):

https://codereview.chromium.org/2437173002/diff/60001/content/test/data/navig...
content/test/data/navigation_controller/inject_iframe_srcdoc_with_nested_frame.html:4:
<p>This page injects an inner frame into an injected blank iframe.
nit: s/blank/srcdoc/?  Also, the page really injects just the srcdoc frame, not
the inner frame.

https://codereview.chromium.org/2437173002/diff/60001/content/test/data/navig...
content/test/data/navigation_controller/inject_iframe_srcdoc_with_nested_frame.html:8:
'<iframe srcdoc="<iframe src=\'form.html\' id=\'outer\'>' +
nit: remove "outer", as it's unused here?  Or rename to "inner" :)

[commit-bot: I haz the power, 2016-10-21 07:28:05]

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

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

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-10-21 07:34:00]

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:34:20]

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 07:34:30]

Thanks!  Nits fixed (and I'll apply them to the other CL as well).  I won't land
this until we decide which is the better route to go.

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

https://codereview.chromium.org/2437173002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3311:
std::string form_value;
On 2016/10/21 06:34:16, alexmos wrote:
> nit: maybe initialize to something other than ""?  Though I guess the
> EXPECT_TRUE would catch any ExecuteScript... failures.

Done.

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

https://codereview.chromium.org/2437173002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:883: // about:blank, but we
can detect in the PageState's URL.
On 2016/10/21 06:34:16, alexmos wrote:
> nit: missing "it" after can detect?

Done.

https://codereview.chromium.org/2437173002/diff/60001/content/test/data/navig...
File
content/test/data/navigation_controller/inject_iframe_srcdoc_with_nested_frame.html
(right):

https://codereview.chromium.org/2437173002/diff/60001/content/test/data/navig...
content/test/data/navigation_controller/inject_iframe_srcdoc_with_nested_frame.html:4:
<p>This page injects an inner frame into an injected blank iframe.
On 2016/10/21 06:34:16, alexmos wrote:
> nit: s/blank/srcdoc/?  Also, the page really injects just the srcdoc frame,
not
> the inner frame.

Done.

https://codereview.chromium.org/2437173002/diff/60001/content/test/data/navig...
content/test/data/navigation_controller/inject_iframe_srcdoc_with_nested_frame.html:8:
'<iframe srcdoc="<iframe src=\'form.html\' id=\'outer\'>' +
On 2016/10/21 06:34:16, alexmos wrote:
> nit: remove "outer", as it's unused here?  Or rename to "inner" :)

Done.

[commit-bot: I haz the power, 2016-10-21 08:52:47]

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

[commit-bot: I haz the power, 2016-10-21 08:52:47]

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_...)

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

Description was changed from

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

This is a minimal change that avoids having the renderer make IPCs to
the browser to get history items that would just turn out to be
about:blank.  The renderer can commit these synchronously.

This approach has one known issue: going back to a page whose
subframe had a real initial URL but was navigated to about:blank will
break (and load the initial URL rather than about:blank).

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 is a minimal change that avoids having the renderer make IPCs to
the browser to get history items that would just turn out to be
about:blank.  The renderer can commit these synchronously.

This approach has one known issue: going back to a page whose
subframe had a real initial URL but was navigated to about:blank will
break (and load the initial URL rather than about:blank).

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

[Closed in favor of a more correct fix:
https://codereview.chromium.org/2438743005/]
==========

[Charlie Reis, 2016-10-21 20:48:50]

Closed this in favor of landing https://codereview.chromium.org/2438743005/
instead, since that one is more correct.