Remove about:srcdoc url conversion.

Remove about:srcdoc url conversion.

Before this CL, there was an asymmetry 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 URLs 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
Committed: https://crrev.com/041956a261766b15f09b7b7a37026bda1ef809f3
Cr-Commit-Position: refs/heads/master@{#435256}

[arthursonzogni, 2016-11-10 16:25:23]

Description was changed from

==========
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,linux_chromium_browser_side_navigation_rel

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

to

==========
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 URLs 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,linux_chromium_browser_side_navigation_rel

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

[arthursonzogni, 2016-11-10 16:29:42]

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

[commit-bot: I haz the power, 2016-11-10 16:29:58]

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-10 17:20:40]

Description was changed from

==========
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 URLs 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,linux_chromium_browser_side_navigation_rel

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

to

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

Before this CL, there was an asymmetry 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 URLs 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,linux_chromium_browser_side_navigation_rel

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

[arthursonzogni, 2016-11-10 17:41:17]

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

[arthursonzogni, 2016-11-10 17:41:18]

Hi Nasko and Charly,

This is the CL that follows ours discussion on:
https://codereview.chromium.org/2482873002/

Before adding more reviewers, I want to have your opinions on the overall
change.
We can now see all the affected files.

+CC @alexmos, @clamy.

[nasko, 2016-11-10 18:14:14]

Thanks a lot for putting this together! It makes code a lot nicer!
The CL can be even smaller if we just teach FilterURL to know of about:srcdoc,
but will also make it safer to handle.

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:879: // Be careful not to
include iframe srcdoc URLs in this check, which do
I really like this cleanup!

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:878: bool is_about_blank =
(child->frame_entry->url() == url::kAboutBlankURL);
Why even declare the boolean here, just use the expression in the assignment on
the next line.

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (left):

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:508:
IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, VerifySrcdoc) {
Let's not remove this test please. It does verify something that still exists
and is testable. The only thing that needs to be removed is the is_srcdoc()
check, but should be replaced with a check that the URL committed is
about:srcdoc. Also the observer will have to ensure it is waiting for srcdoc
navigation.

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:151: if (validated_url !=
GURL(content::kAboutSrcDocURL))
This doesn't strike me as a good practice. We don't want to special case srcdoc
everywhere we call FilterURL. Why not update FilterURL with knowledge about
srcdoc?

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1199: if (*it !=
GURL(content::kAboutSrcDocURL))
All of these checks will be unnecessary if we update FilterURL.

[commit-bot: I haz the power, 2016-11-10 18:32:44]

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

[commit-bot: I haz the power, 2016-11-10 18:32:45]

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

[Charlie Reis, 2016-11-11 22:53:25]

This is great!  I agree about updating FilterURL itself, and otherwise I'm very
excited for this to land.

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:879: // Be careful not to
include iframe srcdoc URLs in this check, which do
On 2016/11/10 18:14:14, nasko wrote:
> I really like this cleanup!

+1!

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:151: if (validated_url !=
GURL(content::kAboutSrcDocURL))
On 2016/11/10 18:14:14, nasko wrote:
> This doesn't strike me as a good practice. We don't want to special case
srcdoc
> everywhere we call FilterURL. Why not update FilterURL with knowledge about
> srcdoc?

Agreed.

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1409: if
(source_instance && (dest_url == about_srcdoc || dest_url == about_blank ||
Good catch-- that seems right to me.  (I suppose there's a risk of regressing
other similar logic that we don't remember to fix, but maybe that's ok.)

[arthursonzogni, 2016-11-14 10:12:55]

Patchset #2 (id:20001) has been deleted

[arthursonzogni, 2016-11-14 16:23:34]

Patchset #2 (id:40001) has been deleted

[arthursonzogni, 2016-11-14 16:23:45]

Patchset #2 (id:60001) has been deleted

[arthursonzogni, 2016-11-14 16:26:27]

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

[commit-bot: I haz the power, 2016-11-14 16:26:44]

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-14 18:18:05]

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

[commit-bot: I haz the power, 2016-11-14 18:18:06]

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

[arthursonzogni, 2016-11-15 10:04:13]

Patchset #2 (id:80001) has been deleted

[arthursonzogni, 2016-11-15 10:04:29]

Patchset #2 (id:100001) has been deleted

[arthursonzogni, 2016-11-15 10:05:00]

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

[commit-bot: I haz the power, 2016-11-15 10:05:13]

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-15 10:18:12]

The CQ bit was unchecked by arthursonzogni@chromium.org

[arthursonzogni, 2016-11-15 10:41:07]

Patchset #3 (id:140001) has been deleted

[arthursonzogni, 2016-11-15 10:41:13]

Patchset #2 (id:120001) has been deleted

[arthursonzogni, 2016-11-15 10:45:29]

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

[commit-bot: I haz the power, 2016-11-15 10:45:41]

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-15 10:48:33]

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

[commit-bot: I haz the power, 2016-11-15 10:48:35]

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_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[arthursonzogni, 2016-11-15 12:40:24]

Patchset #2 (id:160001) has been deleted

[arthursonzogni, 2016-11-15 12:40:32]

Patchset #2 (id:180001) has been deleted

[arthursonzogni, 2016-11-15 12:42:15]

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

