[Extensions] Handle some funny cases in script injection

extensions/browser/script_executor.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/browser/script_executor.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/pickle.h"
@@ skipping 24 matching lines @@
  public:
   Handler(base::ObserverList<ScriptExecutionObserver>* script_observers,
           content::WebContents* web_contents,
           const ExtensionMsg_ExecuteCode_Params& params,
           ScriptExecutor::FrameScope scope,
           const ScriptExecutor::ExecuteScriptCallback& callback)
       : content::WebContentsObserver(web_contents),
         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,
                                             base::Unretained(this), params));
     } else {
       SendExecuteCode(params, web_contents->GetMainFrame());
     }
   }
 
  private:
   // This class manages its own lifetime.
   ~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 {
     // Unpack by hand to check the request_id, since there may be multiple
     // requests in flight but only one is for this.
     if (message.type() != ExtensionHostMsg_ExecuteCodeFinished::ID)
       return false;
 
     int message_request_id;
     base::PickleIterator iter(message);
     CHECK(iter.ReadInt(&message_request_id));
 
     if (message_request_id != request_id_)
       return false;
 
     IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(Handler, message, render_frame_host)
       IPC_MESSAGE_HANDLER(ExtensionHostMsg_ExecuteCodeFinished,
                           OnExecuteCodeFinished)
     IPC_END_MESSAGE_MAP()
     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));
   }
 
   // Handles the ExecuteCodeFinished message.
   void OnExecuteCodeFinished(content::RenderFrameHost* render_frame_host,
                              int request_id,
                              const std::string& error,
                              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.
     const base::Value* script_value = nullptr;
     if (result_list.Get(0u, &script_value)) {
       // If this is the main result, we put it at index 0. Otherwise, we just
       // append it at the end.
       if (is_main_frame && !results_.empty())
         CHECK(results_.Insert(0u, script_value->DeepCopy()));
       else
         results_.Append(script_value->DeepCopy());
     }
 
     if (is_main_frame) {  // Only use the main frame's error and url.
       error_ = error;
       url_ = on_url;
     }
 
     // Wait until the final request finishes before reporting back.
-    if (--num_pending_ > 0)
-      return;
+    if (pending_render_frames_.empty())
+      Finish();
+  }
 
-    if (script_observers_.get() && error.empty() &&
+  void Finish() {
+    if (url_.is_empty()) {  // We never finished the main frame injection.
+      error_ = kRendererDestroyed;
+      results_.Clear();
+    }
+
+    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;
   }
 
   base::WeakPtr<base::ObserverList<ScriptExecutionObserver>> script_observers_;
 
   // The id of the host (the extension or the webui) doing the injection.
   HostID host_id_;
 
   // 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_;
 
   // The error from injecting into the main frame.
   std::string error_;
 
   // The url of the main frame.
   GURL url_;

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

Could you rename to main_frame_url_ and main_frame_error_?

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

Good call; done.

 
   // The callback to run after all injections complete.
   ScriptExecutor::ExecuteScriptCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(Handler);
 };
 
 }  // namespace
 
 ScriptExecutionObserver::~ScriptExecutionObserver() {
@@ skipping 47 matching lines @@
   params.webview_src = webview_src;
   params.file_url = file_url;
   params.wants_result = (result_type == JSON_SERIALIZED_RESULT);
   params.user_gesture = user_gesture;
 
   // Handler handles IPCs and deletes itself on completion.
   new Handler(script_observers_, web_contents_, params, frame_scope, callback);
 }
 
 }  // namespace extensions