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

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
Committed: https://crrev.com/30535f7116c9073705a155c7cf4b0146a28f7293
Cr-Commit-Position: refs/heads/master@{#401664}

[alexmos, 2016-06-23 00:38:32]

Description was changed from

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

BUG=622385
==========

to

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

[alexmos, 2016-06-23 00:46:56]

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

[alexmos, 2016-06-23 00:46:56]

Charlie, Mike - let me know what you think about this potential fix for the CWS
XFO kills.  Basically, this changes the DocumentLoader's request URL (which
becomes the committed URL) to something else when the load is blocked by XFO.  I
used urlWithUniqueSecurityOrigin for the URL, which seemed cleaner/safer than
committing a about:blank URL with a unique origin.  Don't know if doing this has
any other implications; the bots seem happy.  Hopefully this is short-lived
until Mike's CL lands.

[Charlie Reis, 2016-06-23 05:56:45]

I like the idea of committing a unique origin CL instead.  One question below.

https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:367:
updateForSameDocumentNavigation(SecurityOrigin::urlWithUniqueSecurityOrigin(),
SameDocumentNavigationDefault);
Why same document?  Aren't we leaving the current document and replacing it with
an empty document?

[Mike West, 2016-06-23 06:26:40]

https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:367:
updateForSameDocumentNavigation(SecurityOrigin::urlWithUniqueSecurityOrigin(),
SameDocumentNavigationDefault);
On 2016/06/23 at 05:56:45, Charlie Reis wrote:
> Why same document?  Aren't we leaving the current document and replacing it
with an empty document?

We haven't created the document yet for this response, as this method is called
from `DocumentLoader::receivedResponse`. The committed document is still the
pre-navigation document.

Looking at `updateForSameDocumentNavigation`, it's not actually clear what in
there you need; it seems like it would be enough to replace `m_originalRequest`
and `m_request`'s URLs with the unique URL. Do you need the redirect bits as
well?

[Mike West, 2016-06-23 06:27:04]

On 2016/06/23 at 06:26:40, Mike West wrote:
>
https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):
> 
>
https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/DocumentLoader.cpp:367:
updateForSameDocumentNavigation(SecurityOrigin::urlWithUniqueSecurityOrigin(),
SameDocumentNavigationDefault);
> On 2016/06/23 at 05:56:45, Charlie Reis wrote:
> > Why same document?  Aren't we leaving the current document and replacing it
with an empty document?
> 
> We haven't created the document yet for this response, as this method is
called from `DocumentLoader::receivedResponse`. The committed document is still
the pre-navigation document.
> 
> Looking at `updateForSameDocumentNavigation`, it's not actually clear what in
there you need; it seems like it would be enough to replace `m_originalRequest`
and `m_request`'s URLs with the unique URL. Do you need the redirect bits as
well?

(And yes, committing a unique URL is totally the right thing to do in the short
term. :) )

[alexmos, 2016-06-23 14:09:45]

https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:367:
updateForSameDocumentNavigation(SecurityOrigin::urlWithUniqueSecurityOrigin(),
SameDocumentNavigationDefault);
On 2016/06/23 06:26:40, Mike West wrote:
> On 2016/06/23 at 05:56:45, Charlie Reis wrote:
> > Why same document?  Aren't we leaving the current document and replacing it
> with an empty document?
> 
> We haven't created the document yet for this response, as this method is
called
> from `DocumentLoader::receivedResponse`. The committed document is still the
> pre-navigation document.
> 
> Looking at `updateForSameDocumentNavigation`, it's not actually clear what in
> there you need; it seems like it would be enough to replace
`m_originalRequest`
> and `m_request`'s URLs with the unique URL. Do you need the redirect bits as
> well?

I think I do need the redirect bits.  I tried this without them first, modifying
just the m_request url, but that hit a DCHECK in history_service.cc:
https://cs.chromium.org/chromium/src/components/history/core/browser/history_...
So I think there's an expectation that the last thing in the redirects array
will match the committed URL.

I used updateForSameDocumentNavigation since it updates both.  It actually
clears out the redirects array first - not sure whether that's ok (if we arrive
to the blocked page after a bunch of redirects), or whether I should just modify
the URL of the last m_redirectChain entry.

[Mike West, 2016-06-23 15:35:10]

Preemptive, cross-oceanic LGTM for the approach outlined in the comment below.
That seems like the least confusing minimal fix that we can rip out later. If it
works, land it. If not, poke Charlie today. :)

https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:367:
updateForSameDocumentNavigation(SecurityOrigin::urlWithUniqueSecurityOrigin(),
SameDocumentNavigationDefault);
On 2016/06/23 at 14:09:45, alexmos wrote:
> I used updateForSameDocumentNavigation since it updates both.  It actually
clears out the redirects array first - not sure whether that's ok (if we arrive
to the blocked page after a bunch of redirects), or whether I should just modify
the URL of the last m_redirectChain entry.

Hrm. Ok. I think I'd prefer for you to just extract the bits that set the URL
and adds it to the redirect list, and add a `TODO(mkwst): Remove this once XFO
moves to the browser. https://crbug.com/555418`. Calling into the other method
seems confusing, given its name.\

[alexmos, 2016-06-23 17:37:33]

Thanks!

https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2096453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:367:
updateForSameDocumentNavigation(SecurityOrigin::urlWithUniqueSecurityOrigin(),
SameDocumentNavigationDefault);
On 2016/06/23 15:35:10, Mike West wrote:
> On 2016/06/23 at 14:09:45, alexmos wrote:
> > I used updateForSameDocumentNavigation since it updates both.  It actually
> clears out the redirects array first - not sure whether that's ok (if we
arrive
> to the blocked page after a bunch of redirects), or whether I should just
modify
> the URL of the last m_redirectChain entry.
> 
> Hrm. Ok. I think I'd prefer for you to just extract the bits that set the URL
> and adds it to the redirect list, and add a `TODO(mkwst): Remove this once XFO
> moves to the browser. https://crbug.com/555418%60. Calling into the other
method
> seems confusing, given its name.\

Done.  Removed the function call in favor of explicitly updating the URL, and
also changed to update just the last entry in the redirect chain, rather than
clearing the whole chain, which I think is more correct.

[Charlie Reis, 2016-06-23 18:12:10]

Thanks!  LGTM.

[alexmos, 2016-06-23 18:17:06]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-06-23 18:17:06]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2096453002/#ps20001
(title: "Replace updateForSameDocumentNavigation with explicit code to update
the URL")

[commit-bot: I haz the power, 2016-06-23 18:17:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096453002/20001

[commit-bot: I haz the power, 2016-06-23 18:48:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-23 18:51:42]

Description was changed from

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

to

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

Committed: https://crrev.com/30535f7116c9073705a155c7cf4b0146a28f7293
Cr-Commit-Position: refs/heads/master@{#401664}
==========

[commit-bot: I haz the power, 2016-06-23 18:51:44]

Patchset 2 (id:??) landed as
https://crrev.com/30535f7116c9073705a155c7cf4b0146a28f7293
Cr-Commit-Position: refs/heads/master@{#401664}