[commit-bot: I haz the power, 2016-11-15 12:42:31]

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-15 13:59:01]

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

[commit-bot: I haz the power, 2016-11-15 13:59:02]

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

[arthursonzogni, 2016-11-15 14:24:42]

Patchset #2 (id:200001) has been deleted

[arthursonzogni, 2016-11-15 14:24:55]

Patchset #2 (id:220001) has been deleted

[arthursonzogni, 2016-11-15 14:31:44]

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

[commit-bot: I haz the power, 2016-11-15 14:32:04]

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-15 16:22:58]

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

[commit-bot: I haz the power, 2016-11-15 16:23:00]

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

[arthursonzogni, 2016-11-15 16:37:09]

Thanks for your suggestions.
Some answers/questions below:

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:878: bool is_about_blank =
(child->frame_entry->url() == url::kAboutBlankURL);
On 2016/11/10 18:14:14, nasko (out until Nov 17) wrote:
> Why even declare the boolean here, just use the expression in the assignment
on
> the next line.

Done.

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (left):

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:508:
IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, VerifySrcdoc) {
On 2016/11/10 18:14:14, nasko (out until Nov 17) wrote:
> Let's not remove this test please. It does verify something that still exists
> and is testable. The only thing that needs to be removed is the is_srcdoc()
> check, but should be replaced with a check that the URL committed is
> about:srcdoc. Also the observer will have to ensure it is waiting for srcdoc
> navigation.

Done.

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:151: if (validated_url !=
GURL(content::kAboutSrcDocURL))
On 2016/11/11 22:53:24, Charlie Reis (OOO til 11-11) wrote:
> On 2016/11/10 18:14:14, nasko wrote:
> > This doesn't strike me as a good practice. We don't want to special case
> srcdoc
> > everywhere we call FilterURL. Why not update FilterURL with knowledge about
> > srcdoc?
> 
> Agreed.

I agree, but do you think it is careful to modify the FilterURL function? It is
also used in a lot of different places (28 occurences). I only have identified 3
places where the about:srcdoc needs to be kept.
I will make the change and check the test results of the bots.

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1409: if
(source_instance && (dest_url == about_srcdoc || dest_url == about_blank ||
On 2016/11/11 22:53:24, Charlie Reis (OOO til 11-11) wrote:
> Good catch-- that seems right to me.  (I suppose there's a risk of regressing
> other similar logic that we don't remember to fix, but maybe that's ok.)

FYI, this modification fixes the test:
FrameTreeBrowserTest.ChildFrameWithSrcdoc
with the --site-per-process flag.

I hope there won't be serious regression.
I tried to find other place where the same kind of modifications may apply, but
since there is no failing tests, it is harder to understand what should be done.

Probably here:
https://cs.chromium.org/chromium/src/chrome/browser/history/history_utils.cc?...

Maybe not here (it is on the renderer side):
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?cl...
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?cl...

https://codereview.chromium.org/2494633004/diff/260001/testing/buildbot/filte...
File
testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
(right):

https://codereview.chromium.org/2494633004/diff/260001/testing/buildbot/filte...
testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter:1:
-NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry
It was an error I made while fixing a rebase conflict before I published the
patch #1.

[arthursonzogni, 2016-11-15 16:43:51]

I didn't enter the full email address of clamy and alexmos the first time, only
username.
+CC clamy, alexmos: FYI.

[nasko, 2016-11-15 17:15:23]

LGTM once the comments are fixed.

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (left):

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:518:
EXPECT_TRUE(observer.is_srcdoc());
Can we readd this back and check that the observed URL is indeed kAboutSrcDoc?

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:151: if (validated_url !=
GURL(content::kAboutSrcDocURL))
Remove this check as FilterURL now handles it correctly.

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:24: #include
"content/public/common/url_constants.h"
This won't be needed once the check below is removed.

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:250: if (validated_url !=
GURL(content::kAboutSrcDocURL))
Remove this check too.

https://codereview.chromium.org/2494633004/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2360: // An exception
is made for about:srddoc.
nit: s/srddoc/srcdoc/

https://codereview.chromium.org/2494633004/diff/260001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/260001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3564: if (validated_url !=
GURL(content::kAboutSrcDocURL))
Remove check.

[arthursonzogni, 2016-11-16 09:53:38]

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

[commit-bot: I haz the power, 2016-11-16 09:54:00]

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-16 09:56:40]

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

[commit-bot: I haz the power, 2016-11-16 09:56:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[arthursonzogni, 2016-11-16 09:59:59]

The CQ bit was unchecked by arthursonzogni@chromium.org

[arthursonzogni, 2016-11-16 10:20:38]

Patchset #4 (id:280001) has been deleted

[arthursonzogni, 2016-11-16 10:23:21]

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

[commit-bot: I haz the power, 2016-11-16 10:23:51]

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-16 12:47:54]

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

[commit-bot: I haz the power, 2016-11-16 12:47:55]

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

[Charlie Reis, 2016-11-16 18:20:22]

Thanks!  LGTM with nits if we move the check from FilterURL to
ChildPolicySecurityPolicyImpl::CanRequestURL, given Nick's observation.

Also, let's plan to land this just after branch cut, just in case it does lead
to some unexpected behavior for checks we didn't consider.

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:151: if (validated_url !=
GURL(content::kAboutSrcDocURL))
On 2016/11/15 16:37:09, arthursonzogni wrote:
> On 2016/11/11 22:53:24, Charlie Reis (OOO til 11-11) wrote:
> > On 2016/11/10 18:14:14, nasko wrote:
> > > This doesn't strike me as a good practice. We don't want to special case
> > srcdoc
> > > everywhere we call FilterURL. Why not update FilterURL with knowledge
about
> > > srcdoc?
> > 
> > Agreed.
> 
> I agree, but do you think it is careful to modify the FilterURL function? It
is
> also used in a lot of different places (28 occurences). I only have identified
3
> places where the about:srcdoc needs to be kept.
> I will make the change and check the test results of the bots.

Yes, I think it's fine to allow about:srcdoc through in all cases.  I don't
think we were thinking about that case when canonicalizing about:anything to
about:blank.

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

https://codereview.chromium.org/2494633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1409: if
(source_instance && (dest_url == about_srcdoc || dest_url == about_blank ||
On 2016/11/15 16:37:09, arthursonzogni wrote:
> On 2016/11/11 22:53:24, Charlie Reis (OOO til 11-11) wrote:
> > Good catch-- that seems right to me.  (I suppose there's a risk of
regressing
> > other similar logic that we don't remember to fix, but maybe that's ok.)
> 
> FYI, this modification fixes the test:
> FrameTreeBrowserTest.ChildFrameWithSrcdoc
> with the --site-per-process flag.
> 
> I hope there won't be serious regression.
> I tried to find other place where the same kind of modifications may apply,
but
> since there is no failing tests, it is harder to understand what should be
done.
> 
> Probably here:
>
https://cs.chromium.org/chromium/src/chrome/browser/history/history_utils.cc?...

I'm not sure if that ever gets called for subframes.  I suppose it wouldn't hurt
to add it, though, just in case.

> Maybe not here (it is on the renderer side):
>
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?cl...
>
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?cl...

Agreed that it shouldn't matter on the renderer side, since it never rewrote
about:srcdoc to about:blank in the first place (though apparently the code has
changed and those links don't appear to work).

Besides those, there's a chance that it's relevant to
SiteInstanceImpl::IsSameWebSite, but I think the change you're making here is
actually the right way to handle that one.

https://codereview.chromium.org/2494633004/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2363: return;
Was this return added for ChildPolicySecurityPolicyImpl::CanRequestURL rejecting
about:srcdoc?  I think that brings up a good point.

First, we should update CanRequestURL to allow about:srcdoc the same way it
allows about:blank.  Otherwise its other callers might get the wrong answer
(e.g., if the srcdoc page tries to make an XmlHttpRequest).

Second, Nick points out that such a change would make this whole block
unnecessary.  CanRequestURL will return true for about:blank and about:srcdoc,
and false for all other about: URLs, causing us to rewrite them to about:blank
on line 2371 below.  That seems like a nice cleanup to me, keeping the logic in
fewer places.

https://codereview.chromium.org/2494633004/diff/320001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2494633004/diff/320001/content/public/browser...
content/public/browser/web_contents_observer.h:203: // Since the URL validation
will strip error URLs, the boolean flags
nit: flag

[arthursonzogni, 2016-11-17 15:22:10]

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

[commit-bot: I haz the power, 2016-11-17 15:22:24]

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-17 16:12:17]

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

[commit-bot: I haz the power, 2016-11-17 16:12:19]

Dry run: 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...)

[arthursonzogni, 2016-11-17 17:04:59]

Thanks nasko@ and creis@!

Yes it will be better to land this after a branch cut.

Several answers/questions below:

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (left):

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:518:
EXPECT_TRUE(observer.is_srcdoc());
On 2016/11/15 17:15:23, nasko (out until Nov 17) wrote:
> Can we readd this back and check that the observed URL is indeed kAboutSrcDoc?

Yes, good idea.

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:151: if (validated_url !=
GURL(content::kAboutSrcDocURL))
On 2016/11/15 17:15:23, nasko (out until Nov 17) wrote:
> Remove this check as FilterURL now handles it correctly.

Done.

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2494633004/diff/260001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:24: #include
"content/public/common/url_constants.h"
On 2016/11/15 17:15:23, nasko (out until Nov 17) wrote:
> This won't be needed once the check below is removed.

Done.

https://codereview.chromium.org/2494633004/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2360: // An exception
is made for about:srddoc.
On 2016/11/15 17:15:23, nasko (out until Nov 17) wrote:
> nit: s/srddoc/srcdoc/

Done.

https://codereview.chromium.org/2494633004/diff/260001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/260001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3564: if (validated_url !=
GURL(content::kAboutSrcDocURL))
On 2016/11/15 17:15:23, nasko (out until Nov 17) wrote:
> Remove check.

Done.

https://codereview.chromium.org/2494633004/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2363: return;
On 2016/11/16 18:20:22, Charlie Reis (slow) wrote:
> Was this return added for ChildPolicySecurityPolicyImpl::CanRequestURL
rejecting
> about:srcdoc?  I think that brings up a good point.
> 
> First, we should update CanRequestURL to allow about:srcdoc the same way it
> allows about:blank.  Otherwise its other callers might get the wrong answer
> (e.g., if the srcdoc page tries to make an XmlHttpRequest).
> 
> Second, Nick points out that such a change would make this whole block
> unnecessary.  CanRequestURL will return true for about:blank and about:srcdoc,
> and false for all other about: URLs, causing us to rewrite them to about:blank
> on line 2371 below.  That seems like a nice cleanup to me, keeping the logic
in
> fewer places.

Yes it was.
To be honest, I initially tried what you suggest when I saw that the "return"
was needed here.
The problem is that we can't just remove this block of code because comparison
to about:blank made in CanRequestURL is not case-sensitive. Mixing not
case-sensitive and case-sensitive test are not a good idea. Comparison to
about:blank are case-sensitive everywhere except here.

For instance, if the user click on <a href="about:BLANK"></a>, it will cause a
browser crash because of this inconsistency. 

FYI the specification doesn't says that the about:xxxx URL are not
case-sensitive. So:
GURL("about:blank") != GURL(about:BLANK)
even if hostname are not case-sensitive.
GURL("www.chromium.org/xX") == GURL("www.CHROMIUM.org/xX")

I think we could make CanRequestURL and CanCommitURL case-sensitive. What do you
think?

https://codereview.chromium.org/2494633004/diff/320001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2494633004/diff/320001/content/public/browser...
content/public/browser/web_contents_observer.h:203: // Since the URL validation
will strip error URLs, the boolean flags
On 2016/11/16 18:20:22, Charlie Reis (slow) wrote:
> nit: flag

Done.

https://codereview.chromium.org/2494633004/diff/340001/chrome/browser/history...
File chrome/browser/history/history_utils.cc (right):

https://codereview.chromium.org/2494633004/diff/340001/chrome/browser/history...
chrome/browser/history/history_utils.cc:20: url.SchemeIs(url::kAboutScheme) ||
FYI: from what I understood, about:xxxx URLs are rewritten to chrome:xxxx when
they are typed by the user and rewritten to about:blank/about:srcdoc when they
are coming from a renderer.

[arthursonzogni, 2016-11-18 09:13:16]

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

[commit-bot: I haz the power, 2016-11-18 09:13:34]

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-18 12:27:45]

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

