(Reland) Remove about:srcdoc url conversion.

Remove about:srcdoc url conversion.

This is a reland of https://codereview.chromium.org/2494633004/

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about::blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
TBR=dbeam,nasko

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

[arthursonzogni, 2016-11-30 17:08:56]

Patchset #2 (id:20001) has been deleted

[arthursonzogni, 2016-11-30 17:11:21]

Patchset #2 (id:40001) has been deleted

[arthursonzogni, 2016-11-30 17:16:43]

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

[commit-bot: I haz the power, 2016-11-30 17:17:02]

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

[arthursonzogni, 2016-11-30 17:20:10]

Description was changed from

==========
(Reland) Remove about:srcdoc url conversion.

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about:blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
==========

to

==========
Remove about:srcdoc url conversion.

This is a reland of https://codereview.chromium.org/2494633004/

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about:blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
==========

[arthursonzogni, 2016-11-30 17:20:55]

Description was changed from

==========
Remove about:srcdoc url conversion.

This is a reland of https://codereview.chromium.org/2494633004/

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about:blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
==========

to

==========
Remove about:srcdoc url conversion.

This is a reland of https://codereview.chromium.org/2494633004/

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about::blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
==========

[arthursonzogni, 2016-11-30 17:25:19]

Patchset #2 (id:60001) has been deleted

[arthursonzogni, 2016-11-30 17:26:04]

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

[commit-bot: I haz the power, 2016-11-30 17:26:25]

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-11-30 18:44:21]

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

[commit-bot: I haz the power, 2016-11-30 18:44:22]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2016-12-01 09:39:50]

arthursonzogni@chromium.org changed reviewers:
+ creis@chromium.org, nasko@chromium.org

[arthursonzogni, 2016-12-01 09:39:51]

Hi Nasko and Charlie,

Unfortunately my last patch got reverted.
I understood why (see comment below).

Please, can you take a look?
Thanks!

https://codereview.chromium.org/2534393003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2534393003/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:879:
child->frame_entry->url() == url::kAboutBlankURL;
Too bad we can't keep this...
What no longer applies to about:srcdoc stills applies to about:blank/#identifier
for instance.

The browser thinks the subframe has navigated to about:blank, but the renderer
knows that the real URL is about:blank/#identifier.

This caused a test to fail:
fast/history/history-length-append-subframe-with-hash.html

[Charlie Reis, 2016-12-02 00:20:33]

https://codereview.chromium.org/2534393003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2534393003/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:879:
child->frame_entry->url() == url::kAboutBlankURL;
On 2016/12/01 09:39:51, arthursonzogni wrote:
> Too bad we can't keep this...
> What no longer applies to about:srcdoc stills applies to
about:blank/#identifier
> for instance.
> 
> The browser thinks the subframe has navigated to about:blank, but the renderer
> knows that the real URL is about:blank/#identifier.
> 
> This caused a test to fail:
> fast/history/history-length-append-subframe-with-hash.html

Hmm, I think your new version of this is problematic.

This GetSubframeUniqueNames method intentionally keeps track of whether the
subframe will be about:blank, because those frames must be allowed to load
synchronously (for the parent page to be able to immediately inject code into
them).

If we say false for about:blank/#identifier, then that frame will be loaded
asynchronously, leading to bugs like https://crbug.com/626416 and
https://crbug.com/657896 (which I just wrote a postmortem for).  :)

However, I also see why the synchronous approach would cause the test to fail--
saying true here would make the frame load about:blank rather than
about:blank#identifier.

Maybe it's time to finish up https://crbug.com/639842 and pass the full
PageState for any same-process frames, rather than just a boolean of whether
it's about:blank?  I think that would unblock you.

Let me know if you have other ideas in the meantime that don't require as big a
change but avoid the issue I describe above.  (I suppose we could stop rewriting
about:blank/#identifier?  Not sure what the implications would be, but that
could be a net positive change-- these sorts of differences between browser and
renderer do cause bugs, and they were likely overlooked when this rewriting was
introduced.)

[Charlie Reis, 2016-12-02 05:12:51]

