Avoid using stale UserScript pointers

extensions/renderer/user_script_injector.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_injector.h"
 
 #include <tuple>
 #include <vector>
 
 #include "base/lazy_instance.h"
@@ skipping 86 matching lines @@
 }
 
 UserScriptInjector::~UserScriptInjector() {
 }
 
 void UserScriptInjector::OnUserScriptsUpdated(
     const std::set<HostID>& changed_hosts,
     const std::vector<UserScript*>& scripts) {
   // If the host causing this injection changed, then this injection
   // will be removed, and there's no guarantee the backing script still exists.
-  if (changed_hosts.count(host_id_) > 0)
+  if (changed_hosts.count(host_id_) > 0) {
+    script_ = nullptr;

[Devlin, 2016/07/06 15:22:22]

We should call ScriptInjection::OnHostRemoved() when the extension is unloaded. 
If this only happens because of synchronous JS, would a simpler/cleaner solution
be to check injection_host_ in ScriptInjection() after we inject JS, rather than
adding a bunch of null checks here?

(optionally, you could also do also do this and then (D)CHECK script_ in most
places.)

[robwu, 2016/07/06 16:12:06]

I considered it, but because the bug is only related to user scripts, I decided
to put the checks in this class, without putting the burden of being aware of
this subtlety on the user of the class.

[Devlin, 2016/07/06 16:17:58]

I think I'd prefer checking the host in ScriptInjection for two reasons:
- We should be able to pinpoint when the host can be removed when synchronously
injecting (due to blocking) - specifically right after JS injection happens - so
there should only be a single check necessary.  In this file, it's not
immediately clear when script_ may or may not be null, and I can easily imagine
that someone will forget the null check at some point.
- This can actually affect programmatic scripts (e.g. tabs.executeScript) as
well.  While I don't think anything would necessarily break, I don't think we
should be injecting or handling scripts for removed extensions in general.

[robwu, 2016/07/06 16:26:57]

That's a good one. I will submit a separate CL since the current patch already
landed, and not injecting scripts after removal is a behavioral change (opposed
to the current patch, which fixes the bug without other side effects).

[Devlin, 2016/07/06 16:40:56]

Sounds good, thanks!

     return;
+  }
 
   for (std::vector<UserScript*>::const_iterator iter = scripts.begin();
        iter != scripts.end();
        ++iter) {
     // We need to compare to |script_id_| (and not to script_->id()) because the
     // old |script_| may be deleted by now.
     if ((*iter)->id() == script_id_) {
       script_ = *iter;
       break;
     }
@@ skipping 11 matching lines @@
 bool UserScriptInjector::IsUserGesture() const {
   return false;
 }
 
 bool UserScriptInjector::ExpectsResults() const {
   return false;
 }
 
 bool UserScriptInjector::ShouldInjectJs(
     UserScript::RunLocation run_location) const {
-  return script_->run_location() == run_location &&
+  return script_ && script_->run_location() == run_location &&
          !script_->js_scripts().empty();
 }
 
 bool UserScriptInjector::ShouldInjectCss(
     UserScript::RunLocation run_location) const {
-  return run_location == UserScript::DOCUMENT_START &&
+  return script_ && run_location == UserScript::DOCUMENT_START &&
          !script_->css_scripts().empty();
 }
 
 PermissionsData::AccessType UserScriptInjector::CanExecuteOnFrame(
     const InjectionHost* injection_host,
     blink::WebLocalFrame* web_frame,
     int tab_id) const {
+  // There is no harm in allowing the injection when the script is gone,
+  // because there is nothing to inject.
+  if (!script_)
+    return PermissionsData::ACCESS_ALLOWED;
+
   if (script_->consumer_instance_type() ==
           UserScript::ConsumerInstanceType::WEBVIEW) {
     int routing_id = content::RenderView::FromWebView(web_frame->top()->view())
                         ->GetRoutingID();
 
     RoutingInfoKey key(routing_id, script_->id());
 
     RoutingInfoMap& map = g_routing_info_map.Get();
     auto iter = map.find(key);
 
@@ skipping 20 matching lines @@
 
   return injection_host->CanExecuteOnFrame(
       effective_document_url,
       content::RenderFrame::FromWebFrame(web_frame),
       tab_id,
       is_declarative_);
 }
 
 std::vector<blink::WebScriptSource> UserScriptInjector::GetJsSources(
     UserScript::RunLocation run_location) const {
+  std::vector<blink::WebScriptSource> sources;
+  if (!script_)
+    return sources;
+
   DCHECK_EQ(script_->run_location(), run_location);
 
-  std::vector<blink::WebScriptSource> sources;
   const UserScript::FileList& js_scripts = script_->js_scripts();
 
   for (UserScript::FileList::const_iterator iter = js_scripts.begin();
        iter != js_scripts.end();
        ++iter) {
     std::string content = iter->GetContent().as_string();
 
     // We add this dumb function wrapper for user scripts to emulate what
     // Greasemonkey does.
     if (script_->emulate_greasemonkey()) {
@@ skipping 10 matching lines @@
     sources.insert(sources.begin(), g_greasemonkey_api.Get().GetSource());
 
   return sources;
 }
 
 std::vector<std::string> UserScriptInjector::GetCssSources(
     UserScript::RunLocation run_location) const {
   DCHECK_EQ(UserScript::DOCUMENT_START, run_location);
 
   std::vector<std::string> sources;
+  if (!script_)
+    return sources;
+
   const UserScript::FileList& css_scripts = script_->css_scripts();
   for (UserScript::FileList::const_iterator iter = css_scripts.begin();
        iter != css_scripts.end();
        ++iter) {
     sources.push_back(iter->GetContent().as_string());
   }
   return sources;
 }
 
 void UserScriptInjector::GetRunInfo(
     ScriptsRunInfo* scripts_run_info,
     UserScript::RunLocation run_location) const {
+  if (!script_)
+    return;
+
   if (ShouldInjectJs(run_location)) {
     const UserScript::FileList& js_scripts = script_->js_scripts();
     scripts_run_info->num_js += js_scripts.size();
     for (UserScript::FileList::const_iterator iter = js_scripts.begin();
          iter != js_scripts.end();
          ++iter) {
       scripts_run_info->executing_scripts[host_id_.id()].insert(
           iter->url().path());
     }
   }
 
   if (ShouldInjectCss(run_location))
     scripts_run_info->num_css += script_->css_scripts().size();
 }
 
 void UserScriptInjector::OnInjectionComplete(
     std::unique_ptr<base::Value> execution_result,
     UserScript::RunLocation run_location,
     content::RenderFrame* render_frame) {}
 
 void UserScriptInjector::OnWillNotInject(InjectFailureReason reason,
                                          content::RenderFrame* render_frame) {
 }
 
 }  // namespace extensions