[commit-bot: I haz the power, 2016-11-18 12:27:47]

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

[Charlie Reis, 2016-11-22 01:01:20]

Apologies for the long delay!  The end of last week got a bit crazy on my side.

I like the change to CanRequestURL (assuming the renderer agrees that all
about:BLaNk combinations really commit about:blank).  It looks like the
SiteInstance change is a bug, though-- might be worth confirming.  Thanks!

https://codereview.chromium.org/2494633004/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2363: return;
On 2016/11/17 17:04:58, arthursonzogni wrote:
> On 2016/11/16 18:20:22, Charlie Reis (slow) wrote:
> > Was this return added for ChildPolicySecurityPolicyImpl::CanRequestURL
> rejecting
> > about:srcdoc?  I think that brings up a good point.
> > 
> > First, we should update CanRequestURL to allow about:srcdoc the same way it
> > allows about:blank.  Otherwise its other callers might get the wrong answer
> > (e.g., if the srcdoc page tries to make an XmlHttpRequest).
> > 
> > Second, Nick points out that such a change would make this whole block
> > unnecessary.  CanRequestURL will return true for about:blank and
about:srcdoc,
> > and false for all other about: URLs, causing us to rewrite them to
about:blank
> > on line 2371 below.  That seems like a nice cleanup to me, keeping the logic
> in
> > fewer places.
> 
> Yes it was.
> To be honest, I initially tried what you suggest when I saw that the "return"
> was needed here.
> The problem is that we can't just remove this block of code because comparison
> to about:blank made in CanRequestURL is not case-sensitive. Mixing not
> case-sensitive and case-sensitive test are not a good idea. Comparison to
> about:blank are case-sensitive everywhere except here.
> 
> For instance, if the user click on <a href="about:BLANK"></a>, it will cause a
> browser crash because of this inconsistency. 
> 
> FYI the specification doesn't says that the about:xxxx URL are not
> case-sensitive. So:
> GURL("about:blank") != GURL(about:BLANK)
> even if hostname are not case-sensitive.
> GURL("www.chromium.org/xX") == GURL("www.CHROMIUM.org/xX")
> 
> I think we could make CanRequestURL and CanCommitURL case-sensitive. What do
you
> think?

