Remove extension group from DOMWrapperWorld.

extensions/renderer/script_context_set.cc

Index: extensions/renderer/script_context_set.cc
diff --git a/extensions/renderer/script_context_set.cc b/extensions/renderer/script_context_set.cc
index 8f60dc9d598cea4713a40828deb3f09dc88f4404..06710fd9f6bc5b28d2d8b9e646b563c95ece25cc 100644
--- a/extensions/renderer/script_context_set.cc
+++ b/extensions/renderer/script_context_set.cc
@@ -10,7 +10,6 @@
 #include "content/public/common/url_constants.h"
 #include "content/public/renderer/render_frame.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"
@@ -37,7 +36,6 @@ ScriptContextSet::~ScriptContextSet() {
 ScriptContext* ScriptContextSet::Register(
     blink::WebLocalFrame* frame,
     const v8::Local<v8::Context>& v8_context,
-    int extension_group,
     int world_id) {
   const Extension* extension =
       GetExtensionFromFrameAndWorld(frame, world_id, false);
@@ -45,11 +43,10 @@ ScriptContext* ScriptContextSet::Register(
       GetExtensionFromFrameAndWorld(frame, world_id, true);
 
   GURL frame_url = ScriptContext::GetDataSourceURLForFrame(frame);
-  Feature::Context context_type =
-      ClassifyJavaScriptContext(extension, extension_group, frame_url,
-                                frame->document().getSecurityOrigin());
+  Feature::Context context_type = ClassifyJavaScriptContext(
+      extension, world_id, frame_url, frame->document().getSecurityOrigin());
   Feature::Context effective_context_type = ClassifyJavaScriptContext(
-      effective_extension, extension_group,
+      effective_extension, world_id,
       ScriptContext::GetEffectiveDocumentURL(frame, frame_url, true),
       frame->document().getSecurityOrigin());
 
@@ -171,14 +168,15 @@ const Extension* ScriptContextSet::GetExtensionFromFrameAndWorld(
 
 Feature::Context ScriptContextSet::ClassifyJavaScriptContext(
     const Extension* extension,
-    int extension_group,
+    int world_id,
     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) {
+  // The main world ID is 0. Non-zero world IDs should indicate worlds created
+  // for content script.

[dcheng, 2017/01/12 01:39:52]

This is the only real functional change in this CL. I'm not 100% sure this
statement is always true, but all the tests seem OK with it.

[Devlin, 2017/01/12 16:30:12]

This isn't quite correct - instead, it should be:
if (world_id >= ExtensionsRendererClient::Get()->GetLowestIsolatedWorldId())

Otherwise, we'll categorize translate and chrome-internal (dom distiller)
isolated worlds as content script contexts.

It would be an interesting test to make sure that doesn't happen, but I'm not
sure what the best way to do that would be...

[dcheng, 2017/01/12 18:35:12]

Looking at this, I'm not actually sure what the right behavior is.

Before we call ClassifyJavascriptContext(), we call
GetExtensionFromFrameAndWorld().

The implementation of GetExtensionFromFrameAndWorld() also checks that world id
!= 0. If != 0, it assumes content script and tries to look up an extension. I
suppose that if it's for a chrome internal world, the extension lookup would
fail because it simply won't be in the map.

Then we end up here, and the logic nicely lines up -- if world_id != 0 but we
have no extension, we'll end up with an UNSPECIFIED_CONTEXT... I think. So I
guess I can remove kalman's TODO here and update the comment... but I think this
logic might actually be (unintentionally) right.

[Devlin, 2017/01/12 21:31:39]

So, a couple of things here.

Unspecified contexts should be avoided.  They're cop-outs. :)  For our sake, we
only care about extension-related contexts and webui, so if it's not one of
those, it should be considered WEB_PAGE_CONTEXT.  Previously, this logic said
UNSPECIFIED because we had one piece of information (extension group) saying
"this is extensiony" and another saying "no it's not" - so we threw our hands in
the air.  Here, having a non-zero world id doesn't indicate extension-ness, so
we shouldn't assume it.

GetExtensionFromFrameAndWorld() does the wrong thing in checking 0.  It should
also check minimum isolated world for extensions.  I'll write a patch to change
that.  I even wonder if that could explain some of the "when does this happens?"
in the code.

[dcheng, 2017/01/12 21:40:41]

It could, but I'm not sure the burden of fixing that should be in this patch.

Without fixing that, I'm still going to need to use UNSPECIFIED_CONTEXT here.
Currently, we (incorrectly it sounds like) don't early return if it's not
actually an extension context, so we *must* do something here.

[Devlin, 2017/01/12 22:02:09]

There is a behavior change from the current behavior in this patch.

currently:
bool do_we_think_this_is_an_extension = extension_group ==
EXTENSION_GROUP_CONTENT_SCRIPTS;

with this patch:
bool do_we_think_this_is_an_extension = world_id != 0;

However, if we have a context for e.g. translate or chrome internal, this new
version will classify that as UNSPECIFIED_CONTEXT - because world_id != 0, and
there's no |extension|.  Previously (before this patch), it would classify this
as WEB_PAGE_CONTEXT, because the extension_group would not be
EXTENSION_GROUP_CONTENT_SCRIPTS.

Does that make sense?  Feel free to ping with questions. :)

+  if (world_id != 0) {
     return extension ?  // TODO(kalman): when does this happen?
                Feature::CONTENT_SCRIPT_CONTEXT
                      : Feature::UNSPECIFIED_CONTEXT;