I'm not sure if there's something else in the commit parameters indicating the load was unsuccessful.  So far I've only noticed that validated_params.origin is unique, but otherwise it looks a lot like validated_params.url actually committed.  Importantly, validated_params.url_is_unreachable is false, so we treat it as the last successful URL and not an error page.

Alex, can you take a look at what our options are here?  It seems surprising that XFO is allowing the URL to commit.

FWIW, I think relanding https://codereview.chromium.org/1617043002/ will probably fix this, regardless of its current implementation. That's going to be super-tough to merge back, however, so it might be worth finding a smaller, more mergable fix.

If we can detect (in OnDidCommitProvisionalLoad) that it's a blank (error?) page committing and not the actual CWS page, we could skip the renderer kill and just rewrite the URL to about:blank for something to merge to M52.

Is there a way to detect that reliably?

I'm still worried about the implications of committing the URL, such as if it shows up within the PageState stored on the entry.  But hopefully your CL will make this moot.  Remind me, does it prevent the blocked URL from committing?

> If we can detect (in OnDidCommitProvisionalLoad) that it's a blank (error?)
> page committing and not the actual CWS page, we could skip the renderer kill
> and just rewrite the URL to about:blank for something to merge to M52.
>
> Is there a way to detect that reliably?

I don't know off the top of my head. If y'all have time to look at it today, please do. If not, I'll poke at it tomorrow.

> I'm still worried about the implications of committing the URL, such as if
> it shows up within the PageState stored on the entry.  But hopefully your CL will make this moot.  Remind me, does it prevent the blocked URL from committing?

Once we re-land https://codereview.chromium.org/1617043002/, we'll end up committing to an error page, just like we do with blocks via WebRequest, etc.

Yeah, this is likely caused by my https://codereview.chromium.org/1742923002/, where I changed these blocked pages to commit an empty document instead of canceling the load.  The spec says we should pretend that we've received an empty 200 response (or go to an error page), and I guess I didn't realize that leaving the blocked URL as committed URL could have these consequences.

I guess there are a couple of short-term options.  One, we could try to change the committed URL to something else in these cases.  Currently, in cancelLoadAfterXFrameOptionsOrCSPDenied, we update DocumentLoader's response to a blank URL, but not its request, and the latter is where the committed URL comes from.

Two, we could plumb the blocked bit in FrameHostMsg_DidCommitProvisionalLoad_Params; we currently have it in DocumentLoader::wasBlockedAfterXFrameOptionsOrCSP().  

I'll see if I can get one of these working.

Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7: We may want to merge this to M52 if it resolves the RFH_CAN_COMMIT_URL_BLOCKED crashes, since those may be frequent on M52.  We can confirm after https://codereview.chromium.org/2096453002/ lands.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30535f7116c9073705a155c7cf4b0146a28f7293

commit 30535f7116c9073705a155c7cf4b0146a28f7293
Author: alexmos <alexmos@chromium.org>
Date: Thu Jun 23 18:48:23 2016

Don't commit the blocked URL when a frame is blocked by XFrameOptions.

Previously, when a load was blocked by XFO or frame-ancestors, we
committed a blank page and left the original URL as the committed URL.
In some cases, this led to the browser process thinking that the
renderer actually committed a real load for the blocked URL and
killing the renderer if that load was disallowed (e.g., for loading
Chrome Web Store in a frame).