Yes, I think that makes sense, assuming we don't expect the renderer to allow an
about:BlAnK commit, etc.  That would make it consistent with all the other
checks.

https://codereview.chromium.org/2494633004/diff/360001/content/browser/child_...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/2494633004/diff/360001/content/browser/child_...
content/browser/child_process_security_policy_unittest.cc:264:
EXPECT_FALSE(p->CanRequestURL(kRendererID, GURL("about:BlAnK")));
Can you confirm that the renderer doesn't try to commit URLs like this if we ask
it to (e.g., in a link click)?  I just want to be sure the browser and renderer
agree on what URL we're on.

https://codereview.chromium.org/2494633004/diff/360001/content/browser/site_i...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/360001/content/browser/site_i...
content/browser/site_instance_impl.cc:321: if (dest_url == url::kAboutBlankURL
|| dest_url == content::kAboutSrcDocURL)
Let's leave this out.  As I mentioned in a previous patchset, I think we're
already doing the right thing by letting RFHM handle it, and I worry that this
could introduce a problem.  (For example, if a parent frame tells an OOPIF to go
to a srcdoc, it should join the parent's process and not stay in whatever
process it was in before.)

https://codereview.chromium.org/2494633004/diff/360001/content/browser/site_i...
File content/browser/site_instance_impl_unittest.cc (right):

