[Extensions] Skip injecting scripts into remote frames with site isolation turned on.

extensions/renderer/user_script_set.cc

Index: extensions/renderer/user_script_set.cc
diff --git a/extensions/renderer/user_script_set.cc b/extensions/renderer/user_script_set.cc
index f82465d6f1992e70edfedc53266935839c07b5ba..819c6308ff8cf498cff9b8ada853e7071d65c3c6 100644
--- a/extensions/renderer/user_script_set.cc
+++ b/extensions/renderer/user_script_set.cc
@@ -204,11 +204,18 @@ scoped_ptr<ScriptInjection> UserScriptSet::GetInjectionForScript(
   scoped_ptr<ScriptInjector> injector(new UserScriptInjector(script,
                                                              this,
                                                              is_declarative));
-  if (injector->CanExecuteOnFrame(
-          injection_host.get(),
-          web_frame,
-          -1,  // Content scripts are not tab-specific.
-          web_frame->top()->document().url()) ==
+
+  blink::WebDocument top_document = web_frame->top()->document();

[Devlin, 2015/03/18 22:49:31]

nit: This seems like it should be one of the first (if not the first) checks. 
Why not put it at the start of the method?

[Devlin, 2015/03/18 22:50:47]

(I'll defer to nasko that this is a suitable differentiator between OOPI and
not)

[not at google - send to devlin, 2015/03/18 22:54:54]

Another check would be to see if the top frame is remote (isWebRemoteFrame()) -
but I deliberately didn't do that. I can never remember if Remote is still
Remote with site isolation, and this is all highly temporary, and either way
it's a hack.

[not at google - send to devlin, 2015/03/18 22:54:54]

It's a hack, and the least interesting check here really. Only necessary because
the next line doesn't work without it. No need to draw attention.

[nasko, 2015/03/19 17:33:48]

Remote frames cannot host any documents or content, therefore it doesn't make
sense to do any script injection for those. The comment above the method name
also mentions this. 

I would've checked if the top() frame is WebRemoteFrame and bailed out early in
the function. The whole notion of accessing the document is flawed with remote
frames, so this code looks fragile to me.

+  // This can be null if site isolation is turned on. The best we can do is to
+  // just give up - generally the wrong behavior, but better than crashing.
+  // TODO(kalman): Fix this properly by moving all security checks into the
+  // browser. See http://crbug.com/466373 for ongoing work here.
+  if (top_document.isNull())
+    return injection.Pass();
+
+  if (injector->CanExecuteOnFrame(injection_host.get(), web_frame,
+                                  -1,  // Content scripts are not tab-specific.
+                                  top_document.url()) ==
       PermissionsData::ACCESS_DENIED) {
     return injection.Pass();
   }