Fix web accessible resource checks in ShouldAllowOpenURL

chrome/browser/extensions/extension_resource_request_policy_apitest.cc

Index: chrome/browser/extensions/extension_resource_request_policy_apitest.cc
diff --git a/chrome/browser/extensions/extension_resource_request_policy_apitest.cc b/chrome/browser/extensions/extension_resource_request_policy_apitest.cc
index d3c3fcb9c66bb3551dba1901c2a0ea2eaf4b4b86..29da55095815f4ba3dd35a07d24d572ecc68ea49 100644
--- a/chrome/browser/extensions/extension_resource_request_policy_apitest.cc
+++ b/chrome/browser/extensions/extension_resource_request_policy_apitest.cc
@@ -337,39 +337,6 @@ IN_PROC_BROWSER_TEST_F(ExtensionResourceRequestPolicyTest,
   ASSERT_TRUE(RunExtensionSubtest("accessible_cer", "main.html")) << message_;
 }
 
-class NavigationErrorObserver : public content::WebContentsObserver {
- public:
-  NavigationErrorObserver(content::WebContents* web_contents, const GURL& url)
-      : content::WebContentsObserver(web_contents),
-        url_(url),
-        saw_navigation_(false) {}
-
-  void DidFinishNavigation(content::NavigationHandle* handle) override {
-    if (handle->GetURL() != url_)
-      return;
-    EXPECT_TRUE(handle->IsErrorPage());
-    saw_navigation_ = true;
-    if (run_loop_.running())
-      run_loop_.Quit();
-  }
-
-  void Wait() {
-    if (!saw_navigation_)
-      run_loop_.Run();
-  }
-
- private:
-  // The url we want to see a navigation for.
-  GURL url_;
-
-  // Have we seen the navigation for |url_| yet?
-  bool saw_navigation_;
-
-  base::RunLoop run_loop_;
-
-  DISALLOW_COPY_AND_ASSIGN(NavigationErrorObserver);
-};
-
 IN_PROC_BROWSER_TEST_F(ExtensionResourceRequestPolicyTest,
                        IframeNavigateToInaccessible) {
   ASSERT_TRUE(embedded_test_server()->Start());
@@ -388,7 +355,12 @@ IN_PROC_BROWSER_TEST_F(ExtensionResourceRequestPolicyTest,
 
   GURL private_page(
       "chrome-extension://kegmjfcnjamahdnldjmlpachmpielcdk/private.html");
-  NavigationErrorObserver observer(web_contents, private_page);
   ASSERT_TRUE(content::ExecuteScript(web_contents, "navigateFrameNow()"));
-  observer.Wait();
+  WaitForLoadStop(web_contents);
+  EXPECT_NE(private_page, web_contents->GetLastCommittedURL());
+  std::string content;
+  EXPECT_TRUE(ExecuteScriptAndExtractString(
+      ChildFrameAt(web_contents->GetMainFrame(), 0),
+      "domAutomationController.send(document.body.innerText)", &content));
+  EXPECT_NE("Private", content);

[Devlin, 2016/11/02 16:19:22]

It looks like the previous version of this checked that we ended up on an error
page.  Should we preserve that check here?

[alexmos, 2016/11/02 17:32:46]

I removed the error page expectation because there's no error page anymore in
this case.  We used to block this later in ExtensionNavigationThrottle, which
did result in an error page, but now the blocking happens earlier in
ShouldAllowOpenURL (since this is navigating a remote subframe to a
non-web-accessible extension page), which just cancels the navigation without
going to an error page.  I did mention this to Nasko briefly (he wrote the
test), and he was ok with this.

[Devlin, 2016/11/02 20:50:57]

Two thoughts:
- Is there any way we can test that we are on the right page?  e.g. if we're
supposed to end on about:blank, we could verify an empty body, etc.  I'm a
little worried that asserting the content isn't equivalent to the exact content
is a little fragile.
- We seem to be treating attempted access to non-war pages very differently
frequently.  IIRC, we have at least two different errors we show, and also
sometimes about:blank pages (or canceled navigations?).  All of these achieve
the main goal - don't load the page - but I'm worried that it leads to a
confusing story for us, extension devs, users, etc, and might also make things
like fingerprinting easier.  Nasko and I were chatting about actually
documenting what should happen in various cases, and I think this might end up
being an important part.  I'm not sure if there's anything much to do in this
CL; I just worry we're making any consistent story harder to achieve (but we're
already far enough away that this probably won't make a difference).

[alexmos, 2016/11/03 00:43:26]

Unfortunately, this is tricky.  The result depends on whether or not
--isolate-extensions is on.  If it's on, we'll stay on the public.html page
(hitting the new blocking logic that cancels the navigation), and if it's off,
we'll still land at the error page, since the block would still happen in
ExtensionNavigationThrottle.  This check is an easy way to handle both while
we're in the transient state where we might flip --isolate-extensions on and off
as we try to ship it.  I added a comment explaining this and a TODO to
strengthen the check.  I could handle two error modes differently in a stricter
check, but didn't go there given that --isolate-extensions will soon be the
default again. (But let me know if you'd prefer that.)
I totally agree with this, and I think we should try to consolidate the various
kinds of WAR checks and failure modes.  It's really confusing, for example, that
the renderer-side checks aren't always consistent with the browser-side checks
(e.g., the WAR check in ResourceRequestPolicy::CanRequestResource happily lets
the navigation in this test through (due to the |is_own_resource| case, which
doesn't realize that another frame is navigating the target frame), whereas it
should really block it and output a helpful console error).  I was going to try
and clean up the about:blank cases to also lead to canceled navigations (I'm not
yet sure if that's going to break something).  But I'm leaving all that for
future followups.

[Devlin, 2016/11/03 00:57:09]

Okay, this is fine for now, thanks for the TODO.

 }