https://codereview.chromium.org/2494633004/diff/360001/content/browser/site_i...
content/browser/site_instance_impl_unittest.cc:415: // Navigating to a blank
page or about:srcdoc is considered the same site.
As mentioned earlier, let's leave this out, or maybe explicitly test that it's
not treated as the same.  (Even better would be to test what happens in the
example I mentioned, though that could be in a separate CL if needed.)

https://codereview.chromium.org/2494633004/diff/360001/content/public/browser...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/2494633004/diff/360001/content/public/browser...
content/public/browser/site_instance.h:162: // Note that if the destination is
about:blank or about:srcdoc, we consider
Let's leave this out.

[arthursonzogni, 2016-11-22 14:32:08]

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

[commit-bot: I haz the power, 2016-11-22 14:32:21]

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-22 16:43:27]

Thanks for your review Charlie!

* I removed the modification in IsSameWebSite.
* I made the test you suggested in a separate CL.
* There is still the question related to the case-sensitivity of about:blank. I
don't know what to do about it. We can still undo the modification on
CanRequestURL, but I am not sure if it is better.

https://codereview.chromium.org/2494633004/diff/360001/content/browser/child_...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/2494633004/diff/360001/content/browser/child_...
content/browser/child_process_security_policy_unittest.cc:264:
EXPECT_FALSE(p->CanRequestURL(kRendererID, GURL("about:BlAnK")));
On 2016/11/22 01:01:20, Charlie Reis (slow) wrote:
> Can you confirm that the renderer doesn't try to commit URLs like this if we
ask
> it to (e.g., in a link click)?  I just want to be sure the browser and
renderer
> agree on what URL we're on.

The answer is 'no' without PlzNavigate and 'yes' with it.

# With PlzNavigate

   Renderer                          Browser
      |   ------(BeginNavigation)-----> |  <- the URL is filtered here.
      | <------(CommitNavigation)----   |
      |   ----(DidCommitNavigation)---> |

about:blank is committed in the renderer and the browser agree on this.
                             
# Without Plznavigate

   Renderer                          Browser
      |   ----(DidCommitNavigation)---> |  <-the URL is filtered here.

about:BlAnK is committed on the renderer and the browser thinks it is
about:blank.


Nevertheless, because the behavior of filterURL is still the same (only
CanRequestURL has changed), if this is a problem, then the problem was already
present before this patch.

https://codereview.chromium.org/2494633004/diff/360001/content/browser/site_i...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/360001/content/browser/site_i...
content/browser/site_instance_impl.cc:321: if (dest_url == url::kAboutBlankURL
|| dest_url == content::kAboutSrcDocURL)
On 2016/11/22 01:01:20, Charlie Reis (slow) wrote:
> Let's leave this out.  As I mentioned in a previous patchset, I think we're
> already doing the right thing by letting RFHM handle it, and I worry that this
> could introduce a problem.  (For example, if a parent frame tells an OOPIF to
go
> to a srcdoc, it should join the parent's process and not stay in whatever
> process it was in before.)

I add a test in a separate CL:
https://codereview.chromium.org/2525433003/

FYI: the test passes with this CL. But I will still remove this part in case
there are other problems.

https://codereview.chromium.org/2494633004/diff/360001/content/browser/site_i...
File content/browser/site_instance_impl_unittest.cc (right):

https://codereview.chromium.org/2494633004/diff/360001/content/browser/site_i...
content/browser/site_instance_impl_unittest.cc:415: // Navigating to a blank
page or about:srcdoc is considered the same site.
On 2016/11/22 01:01:20, Charlie Reis (slow) wrote:
> As mentioned earlier, let's leave this out, or maybe explicitly test that it's
> not treated as the same.  (Even better would be to test what happens in the
> example I mentioned, though that could be in a separate CL if needed.)

Done (see above).

https://codereview.chromium.org/2494633004/diff/360001/content/public/browser...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/2494633004/diff/360001/content/public/browser...
content/public/browser/site_instance.h:162: // Note that if the destination is
about:blank or about:srcdoc, we consider
On 2016/11/22 01:01:20, Charlie Reis (slow) wrote:
> Let's leave this out.

Done.

[arthursonzogni, 2016-11-22 16:47:55]

Patchset #10 (id:420001) has been deleted

[arthursonzogni, 2016-11-22 16:55:02]

Patchset #11 (id:460001) has been deleted

[arthursonzogni, 2016-11-22 16:55:18]

Patchset #10 (id:440001) has been deleted

[Charlie Reis, 2016-11-23 08:25:21]

Thanks for the analysis!  LGTM with a few sanity check questions below.

