Extensions: suspend extension's scripts when V8 is paused

extensions/renderer/script_injection.cc

Index: extensions/renderer/script_injection.cc
diff --git a/extensions/renderer/script_injection.cc b/extensions/renderer/script_injection.cc
index 58cf3280b95c31fe0048a2a0197e4e6cdd59ecde..0672967bdf324f1d2bfa2b1f5492a344468282d6 100644
--- a/extensions/renderer/script_injection.cc
+++ b/extensions/renderer/script_injection.cc
@@ -19,6 +19,8 @@
 #include "extensions/renderer/extension_groups.h"
 #include "extensions/renderer/extension_injection_host.h"
 #include "extensions/renderer/extensions_renderer_client.h"
+#include "extensions/renderer/script_injection_callback.h"
+#include "extensions/renderer/script_injection_manager.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
@@ -117,7 +119,11 @@ ScriptInjection::ScriptInjection(
       run_location_(run_location),
       tab_id_(tab_id),
       request_id_(kInvalidRequestId),
-      complete_(false) {
+      complete_(false),
+      running_frames_(0),
+      execution_results_(new base::ListValue()),
+      all_injection_started_(false),
+      script_injection_manager_(nullptr) {
 }
 
 ScriptInjection::~ScriptInjection() {
@@ -125,46 +131,48 @@ ScriptInjection::~ScriptInjection() {
     injector_->OnWillNotInject(ScriptInjector::WONT_INJECT);
 }
 
-bool ScriptInjection::TryToInject(UserScript::RunLocation current_location,
-                                  const InjectionHost* injection_host,
-                                  ScriptsRunInfo* scripts_run_info) {
+ScriptInjection::InjectionResult ScriptInjection::TryToInject(
+    UserScript::RunLocation current_location,
+    const InjectionHost* injection_host,
+    ScriptsRunInfo* scripts_run_info) {
   if (current_location < run_location_)
-    return false;  // Wait for the right location.
+    return INJECTION_WAITING;  // Wait for the right location.
 
-  if (request_id_ != kInvalidRequestId)
-    return false;  // We're waiting for permission right now, try again later.
+  if (request_id_ != kInvalidRequestId) {
+    // We're waiting for permission right now, try again later.
+    return INJECTION_WAITING;
+  }
 
   if (!injection_host) {
     NotifyWillNotInject(ScriptInjector::EXTENSION_REMOVED);
-    return true;  // We're done.
+    return INJECTION_FINISHED;  // We're done.
   }
 
   switch (injector_->CanExecuteOnFrame(injection_host, web_frame_, tab_id_,
                                        web_frame_->top()->document().url())) {
     case PermissionsData::ACCESS_DENIED:
       NotifyWillNotInject(ScriptInjector::NOT_ALLOWED);
-      return true;  // We're done.
+      return INJECTION_FINISHED;  // We're done.
     case PermissionsData::ACCESS_WITHHELD:
       SendInjectionMessage(true /* request permission */);
-      return false;  // Wait around for permission.
+      return INJECTION_WAITING;  // Wait around for permission.
     case PermissionsData::ACCESS_ALLOWED:
-      Inject(injection_host, scripts_run_info);
-      return true;  // We're done!
+      return Inject(injection_host, scripts_run_info);
   }
 
   NOTREACHED();
-  return false;
+  return INJECTION_FINISHED;
 }
 
-bool ScriptInjection::OnPermissionGranted(const InjectionHost* injection_host,
-                                          ScriptsRunInfo* scripts_run_info) {
+ScriptInjection::InjectionResult ScriptInjection::OnPermissionGranted(
+    const InjectionHost* injection_host,
+    ScriptsRunInfo* scripts_run_info) {
   if (!injection_host) {
     NotifyWillNotInject(ScriptInjector::EXTENSION_REMOVED);
-    return false;
+    return INJECTION_FINISHED;
   }
 
-  Inject(injection_host, scripts_run_info);
-  return true;
+  return Inject(injection_host, scripts_run_info);
 }
 
 void ScriptInjection::SendInjectionMessage(bool request_permission) {
@@ -187,8 +195,9 @@ void ScriptInjection::NotifyWillNotInject(
   injector_->OnWillNotInject(reason);
 }
 
-void ScriptInjection::Inject(const InjectionHost* injection_host,
-                             ScriptsRunInfo* scripts_run_info) {
+ScriptInjection::InjectionResult ScriptInjection::Inject(
+    const InjectionHost* injection_host,
+    ScriptsRunInfo* scripts_run_info) {
   DCHECK(injection_host);
   DCHECK(scripts_run_info);
   DCHECK(!complete_);
@@ -201,15 +210,10 @@ void ScriptInjection::Inject(const InjectionHost* injection_host,
   if (injector_->ShouldExecuteInChildFrames())
     AppendAllChildFrames(web_frame_, &frame_vector);
 
-  scoped_ptr<blink::WebScopedUserGesture> gesture;
-  if (injector_->IsUserGesture())
-    gesture.reset(new blink::WebScopedUserGesture());
-
   bool inject_js = injector_->ShouldInjectJs(run_location_);
   bool inject_css = injector_->ShouldInjectCss(run_location_);
   DCHECK(inject_js || inject_css);
 
-  scoped_ptr<base::ListValue> execution_results(new base::ListValue());
   GURL top_url = web_frame_->top()->document().url();
   for (std::vector<blink::WebFrame*>::iterator iter = frame_vector.begin();
        iter != frame_vector.end();
@@ -232,61 +236,66 @@ void ScriptInjection::Inject(const InjectionHost* injection_host,
       continue;
     }
     if (inject_js)
-      InjectJs(injection_host, frame, execution_results.get());
+      InjectJs(injection_host, frame);
     if (inject_css)
       InjectCss(frame);
   }
 
-  complete_ = true;
-
-  // TODO(hanxi): don't log these metrics for webUIs' injections.
-  injector_->OnInjectionComplete(execution_results.Pass(),
-                                 scripts_run_info,
-                                 run_location_);
+  all_injection_started_ = true;
+  injector_->GetRunInfo(scripts_run_info, run_location_);
+  TryToFinish();

[Devlin, 2015/02/27 18:25:32]

This TryToFinish seems wrong, since it will notify the ScriptInjectionManager. 
Instead, I think this should just say:
if (complete_)
  injector_->OnInjectionComplete(...)

[kozy, 2015/02/28 15:23:21]

If we have synchronous injections in two frames then first injection will call
TryToFinish in OnJSInjectionCompleted. This first call will change nothing
because running_frames = 0 but all_injection_started is false. Then we start
second injection, it call TryToFinish and change nothing too with the same
reason. And we need to call TryToFinish one more time in this case.
We can change complete_ only in TryToFinish methods and I suppose we can't just
check if(complete_) without TryToFinish call.

+  return complete_ ? INJECTION_FINISHED : INJECTION_BLOCKED;
 }
 
 void ScriptInjection::InjectJs(const InjectionHost* injection_host,
-                               blink::WebLocalFrame* frame,
-                               base::ListValue* execution_results) {
+                               blink::WebLocalFrame* frame) {
+  running_frames_++;

[Devlin, 2015/02/27 18:25:32]

nit: chrome style prefers pre-increment.

[kozy, 2015/02/28 15:23:21]

Done.

   std::vector<blink::WebScriptSource> sources =
       injector_->GetJsSources(run_location_);
   bool in_main_world = injector_->ShouldExecuteInMainWorld();
   int world_id = in_main_world
                      ? DOMActivityLogger::kMainWorldId
                      : GetIsolatedWorldIdForInstance(injection_host, frame);
-  bool expects_results = injector_->ExpectsResults();
+  bool is_user_gesture = injector_->IsUserGesture();
+
+  scoped_ptr<blink::WebScriptExecutionCallback> callback(
+    new ScriptInjectionCallback(this, frame));

[Devlin, 2015/02/27 18:25:32]

nit: indentation off.

[kozy, 2015/02/28 15:23:21]

Done.

 
   base::ElapsedTimer exec_timer;
   if (injection_host->id().type() == HostID::EXTENSIONS)
     DOMActivityLogger::AttachToWorld(world_id, injection_host->id().id());
-  v8::HandleScope scope(v8::Isolate::GetCurrent());
-  v8::Local<v8::Value> script_value;
+
   if (in_main_world) {
     // We only inject in the main world for javascript: urls.
     DCHECK_EQ(1u, sources.size());
 
-    const blink::WebScriptSource& source = sources.front();
-    if (expects_results)
-      script_value = frame->executeScriptAndReturnValue(source);
-    else
-      frame->executeScript(source);
+    frame->requestExecuteScriptAndReturnValue(sources.front(),
+                                              is_user_gesture,
+                                              callback.release());
   } else {  // in isolated world
-    scoped_ptr<blink::WebVector<v8::Local<v8::Value> > > results;
-    if (expects_results)
-      results.reset(new blink::WebVector<v8::Local<v8::Value> >());
-    frame->executeScriptInIsolatedWorld(world_id,
-                                        &sources.front(),
-                                        sources.size(),
-                                        EXTENSION_GROUP_CONTENT_SCRIPTS,
-                                        results.get());
-    if (expects_results && !results->isEmpty())
-      script_value = (*results)[0];
+    frame->requestExecuteScriptInIsolatedWorld(world_id,
+                                               &sources.front(),
+                                               sources.size(),
+                                               EXTENSION_GROUP_CONTENT_SCRIPTS,
+                                               is_user_gesture,
+                                               callback.release());
   }
 
   if (injection_host->id().type() == HostID::EXTENSIONS)
     UMA_HISTOGRAM_TIMES("Extensions.InjectScriptTime", exec_timer.Elapsed());
+}
+
+void ScriptInjection::OnJSInjectionCompleted(
+    blink::WebLocalFrame* frame,
+    const blink::WebVector<v8::Local<v8::Value> >& results) {
+  DCHECK(running_frames_ > 0);
+  --running_frames_;
 
+  bool expects_results = injector_->ExpectsResults();
   if (expects_results) {
+    v8::Local<v8::Value> script_value;
+    if (!results.isEmpty())
+      script_value = results[0];
     // Right now, we only support returning single results (per frame).
     scoped_ptr<content::V8ValueConverter> v8_converter(
         content::V8ValueConverter::create());
@@ -297,10 +306,33 @@ void ScriptInjection::InjectJs(const InjectionHost* injection_host,
     v8::Local<v8::Context> context = frame->mainWorldScriptContext();
     scoped_ptr<base::Value> result(
         v8_converter->FromV8Value(script_value, context));
-    // Always append an execution result (i.e. no result == null result)
-    // so that |execution_results| lines up with the frames.
-    execution_results->Append(result.get() ? result.release()
-                                           : base::Value::CreateNullValue());
+    // if current frame is main frame in injection

[Devlin, 2015/02/27 18:25:32]

This comment isn't really helpful (it just describes the if-statement). 
Instead, let's put a comment like:
// We guarantee that the main frame's result is at the first index, but
// any sub frames results do not have guaranteed order.

[kozy, 2015/02/28 15:23:21]

Done.

+    if (frame == web_frame_) {
+      execution_results_->Insert(0, result.get()
+                                      ? result.release()
+                                      : base::Value::CreateNullValue());

[Devlin, 2015/02/27 18:25:32]

instead of having this result.get() ? logic twice, above the if-statement for
the frames, do:
if (!result)
  result.reset(new base::Value::CreateNullValue());

and then when appending you can just do result.release().

Also, let's take out the frame if, and just do:
execution_results_->Insert(
    frame == web_frame_ ? 0 : execution_results_->GetSize(), result.get());

[kozy, 2015/02/28 15:23:21]

Done.

+    } else  {
+      execution_results_->Append(result.get() ? result.release()
+                                              : base::Value::CreateNullValue());
+    }
+  }
+  TryToFinish();
+}
+
+void ScriptInjection::SetScriptInjectionManager(
+  ScriptInjectionManager* manager){

[Devlin, 2015/02/27 18:25:32]

nit: indenation off.

[kozy, 2015/02/28 15:23:21]

Done.

+  script_injection_manager_ = manager;
+}
+
+void ScriptInjection::TryToFinish() {
+  if (all_injection_started_ && running_frames_ == 0) {
+    complete_ = true;
+    injector_->OnInjectionComplete(execution_results_.Pass(),
+                                   run_location_);
+
+    CHECK(script_injection_manager_);
+    // this object can be destroyed after next line
+    script_injection_manager_->OnInjectionFinished(this);
   }
 }