Revert of Move Extension ScriptContext creation into ScriptContextSet....

Revert of Move Extension ScriptContext creation into ScriptContextSet. (patchset #4 id:60001 of https://codereview.chromium.org/995283004/)

Reason for revert:
Memory failure in ScriptContextTest.GinRunnerLifetime.

Original issue's description:
> 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
>
> Committed: https://crrev.com/75605c7fca774b95984ba67f70ba86bb5750c0f6
> Cr-Commit-Position: refs/heads/master@{#320602}
>
> Committed: https://crrev.com/52f4caf6d01302ebf42136b42fa77d1c67d93ae9
> Cr-Commit-Position: refs/heads/master@{#320770}

TBR=rockot@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=466373

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

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

chrome/renderer/extensions/webstore_bindings.cc

@@ -14,7 +14,6 @@
 #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"
@@ -23,6 +22,7 @@
 
 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;
-  blink::WebLocalFrame* frame = context()->web_frame();
+  WebFrame* frame = context()->web_frame();
 
   if (!GetWebstoreItemIdFromFrame(
       frame, preferred_store_link_url, &webstore_item_id, &error)) {
@@ -102,10 +102,8 @@ void WebstoreBindings::Install(
 
 // static
 bool WebstoreBindings::GetWebstoreItemIdFromFrame(
-    blink::WebLocalFrame* frame,
-    const std::string& preferred_store_link_url,
-    std::string* webstore_item_id,
-    std::string* error) {
+      WebFrame* 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,10 +9,7 @@
 #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"
-
-namespace blink {
-class WebLocalFrame;
-}
+#include "third_party/WebKit/public/web/WebFrame.h"
 
 namespace extensions {
 
@@ -45,10 +42,8 @@ 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::WebLocalFrame* frame,
-      const std::string& preferred_store_link_url,
-      std::string* webstore_item_id,
-      std::string* error);
+      blink::WebFrame* 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,12 +11,10 @@
 #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"
@@ -55,6 +53,7 @@ class RenderThread;
 namespace extensions {
 class ContentWatcher;
 class DispatcherDelegate;
+class Extension;
 class FilteredEventRouter;
 class ManifestPermissionSet;
 class RequestSender;
@@ -77,7 +76,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(); }
@@ -90,6 +89,14 @@ 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,
@@ -235,6 +242,15 @@ 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,
@@ -268,7 +284,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.
-  scoped_ptr<ScriptContextSet> script_context_set_;
+  ScriptContextSet script_context_set_;
 
   scoped_ptr<ContentWatcher> content_watcher_;
 
@@ -284,7 +300,7 @@ class Dispatcher : public content::RenderProcessObserver,
   std::set<std::string> function_names_;
 
   // The extensions and apps that are active in this process.
-  ExtensionIdSet active_extension_ids_;
+  std::set<std::string> 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/WebLocalFrame.h"
+#include "third_party/WebKit/public/web/WebFrame.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/WebLocalFrame.h"
+#include "third_party/WebKit/public/web/WebFrame.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::WebLocalFrame* webframe = context->web_frame())
+  if (blink::WebFrame* webframe = context->web_frame())
     source_url = webframe->document().url();
 
   InsertRequest(request_id, new PendingRequest(name, source,

extensions/renderer/script_context.cc

@@ -13,19 +13,15 @@
 #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"
@@ -79,7 +75,7 @@ class ScriptContext::Runner : public gin::Runner {
 };
 
 ScriptContext::ScriptContext(const v8::Handle<v8::Context>& v8_context,
-                             blink::WebLocalFrame* web_frame,
+                             blink::WebFrame* web_frame,
                              const Extension* extension,
                              Feature::Context context_type,
                              const Extension* effective_extension,
@@ -114,21 +110,6 @@ 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,7 +20,6 @@
 
 namespace blink {
 class WebFrame;
-class WebLocalFrame;
 }
 
 namespace content {
@@ -30,27 +29,18 @@ 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::WebLocalFrame* frame,
+                blink::WebFrame* 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();
@@ -69,7 +59,7 @@ class ScriptContext : public RequestSender::Source {
     return effective_extension_.get();
   }
 
-  blink::WebLocalFrame* web_frame() const { return web_frame_; }
+  blink::WebFrame* web_frame() const { return web_frame_; }
 
   Feature::Context context_type() const { return context_type_; }
 
@@ -124,14 +114,6 @@ 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
@@ -175,9 +157,9 @@ class ScriptContext : public RequestSender::Source {
  private:
   class Runner;
 
-  // The WebLocalFrame associated with this context. This can be NULL because
-  // this object can outlive is destroyed asynchronously.
-  blink::WebLocalFrame* web_frame_;
+  // The WebFrame associated with this context. This can be NULL because this
+  // object can outlive is destroyed asynchronously.
+  blink::WebFrame* 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,50 +5,34 @@
 #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(ExtensionSet* extensions,
-                                   ExtensionIdSet* active_extension_ids)
-    : extensions_(extensions), active_extension_ids_(active_extension_ids) {
+ScriptContextSet::ScriptContextSet() {
 }
-
 ScriptContextSet::~ScriptContextSet() {
 }
 
-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;
+int ScriptContextSet::size() const {
+  return static_cast<int>(contexts_.size());
+}
+
+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);
 }
 
 void ScriptContextSet::Remove(ScriptContext* context) {
@@ -58,25 +42,32 @@ 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())
-                              : nullptr;
+                              : NULL;
 }
 
 ScriptContext* ScriptContextSet::GetCalling() const {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
   v8::Local<v8::Context> calling = isolate->GetCallingContext();
-  return calling.IsEmpty() ? nullptr : GetByV8Context(calling);
+  return calling.IsEmpty() ? NULL : GetByV8Context(calling);
 }
 
 ScriptContext* ScriptContextSet::GetByV8Context(
-    const v8::Handle<v8::Context>& v8_context) const {
-  for (ScriptContext* script_context : contexts_) {
-    if (script_context->v8_context() == v8_context)
-      return script_context;
+    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;
   }
-  return nullptr;
+
+  return NULL;
 }
 
 void ScriptContextSet::ForEach(
@@ -85,9 +76,11 @@ 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.
-  std::set<ScriptContext*> contexts_copy = contexts_;
+  ContextSet contexts = GetAll();
+
+  for (ContextSet::iterator it = contexts.begin(); it != contexts.end(); ++it) {
+    ScriptContext* context = *it;
 
-  for (ScriptContext* context : contexts_copy) {
     // For the same reason as above, contexts may become invalid while we run.
     if (!context->is_valid())
       continue;
@@ -109,103 +102,23 @@ void ScriptContextSet::ForEach(
   }
 }
 
-std::set<ScriptContext*> ScriptContextSet::OnExtensionUnloaded(
+ScriptContextSet::ContextSet ScriptContextSet::OnExtensionUnloaded(
     const std::string& extension_id) {
-  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);
-  }
-
-  // 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;
+  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);
+    }
   }
 
-  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);
+  return removed;
 }
 
 }  // namespace extensions

extensions/renderer/script_context_set.h

@@ -8,12 +8,8 @@
 #include <set>
 #include <string>
 
-#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 "base/basictypes.h"
+#include "base/bind.h"
 #include "v8/include/v8.h"
 
 class GURL;
@@ -22,50 +18,39 @@ namespace base {
 class ListValue;
 }
 
-namespace blink {
-class WebLocalFrame;
-class WebSecurityOrigin;
-}
-
 namespace content {
 class RenderView;
 }
 
+namespace v8 {
+class Context;
+}
+
 namespace extensions {
 class ScriptContext;
 
-// 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.
+// 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.
 class ScriptContextSet {
  public:
-  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();
   ~ScriptContextSet();
 
-  // 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(); }
+  int size() const;
 
-  // 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);
+  // Takes ownership of |context|.
+  void Add(ScriptContext* context);
 
   // 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;
@@ -76,7 +61,7 @@ class ScriptContextSet {
 
   // Gets the ScriptContext corresponding to the specified
   // v8::Context or NULL if no such context exists.
-  ScriptContext* GetByV8Context(const v8::Handle<v8::Context>& context) const;
+  ScriptContext* GetByV8Context(v8::Handle<v8::Context> context) const;
 
   // Synchronously runs |callback| with each ScriptContext that belongs to
   // |extension_id| in |render_view|.
@@ -103,39 +88,10 @@ 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.
-  std::set<ScriptContext*> OnExtensionUnloaded(const std::string& extension_id);
+  ContextSet OnExtensionUnloaded(const std::string& extension_id);
 
  private:
-  // 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_;
+  ContextSet contexts_;
 
   DISALLOW_COPY_AND_ASSIGN(ScriptContextSet);
 };

extensions/renderer/script_context_set_unittest.cc

@@ -2,59 +2,61 @@
 // 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(ScriptContextSetTest, Lifecycle) {
+TEST(ScriptContextSet, Lifecycle) {
   base::MessageLoop loop;
 
-  ExtensionSet extensions;
-  ExtensionIdSet active_extensions;
-  ScriptContextSet context_set(&extensions, &active_extensions);
+  ScriptContextSet context_set;
 
-  blink::WebView* webview = blink::WebView::create(nullptr);
-  blink::WebLocalFrame* frame = blink::WebLocalFrame::create(nullptr);
-  webview->setMainFrame(frame);
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
   v8::HandleScope handle_scope(isolate);
-  // Note: ScriptContext has gin stuff in it, and without a ContextHolder tests
-  // will leak memory.
   gin::ContextHolder context_holder(isolate);
-  v8::Handle<v8::Context> v8_context = v8::Context::New(isolate);
-  v8::Context::Scope context_scope(v8_context);
-  ScriptContext* context = context_set.Register(
-      frame, v8_context, 0, 0);  // no extension group or world ID
+  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_set.Add(context);
+  EXPECT_EQ(1u, context_set.GetAll().count(context));
+  EXPECT_EQ(context, context_set.GetByV8Context(context->v8_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());
+  // Adding the same item multiple times should be OK and deduped.
+  context_set.Add(context);
+  EXPECT_EQ(1u, context_set.GetAll().count(context));
 
-  // Context has been correctly added.
-  EXPECT_EQ(1u, context_set.size());
-  EXPECT_EQ(context, context_set.GetByV8Context(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));
 
-  // Test context is correctly removed.
   context_set.Remove(context);
-  EXPECT_EQ(0u, context_set.size());
-  EXPECT_EQ(nullptr, context_set.GetByV8Context(v8_context));
+  EXPECT_EQ(0, context_set.size());
+  EXPECT_FALSE(context_set.GetByV8Context(context->v8_context()));
+  EXPECT_EQ(1u, set_copy.size());
 
-  // After removal, the context should be invalid.
-  EXPECT_FALSE(context->is_valid());
-  EXPECT_EQ(nullptr, context->web_frame());
+  // After removal, the context should be marked for destruction.
+  EXPECT_FALSE(context->web_frame());
 
   // Run loop to do the actual deletion.
   loop.RunUntilIdle();