(Re-)introduce AncestorThrottle to handle 'X-Frame-Options'.

chrome/renderer/chrome_content_renderer_client.cc

Index: chrome/renderer/chrome_content_renderer_client.cc
diff --git a/chrome/renderer/chrome_content_renderer_client.cc b/chrome/renderer/chrome_content_renderer_client.cc
index 79f1a1642037141c8d69d6b622b663d5873d66c9..2423bee362bc18655b11452e2a1238b3492d2013 100644
--- a/chrome/renderer/chrome_content_renderer_client.cc
+++ b/chrome/renderer/chrome_content_renderer_client.cc
@@ -1019,6 +1019,23 @@ bool ChromeContentRendererClient::ShouldSuppressErrorPage(
       NetErrorHelper::Get(render_frame)->ShouldSuppressErrorPage(url)) {
     return true;
   }
+
+#if defined(ENABLE_EXTENSIONS)
+  // Suppress error pages for the webstore when it's loaded in a frame. This
+  // mitigates confusion which could arise from the fact that error pages commit
+  // with the same URL as the failed load. Since pieces of the browser sometimes
+  // rely on the last-committed URL as a permission check of sorts, and the
+  // webstore is a special flower, this can cause problems that we can avoid by
+  // simply suppressing the error page (see https://crbug.com/622385, for
+  // example).
+  GURL webstore_url(extension_urls::GetWebstoreLaunchURL());
+  if (!render_frame->IsMainFrame() &&
+      url.SchemeIsHTTPOrHTTPS() &&
+      url.DomainIs(webstore_url.host().c_str())) {
+    return true;
+  }
+#endif
+

[Mike West, 2016/09/14 09:21:24]

This is ugly. But without some ~large refactoring in the way error pages are put
together, it's going to be pretty hard to satisfy the constraint that the
committed URL must differ from the actual URL. My vague understanding of error
pages (based on an hour of digging around this morning) is that they
intentionally use the failed URL so that reload works the way it's expected to
work. *shrug*

[alexmos, 2016/09/14 20:25:10]

Yeah, a bit unfortunate that we have to do this, but I guess I'm ok with it.

But here is a question for Charlie about the CWS kill.  The new error page sets
the blocked URL as the last committed URL, but the last committed origin is
actually "null" (since it's a data:text/html,chromewebdata page).  I wonder if
we could switch the CWS kill to work off of the committed origin instead of URL?
 (I.e., instead of RFHI::CanCommitURL doing this check on the URL,  move the
check to only happen via CanCommitOrigin.  Looks like CanCommitOrigin already
calls CanCommitURL with the GURL constructed from the origin, so we're
effectively checking this twice today.)  That also seems better for cases like
blob:/filesystem: URLs that we've been fighting through in recent bugs.  One
concern though would be if you tried to put CWS in a sandboxed iframe, which
would also commit a null origin.  Charlie, what do you think?

[Charlie Reis, 2016/09/14 21:39:05]

Hmm, I'm not ok with this change.  This check isn't going to be specific to the
CWS for long.  We need to be able to enforce the process model for things like
--isolate-extensions and Site Isolation by killing the renderer if it claims to
commit a page it's not allowed to.  We can't suppress error pages in all those
cases.
I'm not sure if that's safe.  Ideally we'd enforce it for both URL and origin,
rather than only checking origin.  The reason is that we end up with a record
that a URL committed in a process when it really couldn't have, and that
NavigationEntry ends up with a SiteInstance for a process that isn't allowed to
load the URL.  Mike mentioned the reload case, and we had exactly that bug in
https://crbug.com/560511, where reloading an error page would skip a process
swap when one was needed.  It's not clear that an origin check would always
catch that, given the possibility of a null origin in a sandboxed iframe that
Alex mentions.

At some point soon, I think we need to revisit how error pages work so that they
don't have this problem (and hopefully so that we don't have 5 variations of
error page strategies with different URL/origin/process properties, across net,
blob, filesystem, XFO, FilterURL, and probably others).

I realize we'll need something for the short term, but I'm not sure what that is
yet.  (It's possible that moving the check from CanCommitURL to CanCommitOrigin
might not be a big regression in the short term, but I'm concerned about it for
the reason above.)

[alexmos, 2016/09/17 01:43:16]

Given Charlie's comments, and as a way to make progress on this in the short
term, I wonder if it's possible to reland this without committing error pages
for XFO blocks at first, and then add them in a followup that would handle all
the complications we're discussing?  I don't know how complicated that would be
though (e.g., looks like XFO blocks might not be able to use
ERR_BLOCKED_BY_CLIENT since that's used for other blocks that might want error
pages).  What do you all think?

(Also, just to point out, there's another issue related to errors and CWS kills,
https://crbug.com/588314 - we could also run into trouble if a redirect points
to a page that is blocked by XFO.)

   // Do not flash an error page if the Instant new tab page fails to load.
   return SearchBouncer::GetInstance()->IsNewTabPage(url);
 }