[Extensions] Handle some funny cases in script injection

extensions/browser/script_executor.cc

Index: extensions/browser/script_executor.cc
diff --git a/extensions/browser/script_executor.cc b/extensions/browser/script_executor.cc
index 2b6164613d806145707d3b541152bd423a9871a7..d589c4f4120794e69aebac217ba28fd1bead5aa8 100644
--- a/extensions/browser/script_executor.cc
+++ b/extensions/browser/script_executor.cc
@@ -42,7 +42,6 @@ class Handler : public content::WebContentsObserver {
         script_observers_(AsWeakPtr(script_observers)),
         host_id_(params.host_id),
         request_id_(params.request_id),
-        num_pending_(0),
         callback_(callback) {
     if (scope == ScriptExecutor::ALL_FRAMES) {
       web_contents->ForEachFrame(base::Bind(&Handler::SendExecuteCode,
@@ -57,9 +56,7 @@ class Handler : public content::WebContentsObserver {
   ~Handler() override {}
 
   // content::WebContentsObserver:
-  void WebContentsDestroyed() override {
-    Finish(kRendererDestroyed, GURL(), base::ListValue());
-  }
+  void WebContentsDestroyed() override { Finish(); }
 
   bool OnMessageReceived(const IPC::Message& message,
                          content::RenderFrameHost* render_frame_host) override {
@@ -82,11 +79,19 @@ class Handler : public content::WebContentsObserver {
     return true;
   }
 
+  void RenderFrameDeleted(
+      content::RenderFrameHost* render_frame_host) override {
+    if (pending_render_frames_.erase(render_frame_host) == 1 &&
+        pending_render_frames_.empty()) {
+      Finish();
+    }
+  }
+
   // Sends an ExecuteCode message to the given frame host, and increments
   // the number of pending messages.
   void SendExecuteCode(const ExtensionMsg_ExecuteCode_Params& params,
                        content::RenderFrameHost* frame) {
-    ++num_pending_;
+    pending_render_frames_.insert(frame);
     frame->Send(new ExtensionMsg_ExecuteCode(frame->GetRoutingID(), params));
   }
 
@@ -97,7 +102,9 @@ class Handler : public content::WebContentsObserver {
                              const GURL& on_url,
                              const base::ListValue& result_list) {
     DCHECK_EQ(request_id_, request_id);
-    DCHECK_GT(num_pending_, 0);
+    DCHECK(!pending_render_frames_.empty());
+    bool erased = pending_render_frames_.erase(render_frame_host) == 1;
+    DCHECK(erased);

[not at google - send to devlin, 2015/06/29 18:28:11]

I prefer the pattern of not having a variable which is only used for a DCHECK,
but rather:

if (!pending_render_frames_.erase(...))
  NOTREACHED();

and speaking of which, what happens if that actually does fail? Will bad/crazy
things happen, in which case, this should actually be a CHECK. Or if you're
feeling conservative, an early-return.

[Devlin, 2015/06/29 19:41:44]

Not to be difficult, but why is an if-statement for the sole purpose of a DCHECK
any better than a variable for the sole purpose of the DCHECK?  In my mind, the
former is more confusing, because it makes it look like the code path can
change, whereas the latter makes it clear it won't.

Re what actually happens, nothing bad will really happen, and it should never
happen, so I don't really wanna slow down (even microscopically) a production
build for this sanity check.

[not at google - send to devlin, 2015/06/29 20:09:07]

If nothing bad happens if it fails, and it should never happen anyway, why
bother DCHECKing at all?

[Devlin, 2015/06/29 20:43:13]

Sanity-checking purposes.  So that future Ben and Devlin get a nice stack trace
when they make a silly patch. ;)

     bool is_main_frame = web_contents()->GetMainFrame() == render_frame_host;
 
     // Set the result, if there is one.
@@ -117,26 +124,26 @@ class Handler : public content::WebContentsObserver {
     }
 
     // Wait until the final request finishes before reporting back.
-    if (--num_pending_ > 0)
-      return;
+    if (pending_render_frames_.empty())
+      Finish();
+  }
+
+  void Finish() {
+    if (url_.is_empty()) {  // We never finished the main frame injection.
+      error_ = kRendererDestroyed;
+      results_.Clear();
+    }
 
-    if (script_observers_.get() && error.empty() &&
+    if (script_observers_.get() && error_.empty() &&
         host_id_.type() == HostID::EXTENSIONS) {
       ScriptExecutionObserver::ExecutingScriptsMap id_map;
       id_map[host_id_.id()] = std::set<std::string>();

[not at google - send to devlin, 2015/06/29 18:28:11]

In tracking down this slightly odd-looking line of code, I came across the
comment in extensions/browser/script_execution_observer.h:

  // |executing_scripts_map| contains all extensions that are executing
  // scripts, mapped to the paths for those scripts. This may be an empty set
  // if the script has no path associated with it (e.g. in the case of
  // tabs.executeScript).

Could you change that slightly to imply that there will still be *entries* for
those extensions (i.e. the extension will exist in the map), just that the set
of scripts they reference may be empty? The way it's worded is ambiguous, it may
imply that the set of *map keys* is empty, which isn't true.

[Devlin, 2015/06/29 19:41:44]

Done.

-      FOR_EACH_OBSERVER(ScriptExecutionObserver,
-                        *script_observers_,
-                        OnScriptsExecuted(web_contents(), id_map, on_url));
+      FOR_EACH_OBSERVER(ScriptExecutionObserver, *script_observers_,
+                        OnScriptsExecuted(web_contents(), id_map, url_));
     }
 
-    Finish(error_, url_, results_);
-  }
-
-  void Finish(const std::string& error,
-              const GURL& url,
-              const base::ListValue& result) {
     if (!callback_.is_null())
-      callback_.Run(error, url, result);
+      callback_.Run(error_, url_, results_);
     delete this;
   }
 
@@ -148,8 +155,8 @@ class Handler : public content::WebContentsObserver {
   // The request id of the injection.
   int request_id_;
 
-  // The number of still-running injections.
-  int num_pending_;
+  // The hosts of the still-running injections.
+  std::set<content::RenderFrameHost*> pending_render_frames_;
 
   // The results of the injection.
   base::ListValue results_;