https://codereview.chromium.org/2494633004/diff/360001/content/browser/child_...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/2494633004/diff/360001/content/browser/child_...
content/browser/child_process_security_policy_unittest.cc:264:
EXPECT_FALSE(p->CanRequestURL(kRendererID, GURL("about:BlAnK")));
On 2016/11/22 16:43:27, arthursonzogni wrote:
> On 2016/11/22 01:01:20, Charlie Reis (slow) wrote:
> > Can you confirm that the renderer doesn't try to commit URLs like this if we
> ask
> > it to (e.g., in a link click)?  I just want to be sure the browser and
> renderer
> > agree on what URL we're on.
> 
> The answer is 'no' without PlzNavigate and 'yes' with it.
> 
> # With PlzNavigate
> 
>    Renderer                          Browser
>       |   ------(BeginNavigation)-----> |  <- the URL is filtered here.
>       | <------(CommitNavigation)----   |
>       |   ----(DidCommitNavigation)---> |
> 
> about:blank is committed in the renderer and the browser agree on this.
>                              
> # Without Plznavigate
> 
>    Renderer                          Browser
>       |   ----(DidCommitNavigation)---> |  <-the URL is filtered here.
> 
> about:BlAnK is committed on the renderer and the browser thinks it is
> about:blank.
> 
> 
> Nevertheless, because the behavior of filterURL is still the same (only
> CanRequestURL has changed), if this is a problem, then the problem was already
> present before this patch.

Thanks for checking.  It does seem unfortunate that the renderer and browser
disagree, but maybe you're right that it isn't a problem.

The one case I'm mildly concerned about is the CanSetAsOriginHeader call in
ResourceDispatcherHostImpl::ShouldServiceRequest, which (I think) operates on an
unfiltered URL from the renderer.  In theory, an about:BlAnK page committed in a
renderer would fail that check if it made an XmlHttpRequest, since CanCommitURL
is now case-sensitive and says it couldn't have committed it.  But then again,
an about:BlAnK URL would set null as its Origin header, not about:BlAnK, so we
should be fine.

There's a chance some other path will get an unfiltered URL into CanCommitURL or
CanRequestURL, but I suppose we can track that down if it happens.  Let's run
with it.

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
content/browser/child_process_security_policy_impl.cc:629: // Every child
process can request <about:blank> and <about:srcdoc>
nit: End with period.

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
content/browser/child_process_security_policy_impl.cc:667: // Of all the pseudo
schemes, only about:blank is allowed to commit.
Do we need to allow about:srcdoc here, or will that never get this far?

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
content/browser/child_process_security_policy_unittest.cc:274:
EXPECT_TRUE(p->CanSetAsOriginHeader(kRendererID, GURL("aBouT:blank")));
Can you add some cases for about:srcdoc?

[arthursonzogni, 2016-11-23 14:39:27]

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
content/browser/child_process_security_policy_impl.cc:629: // Every child
process can request <about:blank> and <about:srcdoc>
On 2016/11/23 08:25:21, Charlie Reis (slow) wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
content/browser/child_process_security_policy_impl.cc:667: // Of all the pseudo
schemes, only about:blank is allowed to commit.
On 2016/11/23 08:25:20, Charlie Reis (slow) wrote:
> Do we need to allow about:srcdoc here, or will that never get this far?

Good question!

CanCommitURL is called inside several function in 
* /content/browser/child_process_security_policy_impl.cc
* /content/browser/blob_storage/blob_dispatcher_host.cc

Where it can't be called with <about:srcdoc>:
* In CanRequestURL: Never called because of the early return.
* In CanCommitURL: Called only with blob: and file: scheme.
* In HasPermissionsForFileSystemFile: called only with file: scheme.
* In OnRegisterPublicBlobURL: called only with blob: scheme.

Where it is called with <about:srcdoc>
* In CanSetAsOriginHeader -> I think the correct behaviour is that it
  must returns false. It is the case if CanCommitURL(<about:srcdoc>)
  returns false too.

Thus, if we don't touch anything, the global behavior is preserved
(and is correct according to me). The only problem is that the meaning
of CanCommitURL is not respected because <about:srcdoc> can actually be
committed. Do you think CanCommitURL(<about:srcdoc>) should be true?

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
content/browser/child_process_security_policy_unittest.cc:274:
EXPECT_TRUE(p->CanSetAsOriginHeader(kRendererID, GURL("aBouT:blank")));
On 2016/11/23 08:25:21, Charlie Reis (slow) wrote:
> Can you add some cases for about:srcdoc?

Done.

[Charlie Reis, 2016-11-23 18:03:37]

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
content/browser/child_process_security_policy_impl.cc:667: // Of all the pseudo
schemes, only about:blank is allowed to commit.
On 2016/11/23 14:39:26, arthursonzogni wrote:
> On 2016/11/23 08:25:20, Charlie Reis (slow) wrote:
> > Do we need to allow about:srcdoc here, or will that never get this far?
> 
> Good question!
> 
> CanCommitURL is called inside several function in 
> * /content/browser/child_process_security_policy_impl.cc
> * /content/browser/blob_storage/blob_dispatcher_host.cc
> 
> Where it can't be called with <about:srcdoc>:
> * In CanRequestURL: Never called because of the early return.
> * In CanCommitURL: Called only with blob: and file: scheme.
> * In HasPermissionsForFileSystemFile: called only with file: scheme.
> * In OnRegisterPublicBlobURL: called only with blob: scheme.
> 
> Where it is called with <about:srcdoc>
> * In CanSetAsOriginHeader -> I think the correct behaviour is that it
>   must returns false. It is the case if CanCommitURL(<about:srcdoc>)
>   returns false too.
> 
> Thus, if we don't touch anything, the global behavior is preserved
> (and is correct according to me). The only problem is that the meaning
> of CanCommitURL is not respected because <about:srcdoc> can actually be
> committed. Do you think CanCommitURL(<about:srcdoc>) should be true?