https://codereview.chromium.org/2534393003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2534393003/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:879:
child->frame_entry->url() == url::kAboutBlankURL;
On 2016/12/02 00:20:32, Charlie Reis (slow) wrote:
> On 2016/12/01 09:39:51, arthursonzogni wrote:
> > Too bad we can't keep this...
> > What no longer applies to about:srcdoc stills applies to
> about:blank/#identifier
> > for instance.
> > 
> > The browser thinks the subframe has navigated to about:blank, but the
renderer
> > knows that the real URL is about:blank/#identifier.
> > 
> > This caused a test to fail:
> > fast/history/history-length-append-subframe-with-hash.html
> 
> Hmm, I think your new version of this is problematic.
> 
> This GetSubframeUniqueNames method intentionally keeps track of whether the
> subframe will be about:blank, because those frames must be allowed to load
> synchronously (for the parent page to be able to immediately inject code into
> them).
> 
> If we say false for about:blank/#identifier, then that frame will be loaded
> asynchronously, leading to bugs like https://crbug.com/626416 and
> https://crbug.com/657896 (which I just wrote a postmortem for).  :)
> 
> However, I also see why the synchronous approach would cause the test to
fail--
> saying true here would make the frame load about:blank rather than
> about:blank#identifier.
> 
> Maybe it's time to finish up https://crbug.com/639842 and pass the full
> PageState for any same-process frames, rather than just a boolean of whether
> it's about:blank?  I think that would unblock you.
> 
> Let me know if you have other ideas in the meantime that don't require as big
a
> change but avoid the issue I describe above.  (I suppose we could stop
rewriting
> about:blank/#identifier?  Not sure what the implications would be, but that
> could be a net positive change-- these sorts of differences between browser
and
> renderer do cause bugs, and they were likely overlooked when this rewriting
was
> introduced.)

Ah, it occurs to me that this bug is present today!  (I was looking at the diffs
between patchsets and not the overall diff.)  Guess we've discovered a bug to
fix...

As a result, this CL LGTM and I'll follow up on the asynchronous
about:blank/#identifier issue.

[arthursonzogni, 2016-12-05 08:51:20]

arthursonzogni@chromium.org changed reviewers:
+ dbeam@chromium.org, jochen@chromium.org, lfg@chromium.org

[arthursonzogni, 2016-12-05 08:51:21]

Thanks Charlie!

---

Hi @dbeam, @lfg, @jochen. Please, could you take a look at this? It is a reland
of:
https://codereview.chromium.org/2494633004/

Nothing have change, except that I undo the modification to one method:
NavigationEntry::GetSubFrameUniqueNames.

[jochen (gone - plz use gerrit), 2016-12-05 15:25:47]

lgtm

[lfg, 2016-12-05 17:47:04]

guest_view lgtm

PS: feel free to TBR changes like this.

[Charlie Reis, 2016-12-05 18:17:47]

Description was changed from

==========
Remove about:srcdoc url conversion.

This is a reland of https://codereview.chromium.org/2494633004/

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about::blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
==========

to

==========
Remove about:srcdoc url conversion.

This is a reland of https://codereview.chromium.org/2494633004/

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about::blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
TBR=dbeam,nasko
==========

[Charlie Reis, 2016-12-05 18:19:02]

On 2016/12/05 17:47:04, lfg wrote:
> guest_view lgtm
> 
> PS: feel free to TBR changes like this.

Yes, I'm going to TBR this and proceed to land it, since the other reviewers
have already approved on https://codereview.chromium.org/2494633004/ and I've
reviewed the delta.

[Charlie Reis, 2016-12-05 18:19:27]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-12-05 18:19:51]

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-12-05 18:22:29]

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

[commit-bot: I haz the power, 2016-12-05 18:22:31]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Charlie Reis, 2016-12-05 18:36:58]

On 2016/12/05 18:22:31, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   ios-device on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
>   ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
>   mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

Guess this needs a rebase before it can land.

[arthursonzogni, 2016-12-06 09:27:03]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2016-12-06 09:27:03]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
jochen@chromium.org, lfg@chromium.org
Link to the patchset: https://codereview.chromium.org/2534393003/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-12-06 09:27:15]

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-12-06 10:52:32]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481016423316820,
"parent_rev": "ea37d817a119c3b34519b1455af49650c9a87e54", "commit_rev":
"1eb974ce5378424ac80378bfe95c672c3dca2f96"}

[commit-bot: I haz the power, 2016-12-06 10:52:58]

Description was changed from

==========
Remove about:srcdoc url conversion.

This is a reland of https://codereview.chromium.org/2494633004/

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about::blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
TBR=dbeam,nasko
==========

to

==========
Remove about:srcdoc url conversion.

This is a reland of https://codereview.chromium.org/2494633004/

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about::blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
TBR=dbeam,nasko
==========

[commit-bot: I haz the power, 2016-12-06 10:53:00]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-12-06 10:54:33]

Description was changed from

==========
Remove about:srcdoc url conversion.

This is a reland of https://codereview.chromium.org/2494633004/

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about::blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
TBR=dbeam,nasko
==========

to

==========
Remove about:srcdoc url conversion.

This is a reland of https://codereview.chromium.org/2494633004/

Before this CL, there was an asymetry between the renderer and the
browser. Iframe that has navigated to its srcdoc attribute got the
about:srcdoc URL in the renderer and the about::blank in the browser.
The about::srcdoc url were converted to about::blank in the RendererFrameHost.
A boolean |is_srcdoc| was created and transmitted across classes in
the browser to keep track of the "real" url, such as it could be
converted back (browser->renderer).

This led to a lot unnecessary works to keep track of this boolean and
not to forget to convert the URL back.

This CL removes the conversion and the boolean from the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
TBR=dbeam,nasko

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

[commit-bot: I haz the power, 2016-12-06 10:54:35]

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