mkwst@ is working on a CL
(https://codereview.chromium.org/1617043002/) that will ultimately fix
this by moving XFO enforcement to the browser process and committing
an error page when a load is blocked.  Until then, this is a
short-term fix to change the committed URL for the blocked (blank)
page to urlWithUniqueSecurityOrigin (data:,).

BUG= 622385 

Review-Url: https://codereview.chromium.org/2096453002
Cr-Commit-Position: refs/heads/master@{#401664}

[modify] https://crrev.com/30535f7116c9073705a155c7cf4b0146a28f7293/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/30535f7116c9073705a155c7cf4b0146a28f7293/third_party/WebKit/Source/core/loader/DocumentLoader.cpp

This should hopefully be fixed in tomorrow's canary.  We can reopen if the crashes persist in 53.0.2778.0 or later.

Note that we'll be looking for crashes with a bad_message_reason crash key of 1 (corresponding to RFH_CAN_COMMIT_URL_BLOCKED).  Here's a query:
https://crash.corp.google.com/browse?q=product.name%3D%27Chrome%27%20AND%20product.version%3E%3D%2753.0.2777.0%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27browser%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27%5BRenderer%20kill%5D%20content%3A%3ARenderFrameHostImpl%3A%3AOnDidCommitProvisionalLoad%27%20OMIT%20RECORD%20IF%20SUM(ProductData.key%3D%27bad_message_reason%27%20AND%20ProductData.value%3D%271%27)%20%3D%200&ignore_case=false&enable_rewrite=false&omit_field_name=&omit_field_value=&omit_field_opt=#samplereports

Hmm.  There's a single report over the last few days: 63685e1600000000 in 53.0.2779.0.  (Note that the link in comment 10 shows more because it included version 53.0.2777.0, which was before the fix landed.)

That's pretty strange to me.  It suggests that there might be a URL within https://chrome.google.com/webstore that doesn't have XFO headers, which I was told shouldn't happen.  With just a single report, it could also be bad data.

If we see more of these over time, we can add a crash key for which URL it was and try to get it fixed.  I think it's probably safe to leave this closed and consider it fixed, though.  (There were 47 such kills in 53.0.2777.0, so we've certainly gotten most of them.)

Alex, do you think this is safe to merge?  I think it's a common kill on M52.

I think it should be safe to merge.  It should hopefully fix the vast majority of these crashes in M52.

Your change meets the bar and is auto-approved for M52 (branch: 2743)

Tested this on Win 7/64 bit - Version 53.0.2782.0 canary (64-bit)
Please find the attached screen shot.

I am seeing the following upon running the navFrame("https://chrome.google.com/webstore") in console:

Refused to display 'https://chrome.google.com/webstore' in a frame because it set 'X-Frame-Options' to 'SAMEORIGIN'.

Is this expected ? Please confirm the behavior.

If the above behavior is correct, please merge the CL in to M52 branch so that it gets picked up for beta promotion scheduled this thursday.

Deleted: 622385 on Win.PNG 40.3 KB

	622385 on Win.PNG 40.3 KB View Download

#14: Yes, that console message is expected behavior, since the CWS page is served with X-Frame-Options.  This also used to crash the renderer, which got fixed by my CL.  I'll go ahead and merge it.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4a487bdca9a78e5fd9c6c149faf45a49c83d6b6

commit d4a487bdca9a78e5fd9c6c149faf45a49c83d6b6
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Tue Jun 28 20:01:26 2016

Don't commit the blocked URL when a frame is blocked by XFrameOptions.

Previously, when a load was blocked by XFO or frame-ancestors, we
committed a blank page and left the original URL as the committed URL.
In some cases, this led to the browser process thinking that the
renderer actually committed a real load for the blocked URL and
killing the renderer if that load was disallowed (e.g., for loading
Chrome Web Store in a frame).

mkwst@ is working on a CL
(https://codereview.chromium.org/1617043002/) that will ultimately fix
this by moving XFO enforcement to the browser process and committing
an error page when a load is blocked.  Until then, this is a
short-term fix to change the committed URL for the blocked (blank)
page to urlWithUniqueSecurityOrigin (data:,).

BUG= 622385 

Review-Url: https://codereview.chromium.org/2096453002
Cr-Commit-Position: refs/heads/master@{#401664}
(cherry picked from commit 30535f7116c9073705a155c7cf4b0146a28f7293)

Review URL: https://codereview.chromium.org/2108873002 .

Cr-Commit-Position: refs/branch-heads/2743@{#509}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/d4a487bdca9a78e5fd9c6c149faf45a49c83d6b6/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/d4a487bdca9a78e5fd9c6c149faf45a49c83d6b6/third_party/WebKit/Source/core/loader/DocumentLoader.cpp

Verified the fix on Windows 7, MAC (10.11.5) & Ubuntu Trusty (14.04) for Google Chrome Beta Version - 52.0.2743.60

Followed the steps in the comment #14

Screen-shot is attached.

TE-Verified labels are attached.

Deleted: Screen Shot 2016-06-30 at 12.17.31 PM.png 95.3 KB

	Screen Shot 2016-06-30 at 12.17.31 PM.png 95.3 KB View Download

Still just the one report mentioned in comment 11, so I think that was probably bad data and we can leave this fixed.  Here's the query I'm using to check:

https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_signature_1.name%3D%27%5BRenderer%20kill%5D%20content%3A%3ARenderFrameHostImpl%3A%3AOnDidCommitProvisionalLoad%27%20AND%20product.Version%3E%3D%2753.0.2778.0%27%20OMIT%20RECORD%20IF%20SUM(ProductData.key%3D%27bad_message_reason%27%20AND%20ProductData.value%3D%271%27)%20%3D%200&ignore_case=false&enable_rewrite=false&omit_field_name=&omit_field_value=&omit_field_opt=%3D&stbtiq=&reportid=&index=0

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5ca4b227e9d0a48b9ff8af941ff69a760305f034

commit 5ca4b227e9d0a48b9ff8af941ff69a760305f034
Author: clamy <clamy@chromium.org>
Date: Wed Feb 22 13:20:05 2017

PlzNavigate: remove NavigatorImpl::FailedNavigation

This CL removes NavigatorImpl::FailedNavigation and move its code into
its sole caller, NavigationRequest::OnRequestFailed. This makes it
easier to update the navigation url if the commit of the error page
would result in a renderer kill (see
https://codereview.chromium.org/2697713005/).

BUG= 622385 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2710643003
Cr-Commit-Position: refs/heads/master@{#452025}

[modify] https://crrev.com/5ca4b227e9d0a48b9ff8af941ff69a760305f034/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/5ca4b227e9d0a48b9ff8af941ff69a760305f034/content/browser/frame_host/navigator.h
[modify] https://crrev.com/5ca4b227e9d0a48b9ff8af941ff69a760305f034/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/5ca4b227e9d0a48b9ff8af941ff69a760305f034/content/browser/frame_host/navigator_impl.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4ba80de54a09324ae69bde773f194df0eec6e8b3

commit 4ba80de54a09324ae69bde773f194df0eec6e8b3
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Fri Feb 24 13:39:37 2017

DCHECK: Browser asking renderer to commit URLs it is not allowed to.

This CL adds a DCHECK when the browser ask the renderer to commit an URL
and then kill the renderer when it does.

This CL was previously related with a problem when the renderer tries to
commit an error page to the chrome webstore inside an iframe. It turns
out that there was no more problems with PlzNavigate since the
ChromeWebStore error page uses a different process.

BUG= 588314 , 622385 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2697713005
Cr-Commit-Position: refs/heads/master@{#452808}

[modify] https://crrev.com/4ba80de54a09324ae69bde773f194df0eec6e8b3/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/4ba80de54a09324ae69bde773f194df0eec6e8b3/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/4ba80de54a09324ae69bde773f194df0eec6e8b3/content/browser/frame_host/render_frame_host_impl.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc3210aac06fd0f9437a66b6d9d784cb80b481d9

commit cc3210aac06fd0f9437a66b6d9d784cb80b481d9
Author: creis <creis@chromium.org>
Date: Tue Mar 21 19:29:41 2017

Don't reset URL for CAN_COMMIT_URL_BLOCKED renderer kill.

This is no longer necessary now that we have an early return.

BUG= 622385 
TEST=Non-blank URL in crash dumps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2758143002
Cr-Commit-Position: refs/heads/master@{#458509}

[modify] https://crrev.com/cc3210aac06fd0f9437a66b6d9d784cb80b481d9/content/browser/frame_host/render_frame_host_impl.cc