Glad to hear.

In the abstract, yes, about:srcdoc can commit, so it's a little odd for this to
return false in that case.  Sounds like that would only matter in future
possible uses for CanCommitURL, though, and we have test coverage that should
catch the problem if it does start to matter.  Thus, I'm ok either way for this
CL.

[Charlie Reis, 2016-11-24 00:17:00]

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
content/browser/child_process_security_policy_impl.cc:667: // Of all the pseudo
schemes, only about:blank is allowed to commit.
On 2016/11/23 18:03:37, Charlie Reis (slow) wrote:
> On 2016/11/23 14:39:26, arthursonzogni wrote:
> > On 2016/11/23 08:25:20, Charlie Reis (slow) wrote:
> > > Do we need to allow about:srcdoc here, or will that never get this far?
> > 
> > Good question!
> > 
> > CanCommitURL is called inside several function in 
> > * /content/browser/child_process_security_policy_impl.cc
> > * /content/browser/blob_storage/blob_dispatcher_host.cc
> > 
> > Where it can't be called with <about:srcdoc>:
> > * In CanRequestURL: Never called because of the early return.
> > * In CanCommitURL: Called only with blob: and file: scheme.
> > * In HasPermissionsForFileSystemFile: called only with file: scheme.
> > * In OnRegisterPublicBlobURL: called only with blob: scheme.
> > 
> > Where it is called with <about:srcdoc>
> > * In CanSetAsOriginHeader -> I think the correct behaviour is that it
> >   must returns false. It is the case if CanCommitURL(<about:srcdoc>)
> >   returns false too.
> > 
> > Thus, if we don't touch anything, the global behavior is preserved
> > (and is correct according to me). The only problem is that the meaning
> > of CanCommitURL is not respected because <about:srcdoc> can actually be
> > committed. Do you think CanCommitURL(<about:srcdoc>) should be true?
> 
> Glad to hear.
> 
> In the abstract, yes, about:srcdoc can commit, so it's a little odd for this
to
> return false in that case.  Sounds like that would only matter in future
> possible uses for CanCommitURL, though, and we have test coverage that should
> catch the problem if it does start to matter.  Thus, I'm ok either way for
this
> CL.

Nick brings up the point that it's probably saner for CanCommitURL to return
true for about:srcdoc if we can, even if it happens to work in most cases.

[arthursonzogni, 2016-11-24 09:51:12]

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

[commit-bot: I haz the power, 2016-11-24 09:51:28]

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-24 10:24:13]

The CQ bit was unchecked by arthursonzogni@chromium.org

[arthursonzogni, 2016-11-24 10:24:44]

Patchset #11 (id:500001) has been deleted

[arthursonzogni, 2016-11-24 10:36:53]

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

[commit-bot: I haz the power, 2016-11-24 10:37:17]

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-24 12:38:25]

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

[commit-bot: I haz the power, 2016-11-24 12:38:27]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[arthursonzogni, 2016-11-24 12:51:30]

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

[commit-bot: I haz the power, 2016-11-24 12:51:52]

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-24 14:42:04]

On 2016/11/24 00:17:00, Charlie Reis (slow) wrote:
>
https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
> File content/browser/child_process_security_policy_impl.cc (right):
> 
>
https://codereview.chromium.org/2494633004/diff/400001/content/browser/child_...
> content/browser/child_process_security_policy_impl.cc:667: // Of all the
pseudo
> schemes, only about:blank is allowed to commit.
> On 2016/11/23 18:03:37, Charlie Reis (slow) wrote:
> > On 2016/11/23 14:39:26, arthursonzogni wrote:
> > > On 2016/11/23 08:25:20, Charlie Reis (slow) wrote:
> > > > Do we need to allow about:srcdoc here, or will that never get this far?
> > > 
> > > Good question!
> > > 
> > > CanCommitURL is called inside several function in 
> > > * /content/browser/child_process_security_policy_impl.cc
> > > * /content/browser/blob_storage/blob_dispatcher_host.cc
> > > 
> > > Where it can't be called with <about:srcdoc>:
> > > * In CanRequestURL: Never called because of the early return.
> > > * In CanCommitURL: Called only with blob: and file: scheme.
> > > * In HasPermissionsForFileSystemFile: called only with file: scheme.
> > > * In OnRegisterPublicBlobURL: called only with blob: scheme.
> > > 
> > > Where it is called with <about:srcdoc>
> > > * In CanSetAsOriginHeader -> I think the correct behaviour is that it
> > >   must returns false. It is the case if CanCommitURL(<about:srcdoc>)
> > >   returns false too.
> > > 
> > > Thus, if we don't touch anything, the global behavior is preserved
> > > (and is correct according to me). The only problem is that the meaning
> > > of CanCommitURL is not respected because <about:srcdoc> can actually be
> > > committed. Do you think CanCommitURL(<about:srcdoc>) should be true?
> > 
> > Glad to hear.
> > 
> > In the abstract, yes, about:srcdoc can commit, so it's a little odd for this
> to
> > return false in that case.  Sounds like that would only matter in future
> > possible uses for CanCommitURL, though, and we have test coverage that
should
> > catch the problem if it does start to matter.  Thus, I'm ok either way for
> this
> > CL.
> 
> Nick brings up the point that it's probably saner for CanCommitURL to return
> true for about:srcdoc if we can, even if it happens to work in most cases.

