Move Extension ScriptContext creation into ScriptContextSet.

Previously extensions::Dispatcher was responsible for creating ScriptContexts
then Adding them to its ScriptContextSet. Now, ScriptContextSet creates them
itself. This prepares the Extension system for later changing to lazily or
asynchronously creating ScriptContexts.

BUG=466373
R=rockot@chromium.org

Review URL: https://codereview.chromium.org/995283004

Cr-Commit-Position: refs/heads/master@{#320971}

chrome/renderer/extensions/webstore_bindings.cc

@@ -14,6 +14,7 @@
 #include "extensions/renderer/script_context.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebElement.h"
+#include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "third_party/WebKit/public/web/WebNode.h"
 #include "third_party/WebKit/public/web/WebNodeList.h"
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
@@ -22,7 +23,6 @@
 
 using blink::WebDocument;
 using blink::WebElement;
-using blink::WebFrame;
 using blink::WebNode;
 using blink::WebNodeList;
 using blink::WebUserGestureIndicator;
@@ -79,7 +79,7 @@ void WebstoreBindings::Install(
 
   std::string webstore_item_id;
   std::string error;
-  WebFrame* frame = context()->web_frame();
+  blink::WebLocalFrame* frame = context()->web_frame();
 
   if (!GetWebstoreItemIdFromFrame(
       frame, preferred_store_link_url, &webstore_item_id, &error)) {
@@ -102,8 +102,10 @@ void WebstoreBindings::Install(
 
 // static
 bool WebstoreBindings::GetWebstoreItemIdFromFrame(
-      WebFrame* frame, const std::string& preferred_store_link_url,
-      std::string* webstore_item_id, std::string* error) {
+    blink::WebLocalFrame* frame,
+    const std::string& preferred_store_link_url,
+    std::string* webstore_item_id,
+    std::string* error) {
   if (frame != frame->top()) {
     *error = kNotInTopFrameError;
     return false;

chrome/renderer/extensions/webstore_bindings.h

@@ -9,7 +9,10 @@
 #include "chrome/common/extensions/webstore_install_result.h"
 #include "chrome/renderer/extensions/chrome_v8_extension_handler.h"
 #include "extensions/renderer/object_backed_native_handler.h"
-#include "third_party/WebKit/public/web/WebFrame.h"
+
+namespace blink {
+class WebLocalFrame;
+}
 
 namespace extensions {
 
@@ -42,8 +45,10 @@ class WebstoreBindings : public ObjectBackedNativeHandler,
   // parameter will be populated with the ID. On failure, false will be returned
   // and |error| will be populated with the error.
   static bool GetWebstoreItemIdFromFrame(
-      blink::WebFrame* frame, const std::string& preferred_store_link_url,
-      std::string* webstore_item_id, std::string* error);
+      blink::WebLocalFrame* frame,
+      const std::string& preferred_store_link_url,
+      std::string* webstore_item_id,
+      std::string* error);
 
   DISALLOW_COPY_AND_ASSIGN(WebstoreBindings);
 };

extensions/renderer/dispatcher.h

@@ -11,10 +11,12 @@
 #include <utility>
 #include <vector>
 
+#include "base/memory/scoped_ptr.h"
 #include "base/scoped_observer.h"
 #include "base/timer/timer.h"
 #include "content/public/renderer/render_process_observer.h"
 #include "extensions/common/event_filter.h"
+#include "extensions/common/extension.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/extensions_client.h"
 #include "extensions/common/features/feature.h"
@@ -53,7 +55,6 @@ class RenderThread;
 namespace extensions {
 class ContentWatcher;
 class DispatcherDelegate;
-class Extension;
 class FilteredEventRouter;
 class ManifestPermissionSet;
 class RequestSender;
@@ -76,7 +77,7 @@ class Dispatcher : public content::RenderProcessObserver,
   const ExtensionSet* extensions() const { return &extensions_; }
 
   const ScriptContextSet& script_context_set() const {
-    return script_context_set_;
+    return *script_context_set_;
   }
 
   V8SchemaRegistry* v8_schema_registry() { return v8_schema_registry_.get(); }
@@ -89,14 +90,6 @@ class Dispatcher : public content::RenderProcessObserver,
 
   bool IsExtensionActive(const std::string& extension_id) const;
 
-  // Finds the extension for the JavaScript context associated with the
-  // specified |frame| and isolated world. If |world_id| is zero, finds the
-  // extension ID associated with the main world's JavaScript context. If the
-  // JavaScript context isn't from an extension, returns empty string.
-  const Extension* GetExtensionFromFrameAndWorld(const blink::WebFrame* frame,
-                                                 int world_id,
-                                                 bool use_effective_url);
-
   void DidCreateScriptContext(blink::WebLocalFrame* frame,
                               const v8::Handle<v8::Context>& context,
                               int extension_group,
@@ -242,15 +235,6 @@ class Dispatcher : public content::RenderProcessObserver,
   // Returns whether the current renderer hosts a platform app.
   bool IsWithinPlatformApp();
 
-  bool IsSandboxedPage(const GURL& url) const;
-
-  // Returns the Feature::Context type of context for a JavaScript context.
-  Feature::Context ClassifyJavaScriptContext(
-      const Extension* extension,
-      int extension_group,
-      const GURL& url,
-      const blink::WebSecurityOrigin& origin);
-
   // Gets |field| from |object| or creates it as an empty object if it doesn't
   // exist.
   v8::Handle<v8::Object> GetOrCreateObject(const v8::Handle<v8::Object>& object,
@@ -284,7 +268,7 @@ class Dispatcher : public content::RenderProcessObserver,
 
   // All the bindings contexts that are currently loaded for this renderer.
   // There is zero or one for each v8 context.
-  ScriptContextSet script_context_set_;
+  scoped_ptr<ScriptContextSet> script_context_set_;
 
   scoped_ptr<ContentWatcher> content_watcher_;
 
@@ -300,7 +284,7 @@ class Dispatcher : public content::RenderProcessObserver,
   std::set<std::string> function_names_;
 
   // The extensions and apps that are active in this process.
-  std::set<std::string> active_extension_ids_;
+  ExtensionIdSet active_extension_ids_;
 
   ResourceBundleSourceMap source_map_;
 

extensions/renderer/document_custom_bindings.cc

@@ -9,7 +9,7 @@
 #include "base/bind.h"
 #include "extensions/renderer/script_context.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
-#include "third_party/WebKit/public/web/WebFrame.h"
+#include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "v8/include/v8.h"
 
 namespace extensions {

extensions/renderer/request_sender.cc

@@ -10,7 +10,7 @@
 #include "extensions/renderer/dispatcher.h"
 #include "extensions/renderer/script_context.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
-#include "third_party/WebKit/public/web/WebFrame.h"
+#include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "third_party/WebKit/public/web/WebScopedUserGesture.h"
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
 #include "third_party/WebKit/public/web/WebUserGestureToken.h"
@@ -97,7 +97,7 @@ void RequestSender::StartRequest(Source* source,
     return;
 
   GURL source_url;
-  if (blink::WebFrame* webframe = context->web_frame())
+  if (blink::WebLocalFrame* webframe = context->web_frame())
     source_url = webframe->document().url();
 
   InsertRequest(request_id, new PendingRequest(name, source,

extensions/renderer/script_context.cc

@@ -13,15 +13,19 @@
 #include "content/public/common/url_constants.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_view.h"
+#include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_api.h"
+#include "extensions/common/extension_set.h"
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/features/base_feature_provider.h"
+#include "extensions/common/manifest_handlers/sandboxed_page_info.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "gin/per_context_data.h"
 #include "third_party/WebKit/public/web/WebDataSource.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
+#include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h"
 #include "third_party/WebKit/public/web/WebSecurityOrigin.h"
 #include "third_party/WebKit/public/web/WebView.h"
@@ -75,7 +79,7 @@ class ScriptContext::Runner : public gin::Runner {
 };
 
 ScriptContext::ScriptContext(const v8::Handle<v8::Context>& v8_context,
-                             blink::WebFrame* web_frame,
+                             blink::WebLocalFrame* web_frame,
                              const Extension* extension,
                              Feature::Context context_type,
                              const Extension* effective_extension,
@@ -99,7 +103,9 @@ ScriptContext::ScriptContext(const v8::Handle<v8::Context>& v8_context,
           << (effective_extension_.get() ? effective_extension_->id() : "")
           << "  effective context type: "
           << GetEffectiveContextTypeDescription();
-  gin::PerContextData::From(v8_context)->set_runner(runner_.get());
+  gin::PerContextData* gin_data = gin::PerContextData::From(v8_context);
+  CHECK(gin_data);  // may fail if the v8::Context hasn't been registered yet
+  gin_data->set_runner(runner_.get());
 }
 
 ScriptContext::~ScriptContext() {
@@ -110,6 +116,21 @@ ScriptContext::~ScriptContext() {
   Invalidate();
 }
 
+// static
+bool ScriptContext::IsSandboxedPage(const ExtensionSet& extensions,
+                                    const GURL& url) {
+  // TODO(kalman): This is checking for the wrong thing, it should be checking
+  // if the frame's security origin is unique. The extension sandbox directive
+  // is checked for in extensions/common/manifest_handlers/csp_info.cc.
+  if (url.SchemeIs(kExtensionScheme)) {
+    const Extension* extension = extensions.GetByID(url.host());
+    if (extension) {
+      return SandboxedPageInfo::IsSandboxedPage(extension, url.path());
+    }
+  }
+  return false;
+}
+
 void ScriptContext::Invalidate() {
   if (!is_valid())
     return;

extensions/renderer/script_context.h

@@ -20,6 +20,7 @@
 
 namespace blink {
 class WebFrame;
+class WebLocalFrame;
 }
 
 namespace content {
@@ -29,18 +30,27 @@ class RenderView;
 
 namespace extensions {
 class Extension;
+class ExtensionSet;
 
 // Extensions wrapper for a v8 context.
 class ScriptContext : public RequestSender::Source {
  public:
   ScriptContext(const v8::Handle<v8::Context>& context,
-                blink::WebFrame* frame,
+                blink::WebLocalFrame* frame,
                 const Extension* extension,
                 Feature::Context context_type,
                 const Extension* effective_extension,
                 Feature::Context effective_context_type);
   ~ScriptContext() override;
 
+  // Returns whether |url| is sandboxed (as declared in any Extension in
+  // |extension_set| as sandboxed).
+  //
+  // Declared in ScriptContext for lack of a better place, but this should
+  // become unnecessary at some point as crbug.com/466373 is worked on.
+  static bool IsSandboxedPage(const ExtensionSet& extension_set,
+                              const GURL& url);
+
   // Clears the WebFrame for this contexts and invalidates the associated
   // ModuleSystem.
   void Invalidate();
@@ -59,7 +69,7 @@ class ScriptContext : public RequestSender::Source {
     return effective_extension_.get();
   }
 
-  blink::WebFrame* web_frame() const { return web_frame_; }
+  blink::WebLocalFrame* web_frame() const { return web_frame_; }
 
   Feature::Context context_type() const { return context_type_; }
 
@@ -114,6 +124,14 @@ class ScriptContext : public RequestSender::Source {
   v8::Isolate* isolate() const { return isolate_; }
 
   // Get the URL of this context's web frame.
+  //
+  // TODO(kalman): Remove this and replace with a GetOrigin() call which reads
+  // of WebDocument::securityOrigin():
+  //  - The URL can change (e.g. pushState) but the origin cannot. Luckily it
+  //    appears as though callers don't make security decisions based on the
+  //    result of GetURL() so it's not a problem... yet.
+  //  - Origin is the correct check to be making.
+  //  - It might let us remove the about:blank resolving?
   GURL GetURL() const;
 
   // Returns whether the API |api| or any part of the API could be
@@ -157,9 +175,9 @@ class ScriptContext : public RequestSender::Source {
  private:
   class Runner;
 
-  // The WebFrame associated with this context. This can be NULL because this
-  // object can outlive is destroyed asynchronously.
-  blink::WebFrame* web_frame_;
+  // The WebLocalFrame associated with this context. This can be NULL because
+  // this object can outlive is destroyed asynchronously.
+  blink::WebLocalFrame* web_frame_;
 
   // The extension associated with this context, or NULL if there is none. This
   // might be a hosted app in the case that this context is hosting a web URL.

extensions/renderer/script_context_set.cc

@@ -5,34 +5,50 @@
 #include "extensions/renderer/script_context_set.h"
 
 #include "base/message_loop/message_loop.h"
+#include "content/public/common/url_constants.h"
 #include "content/public/renderer/render_view.h"
 #include "extensions/common/extension.h"
+#include "extensions/renderer/extension_groups.h"
 #include "extensions/renderer/script_context.h"
+#include "extensions/renderer/script_injection.h"
+#include "third_party/WebKit/public/web/WebDocument.h"
+#include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "v8/include/v8.h"
 
 namespace extensions {
 
-ScriptContextSet::ScriptContextSet() {
-}
-ScriptContextSet::~ScriptContextSet() {
+ScriptContextSet::ScriptContextSet(ExtensionSet* extensions,
+                                   ExtensionIdSet* active_extension_ids)
+    : extensions_(extensions), active_extension_ids_(active_extension_ids) {
 }
 
-int ScriptContextSet::size() const {
-  return static_cast<int>(contexts_.size());
+ScriptContextSet::~ScriptContextSet() {
 }
 
-void ScriptContextSet::Add(ScriptContext* context) {
-#if DCHECK_IS_ON()
-  // It's OK to insert the same context twice, but we should only ever have
-  // one ScriptContext per v8::Context.
-  for (ContextSet::iterator iter = contexts_.begin(); iter != contexts_.end();
-       ++iter) {
-    ScriptContext* candidate = *iter;
-    if (candidate != context)
-      DCHECK(candidate->v8_context() != context->v8_context());
-  }
-#endif
-  contexts_.insert(context);
+ScriptContext* ScriptContextSet::Register(
+    blink::WebLocalFrame* frame,
+    const v8::Handle<v8::Context>& v8_context,
+    int extension_group,
+    int world_id) {
+  const Extension* extension =
+      GetExtensionFromFrameAndWorld(frame, world_id, false);
+  const Extension* effective_extension =
+      GetExtensionFromFrameAndWorld(frame, world_id, true);
+
+  GURL frame_url = ScriptContext::GetDataSourceURLForFrame(frame);
+  Feature::Context context_type =
+      ClassifyJavaScriptContext(extension, extension_group, frame_url,
+                                frame->document().securityOrigin());
+  Feature::Context effective_context_type = ClassifyJavaScriptContext(
+      effective_extension, extension_group,
+      ScriptContext::GetEffectiveDocumentURL(frame, frame_url, true),
+      frame->document().securityOrigin());
+
+  ScriptContext* context =
+      new ScriptContext(v8_context, frame, extension, context_type,
+                        effective_extension, effective_context_type);
+  contexts_.insert(context);  // takes ownership
+  return context;
 }
 
 void ScriptContextSet::Remove(ScriptContext* context) {
@@ -42,32 +58,25 @@ void ScriptContextSet::Remove(ScriptContext* context) {
   }
 }
 
-ScriptContextSet::ContextSet ScriptContextSet::GetAll() const {
-  return contexts_;
-}
-
 ScriptContext* ScriptContextSet::GetCurrent() const {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
   return isolate->InContext() ? GetByV8Context(isolate->GetCurrentContext())
-                              : NULL;
+                              : nullptr;
 }
 
 ScriptContext* ScriptContextSet::GetCalling() const {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
   v8::Local<v8::Context> calling = isolate->GetCallingContext();
-  return calling.IsEmpty() ? NULL : GetByV8Context(calling);
+  return calling.IsEmpty() ? nullptr : GetByV8Context(calling);
 }
 
 ScriptContext* ScriptContextSet::GetByV8Context(
-    v8::Handle<v8::Context> v8_context) const {
-  for (ContextSet::const_iterator iter = contexts_.begin();
-       iter != contexts_.end();
-       ++iter) {
-    if ((*iter)->v8_context() == v8_context)
-      return *iter;
+    const v8::Handle<v8::Context>& v8_context) const {
+  for (ScriptContext* script_context : contexts_) {
+    if (script_context->v8_context() == v8_context)
+      return script_context;
   }
-
-  return NULL;
+  return nullptr;
 }
 
 void ScriptContextSet::ForEach(
@@ -76,11 +85,9 @@ void ScriptContextSet::ForEach(
     const base::Callback<void(ScriptContext*)>& callback) const {
   // We copy the context list, because calling into javascript may modify it
   // out from under us.
-  ContextSet contexts = GetAll();
-
-  for (ContextSet::iterator it = contexts.begin(); it != contexts.end(); ++it) {
-    ScriptContext* context = *it;
+  std::set<ScriptContext*> contexts_copy = contexts_;
 
+  for (ScriptContext* context : contexts_copy) {
     // For the same reason as above, contexts may become invalid while we run.
     if (!context->is_valid())
       continue;
@@ -102,23 +109,103 @@ void ScriptContextSet::ForEach(
   }
 }
 
-ScriptContextSet::ContextSet ScriptContextSet::OnExtensionUnloaded(
+std::set<ScriptContext*> ScriptContextSet::OnExtensionUnloaded(
     const std::string& extension_id) {
-  ContextSet contexts = GetAll();
-  ContextSet removed;
-
-  // Clean up contexts belonging to the unloaded extension. This is done so
-  // that content scripts (which remain injected into the page) don't continue
-  // receiving events and sending messages.
-  for (ContextSet::iterator it = contexts.begin(); it != contexts.end(); ++it) {
-    if ((*it)->extension() && (*it)->extension()->id() == extension_id) {
-      (*it)->DispatchOnUnloadEvent();
-      removed.insert(*it);
-      Remove(*it);
-    }
+  std::set<ScriptContext*> removed;
+  ForEach(extension_id,
+          base::Bind(&ScriptContextSet::DispatchOnUnloadEventAndRemove,
+                     base::Unretained(this), &removed));
+  return removed;
+}
+
+const Extension* ScriptContextSet::GetExtensionFromFrameAndWorld(
+    const blink::WebLocalFrame* frame,
+    int world_id,
+    bool use_effective_url) {
+  std::string extension_id;
+  if (world_id != 0) {
+    // Isolated worlds (content script).
+    extension_id = ScriptInjection::GetHostIdForIsolatedWorld(world_id);
+  } else if (!frame->document().securityOrigin().isUnique()) {
+    // TODO(kalman): Delete the above check.
+    // Extension pages (chrome-extension:// URLs).
+    GURL frame_url = ScriptContext::GetDataSourceURLForFrame(frame);
+    frame_url = ScriptContext::GetEffectiveDocumentURL(frame, frame_url,
+                                                       use_effective_url);
+    extension_id = extensions_->GetExtensionOrAppIDByURL(frame_url);
   }
 
-  return removed;
+  // There are conditions where despite a context being associated with an
+  // extension, no extension actually gets found. Ignore "invalid" because CSP
+  // blocks extension page loading by switching the extension ID to "invalid".
+  const Extension* extension = extensions_->GetByID(extension_id);
+  if (!extension && !extension_id.empty() && extension_id != "invalid") {
+    // TODO(kalman): Do something here?
+  }
+  return extension;
+}
+
+Feature::Context ScriptContextSet::ClassifyJavaScriptContext(
+    const Extension* extension,
+    int extension_group,
+    const GURL& url,
+    const blink::WebSecurityOrigin& origin) {
+  // WARNING: This logic must match ProcessMap::GetContextType, as much as
+  // possible.
+
+  DCHECK_GE(extension_group, 0);
+  if (extension_group == EXTENSION_GROUP_CONTENT_SCRIPTS) {
+    return extension ?  // TODO(kalman): when does this happen?
+               Feature::CONTENT_SCRIPT_CONTEXT
+                     : Feature::UNSPECIFIED_CONTEXT;
+  }
+
+  // We have an explicit check for sandboxed pages before checking whether the
+  // extension is active in this process because:
+  // 1. Sandboxed pages run in the same process as regular extension pages, so
+  //    the extension is considered active.
+  // 2. ScriptContext creation (which triggers bindings injection) happens
+  //    before the SecurityContext is updated with the sandbox flags (after
+  //    reading the CSP header), so the caller can't check if the context's
+  //    security origin is unique yet.
+  if (ScriptContext::IsSandboxedPage(*extensions_, url))
+    return Feature::WEB_PAGE_CONTEXT;
+
+  if (extension && active_extension_ids_->count(extension->id()) > 0) {
+    // |extension| is active in this process, but it could be either a true
+    // extension process or within the extent of a hosted app. In the latter
+    // case this would usually be considered a (blessed) web page context,
+    // unless the extension in question is a component extension, in which case
+    // we cheat and call it blessed.
+    return (extension->is_hosted_app() &&
+            extension->location() != Manifest::COMPONENT)
+               ? Feature::BLESSED_WEB_PAGE_CONTEXT
+               : Feature::BLESSED_EXTENSION_CONTEXT;
+  }
+
+  // TODO(kalman): This isUnique() check is wrong, it should be performed as
+  // part of ScriptContext::IsSandboxedPage().
+  if (!origin.isUnique() && extensions_->ExtensionBindingsAllowed(url)) {
+    if (!extension)  // TODO(kalman): when does this happen?
+      return Feature::UNSPECIFIED_CONTEXT;
+    return extension->is_hosted_app() ? Feature::BLESSED_WEB_PAGE_CONTEXT
+                                      : Feature::UNBLESSED_EXTENSION_CONTEXT;
+  }
+
+  if (!url.is_valid())
+    return Feature::UNSPECIFIED_CONTEXT;
+
+  if (url.SchemeIs(content::kChromeUIScheme))
+    return Feature::WEBUI_CONTEXT;
+
+  return Feature::WEB_PAGE_CONTEXT;
+}
+
+void ScriptContextSet::DispatchOnUnloadEventAndRemove(
+    std::set<ScriptContext*>* out,
+    ScriptContext* context) {
+  Remove(context);  // deleted asynchronously
+  out->insert(context);
 }
 
 }  // namespace extensions

extensions/renderer/script_context_set.h

@@ -8,8 +8,12 @@
 #include <set>
 #include <string>
 
-#include "base/basictypes.h"
-#include "base/bind.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_set.h"
+#include "extensions/common/features/feature.h"
+#include "url/gurl.h"
 #include "v8/include/v8.h"
 
 class GURL;
@@ -18,39 +22,50 @@ namespace base {
 class ListValue;
 }
 
-namespace content {
-class RenderView;
+namespace blink {
+class WebLocalFrame;
+class WebSecurityOrigin;
 }
 
-namespace v8 {
-class Context;
+namespace content {
+class RenderView;
 }
 
 namespace extensions {
 class ScriptContext;
 
-// A container of ExtensionBindingsContext. Since calling JavaScript within a
-// context can cause any number of contexts to be created or destroyed, this
-// has additional smarts to help with the set changing underneath callers.
+// A container of ScriptContexts, responsible for both creating and managing
+// them.
+//
+// Since calling JavaScript within a context can cause any number of contexts
+// to be created or destroyed, this has additional smarts to help with the set
+// changing underneath callers.
 class ScriptContextSet {
  public:
-  ScriptContextSet();
+  ScriptContextSet(
+      ExtensionSet* extensions,
+      // Set of the IDs of extensions that are active in this process.
+      // Must outlive this. TODO(kalman): Combine this and |extensions|.
+      ExtensionIdSet* active_extension_ids);
+
   ~ScriptContextSet();
 
-  int size() const;
+  // Returns the number of contexts being tracked by this set.
+  // This may also include invalid contexts. TODO(kalman): Useful?
+  size_t size() const { return contexts_.size(); }
 
-  // Takes ownership of |context|.
-  void Add(ScriptContext* context);
+  // Creates and starts managing a new ScriptContext. Ownership is held.
+  // Returns a weak reference to the new ScriptContext.
+  ScriptContext* Register(blink::WebLocalFrame* frame,
+                          const v8::Handle<v8::Context>& v8_context,
+                          int extension_group,
+                          int world_id);
 
   // If the specified context is contained in this set, remove it, then delete
   // it asynchronously. After this call returns the context object will still
   // be valid, but its frame() pointer will be cleared.
   void Remove(ScriptContext* context);
 
-  // Returns a copy to protect against changes.
-  typedef std::set<ScriptContext*> ContextSet;
-  ContextSet GetAll() const;
-
   // Gets the ScriptContext corresponding to v8::Context::GetCurrent(), or
   // NULL if no such context exists.
   ScriptContext* GetCurrent() const;
@@ -61,7 +76,7 @@ class ScriptContextSet {
 
   // Gets the ScriptContext corresponding to the specified
   // v8::Context or NULL if no such context exists.
-  ScriptContext* GetByV8Context(v8::Handle<v8::Context> context) const;
+  ScriptContext* GetByV8Context(const v8::Handle<v8::Context>& context) const;
 
   // Synchronously runs |callback| with each ScriptContext that belongs to
   // |extension_id| in |render_view|.
@@ -88,10 +103,39 @@ class ScriptContextSet {
   // Returns the set of ScriptContexts that were removed as a result. These
   // are safe to interact with until the end of the current event loop, since
   // they're deleted asynchronously.
-  ContextSet OnExtensionUnloaded(const std::string& extension_id);
+  std::set<ScriptContext*> OnExtensionUnloaded(const std::string& extension_id);
 
  private:
-  ContextSet contexts_;
+  // Finds the extension for the JavaScript context associated with the
+  // specified |frame| and isolated world. If |world_id| is zero, finds the
+  // extension ID associated with the main world's JavaScript context. If the
+  // JavaScript context isn't from an extension, returns empty string.
+  const Extension* GetExtensionFromFrameAndWorld(
+      const blink::WebLocalFrame* frame,
+      int world_id,
+      bool use_effective_url);
+
+  // Returns the Feature::Context type of context for a JavaScript context.
+  Feature::Context ClassifyJavaScriptContext(
+      const Extension* extension,
+      int extension_group,
+      const GURL& url,
+      const blink::WebSecurityOrigin& origin);
+
+  // Calls Remove on |context| then appends |context| to |out|.
+  // This is a helper designed to be used by OnExtensionUnloaded with ForEach.
+  void DispatchOnUnloadEventAndRemove(std::set<ScriptContext*>* out,
+                                      ScriptContext* context);
+
+  // Weak reference to all installed Extensions.
+  ExtensionSet* extensions_;
+
+  // Weak reference to all installed Extensions that are also active in this
+  // process.
+  ExtensionIdSet* active_extension_ids_;
+
+  // The set of all ScriptContexts we own.
+  std::set<ScriptContext*> contexts_;
 
   DISALLOW_COPY_AND_ASSIGN(ScriptContextSet);
 };

extensions/renderer/script_context_set_unittest.cc

@@ -2,61 +2,63 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <vector>
+
 #include "base/message_loop/message_loop.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/extension_set.h"
 #include "extensions/common/features/feature.h"
 #include "extensions/renderer/script_context.h"
 #include "extensions/renderer/script_context_set.h"
 #include "gin/public/context_holder.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "third_party/WebKit/public/web/WebView.h"
 #include "v8/include/v8.h"
 
 namespace extensions {
 
-TEST(ScriptContextSet, Lifecycle) {
+TEST(ScriptContextSetTest, Lifecycle) {
   base::MessageLoop loop;
 
-  ScriptContextSet context_set;
+  blink::WebView* webview = blink::WebView::create(nullptr);
+  blink::WebLocalFrame* frame = blink::WebLocalFrame::create(nullptr);
+  webview->setMainFrame(frame);
 
+  // Do this after construction of the webview, since it may construct the
+  // Isolate.
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
+
   v8::HandleScope handle_scope(isolate);
+  v8::Local<v8::Context> v8_context = v8::Context::New(isolate);
+  v8::Context::Scope context_scope(v8_context);
+  // ScriptContext relies on gin, it just doesn't look like it from here.
   gin::ContextHolder context_holder(isolate);
-  context_holder.SetContext(v8::Context::New(isolate));
-
-  blink::WebView* webview = blink::WebView::create(nullptr);
-  blink::WebFrame* frame = blink::WebLocalFrame::create(nullptr);
-  webview->setMainFrame(frame);
-  const Extension* extension = NULL;
-  ScriptContext* context =
-      new ScriptContext(context_holder.context(),
-                        frame,
-                        extension,
-                        Feature::BLESSED_EXTENSION_CONTEXT,
-                        extension,
-                        Feature::BLESSED_EXTENSION_CONTEXT);
+  context_holder.SetContext(v8_context);
 
-  context_set.Add(context);
-  EXPECT_EQ(1u, context_set.GetAll().count(context));
-  EXPECT_EQ(context, context_set.GetByV8Context(context->v8_context()));
+  ExtensionSet extensions;
+  ExtensionIdSet active_extensions;
+  ScriptContextSet context_set(&extensions, &active_extensions);
+  ScriptContext* context = context_set.Register(
+      frame, v8_context, 0, 0);  // no extension group or world ID
 
-  // Adding the same item multiple times should be OK and deduped.
-  context_set.Add(context);
-  EXPECT_EQ(1u, context_set.GetAll().count(context));
+  // Context is valid and resembles correctness.
+  EXPECT_TRUE(context->is_valid());
+  EXPECT_EQ(frame, context->web_frame());
+  EXPECT_EQ(v8_context, context->v8_context());
 
-  // GetAll() returns a copy so removing from one should not remove from others.
-  ScriptContextSet::ContextSet set_copy = context_set.GetAll();
-  EXPECT_EQ(1u, set_copy.count(context));
+  // Context has been correctly added.
+  EXPECT_EQ(1u, context_set.size());
+  EXPECT_EQ(context, context_set.GetByV8Context(v8_context));
 
+  // Test context is correctly removed.
   context_set.Remove(context);
-  EXPECT_EQ(0, context_set.size());
-  EXPECT_FALSE(context_set.GetByV8Context(context->v8_context()));
-  EXPECT_EQ(1u, set_copy.size());
+  EXPECT_EQ(0u, context_set.size());
+  EXPECT_EQ(nullptr, context_set.GetByV8Context(v8_context));
 
-  // After removal, the context should be marked for destruction.
-  EXPECT_FALSE(context->web_frame());
+  // After removal, the context should be invalid.
+  EXPECT_FALSE(context->is_valid());
+  EXPECT_EQ(nullptr, context->web_frame());
 
   // Run loop to do the actual deletion.
   loop.RunUntilIdle();