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

extensions/renderer/user_script_set.cc

 // Copyright 2014 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.
 
 #include "extensions/renderer/user_script_set.h"
 
 #include "base/memory/ref_counted.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/renderer/render_thread.h"
 #include "extensions/common/extension.h"
@@ skipping 186 matching lines @@
 
   GURL effective_document_url = ScriptContext::GetEffectiveDocumentURL(
       web_frame, document_url, script->match_about_blank());
 
   if (!script->MatchesURL(effective_document_url))
     return injection.Pass();
 
   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();
   }
 
   bool inject_css = !script->css_scripts().empty() &&
                     run_location == UserScript::DOCUMENT_START;
   bool inject_js =
       !script->js_scripts().empty() && script->run_location() == run_location;
   if (inject_css || inject_js) {
     injection.reset(new ScriptInjection(
         injector.Pass(),
         web_frame->toWebLocalFrame(),
         injection_host.Pass(),
         run_location,
         tab_id));
   }
   return injection.Pass();
 }
 
 }  // namespace extensions