> Nick brings up the point that it's probably saner for CanCommitURL to return
> true for about:srcdoc if we can, even if it happens to work in most cases.

Okay, it is probably the best thing to do.

Thanks Charlie and Nasko for your review. I will now ask someone else to
review the remaining files that are more trivial.

[commit-bot: I haz the power, 2016-11-24 16:26:14]

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

[commit-bot: I haz the power, 2016-11-24 16:26:16]

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

[arthursonzogni, 2016-11-24 17:32:34]

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

[arthursonzogni, 2016-11-24 17:32:35]

Hi @jochen, @paulmeyer and @dbeam,

I need a review for a bunch of files. The main topic has already been reviewed,
but there is still some files without an owner approval. The main remaining
thing to check is an unused argument that has been removed from a virtual
method. That should be easy.

@jochen
  chrome/android/*           (3 files)
  chrome/browser/history/*   (1 file )
  chrome/browser/prerender/* (3 files)
  chrome/browser/tracing/*   (2 files)
  components/dom_distiller/* (2 files)

@paulmeyer
  extensions/*               (1 file )

@dbeam
  chrome/browser/ui/*        (4 files)

Thanks!

[jochen (gone - plz use gerrit), 2016-11-25 06:43:58]

lgtm

[Charlie Reis, 2016-11-29 17:37:56]

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

[Charlie Reis, 2016-11-29 17:37:58]

paulmeyer@ is OOO.  lfg@, can you review extensions/?

dbeam@: Friendly ping for chrome/browser/ui/.

[lfg, 2016-11-29 19:06:17]

guest_view lgtm

[Dan Beam, 2016-11-29 19:34:13]

lgtm

[Charlie Reis, 2016-11-29 19:42:30]

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

[commit-bot: I haz the power, 2016-11-29 19:42:54]

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-11-29 19:45:25]

Thanks!  Arthur, I've started a CQ dry run in case you think it's ready to land
today.

I did notice that the linux_chromium_browser_side_navigation_rel bot was red
from an earlier job, though.  You may want to check if that's a real failure:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

[commit-bot: I haz the power, 2016-11-30 00:34:16]

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

[commit-bot: I haz the power, 2016-11-30 00:34:18]

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

[arthursonzogni, 2016-11-30 12:52:30]

Description was changed from

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

Before this CL, there was an asymmetry 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 URLs 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,linux_chromium_browser_side_navigation_rel

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

to

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

Before this CL, there was an asymmetry 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 URLs 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 13:01:02]

On 2016/11/29 19:45:25, Charlie Reis (slow) wrote:
> Thanks!  Arthur, I've started a CQ dry run in case you think it's ready to
land
> today.
> 
> I did notice that the linux_chromium_browser_side_navigation_rel bot was red
> from an earlier job, though.  You may want to check if that's a real failure:
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

@creis, thanks! Indeed, the failures reported by
linux_chromium_browser_side_navigation_rel are not caused by my patch.
This bot has been red for several days and it will be green again in some time.

Thanks everyone for the reviews!

[arthursonzogni, 2016-11-30 13:01:22]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2016-11-30 13:01:23]

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

[commit-bot: I haz the power, 2016-11-30 13:01:48]

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 14:35:31]

CQ is committing da patch.

Bot data: {"patchset_id": 560001, "attempt_start_ts": 1480510882999050,
"parent_rev": "cc8b1b8e7a79c51c1073c8dd0efc28e9cd6b978c", "commit_rev":
"2dc97110849625ac80084adbc5dfb163ffcf87d4"}

[commit-bot: I haz the power, 2016-11-30 14:36:03]

Description was changed from

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

Before this CL, there was an asymmetry 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 URLs 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.

Before this CL, there was an asymmetry 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 URLs 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
==========

[commit-bot: I haz the power, 2016-11-30 14:36:06]

Committed patchset #13 (id:560001)

[commit-bot: I haz the power, 2016-11-30 14:37:54]

Description was changed from

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

Before this CL, there was an asymmetry 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 URLs 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.

Before this CL, there was an asymmetry 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 URLs 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

Committed: https://crrev.com/041956a261766b15f09b7b7a37026bda1ef809f3
Cr-Commit-Position: refs/heads/master@{#435256}
==========

[commit-bot: I haz the power, 2016-11-30 14:37:57]

Patchset 13 (id:??) landed as
https://crrev.com/041956a261766b15f09b7b7a37026bda1ef809f3
Cr-Commit-Position: refs/heads/master@{#435256}

[battre, 2016-11-30 15:37:47]

A revert of this CL (patchset #13 id:560001) has been created in
https://codereview.chromium.org/2541063002/ by battre@chromium.org.

The reason for reverting is: Speculative revert for http://crbug.com/660061#c2.