Fix web_accesible_resources enforcement for Site Isolation.

extensions/browser/extension_navigation_throttle.h

Index: extensions/browser/extension_navigation_throttle.h
diff --git a/extensions/browser/extension_navigation_throttle.h b/extensions/browser/extension_navigation_throttle.h
new file mode 100644
index 0000000000000000000000000000000000000000..6e3c97aacc3e0749edd3d1765fc2c7fa08bc3cef
--- /dev/null
+++ b/extensions/browser/extension_navigation_throttle.h
@@ -0,0 +1,37 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef EXTENSIONS_BROWSER_EXTENSION_NAVIGATION_THROTTLE_H_
+#define EXTENSIONS_BROWSER_EXTENSION_NAVIGATION_THROTTLE_H_
+
+#include <string>

[Devlin, 2016/06/03 22:22:46]

not needed

[nasko, 2016/06/06 17:40:44]

Done.

+
+#include "base/callback.h"

[Devlin, 2016/06/03 22:22:46]

IWYU nit: base/macros.h

[nasko, 2016/06/06 17:40:44]

Done.

+#include "content/public/browser/navigation_throttle.h"
+
+namespace content {
+class NavigationHandle;
+}
+
+namespace extensions {
+
+class NavigationParams;
+
+// This class allows the extensions subsystem to have control over navigations
+// and optionally cancel/block them. This is a UI thread class.
+class ExtensionNavigationThrottle : public content::NavigationThrottle {
+ public:
+  ExtensionNavigationThrottle(content::NavigationHandle* navigation_handle);

[Devlin, 2016/06/03 22:22:45]

explicit

[nasko, 2016/06/06 17:40:44]

Done.

+  ~ExtensionNavigationThrottle() override;
+
+  // content::NavigationThrottle implementation:
+  ThrottleCheckResult WillStartRequest() override;

[Devlin, 2016/06/03 22:22:45]

I'm not very familiar with NavigationThrottle, so I'm not entirely clear on when
this will be called as opposed to our n other WAR checks.  Are they all still
necessary?  Can we change any ifs to dchecks?

[nasko, 2016/06/06 17:40:44]

This will be called before the request is sent to the network. It is called
after the job is created on the IO thread, which is where all the existing
checks are performed. However, those checks run on the IO thread, where we don't
have the ancestor information, so we can't do it there.

Because the existing checks run ahead of this one, we can't convert ifs into
DCHECKs. Also, if it gets blocked on the IO thread, we end up saving ourselves
creating a request and jumping to the UI thread, so I'd say we keep them for
perf reasons, even though we can technically remove them.

[Devlin, 2016/06/06 22:17:22]

I'm a little worried that this will become (more) confusing between having
extension_protocols, this class, url_request_util, chrome_url_request_util, etc.
 It'd be nice to have a cleaner model.  Unfortunately, I don't know how feasible
that is, given the threading restrictions and whatnot, at least not without a
bit of a refactor.

[nasko, 2016/06/06 23:23:03]

I think the cleaner model will be once we ship PlzNavigate. We can then enforce
navigation restrictions on the UI thread and subresources on the IO thread.
Also, we should probably remove the redundant checks from renderer side, unless
we think they are providing enough of perf advantage to leave them in.
I'm definitely open to simplifications. Feel free to start a discussion and I'll
contribute. I'm very big fan of simpler codebase.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExtensionNavigationThrottle);
+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_EXTENSION_NAVIGATION_THROTTLE_H_