Bug 27785 – [V8] [two-sided] Add a way to register V8 extensions for Isolated Worlds only

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Matt Perry]

Right now we can only register for all types of contexts (frames and isolated worlds).  We need to be able to control both separately.

[Matt Perry]

Created attachment 33678 [details]
first attempt

There is a corresponding Chromium change here: http://codereview.chromium.org/159542

[Adam Barth]

Comment on attachment 33678 [details]
first attempt

+        } else if (it->scheme.length() > 0 && (it->scheme != m_frame->loader()->activeDocumentLoader()->url().protocol() || it->scheme != m_frame->page()->mainFrame()->loader()->activeDocumentLoader()->url().protocol())) {

You don't need the "else" here because the "if" clause ends in continue.

+        v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object> global, bool isolatedContext);

Adding bools to this function doesn't really scale.  Can we pass in the set of extensions somehow?  You might want a helper function that grabs the proper set of extensions for normal and for isolated worlds.

Also, there's is no such thing as an "isolated context".  There are isolated worlds, which eventually will hold more than one context each.

[Matt Perry]

(In reply to comment #2)
> +        v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object>
> global, bool isolatedContext);
> 
> Adding bools to this function doesn't really scale.  Can we pass in the set of
> extensions somehow?  You might want a helper function that grabs the proper set
> of extensions for normal and for isolated worlds.

Doesn't scale how? Do you foresee many other parameters being added here?

> Also, there's is no such thing as an "isolated context".  There are isolated
> worlds, which eventually will hold more than one context each.

I've been using "isolated context" as a generic term for contexts created by either "evaluateInNewContext" or "evaluateInNewWorld". Do you have a suggestion for a better term?

[Adam Barth]

(In reply to comment #3)
> (In reply to comment #2)
> > +        v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object>
> > global, bool isolatedContext);
> > 
> > Adding bools to this function doesn't really scale.  Can we pass in the set of
> > extensions somehow?  You might want a helper function that grabs the proper set
> > of extensions for normal and for isolated worlds.
> 
> Doesn't scale how? Do you foresee many other parameters being added here?

There seems to be room for expansion in the rest of the patch:

+ enum IsolatedContextTag { IsolatedContextOnly };

struct V8ExtensionInfo {
    String scheme;
    bool isolatedContextOnly;
    v8::Extension* extension;
};

What if we want to add extensions only for contexts created with certain permissions?  etc.

> > Also, there's is no such thing as an "isolated context".  There are isolated
> > worlds, which eventually will hold more than one context each.
> 
> I've been using "isolated context" as a generic term for contexts created by
> either "evaluateInNewContext" or "evaluateInNewWorld". Do you have a suggestion
> for a better term?

evaluateInNewContext will eventually be removed.  We're keeping it around in case we need to audible back to the old design for some reason.

[Matt Perry]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > +        v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object>
> > > global, bool isolatedContext);
> > > 
> > > Adding bools to this function doesn't really scale.  Can we pass in the set of
> > > extensions somehow?  You might want a helper function that grabs the proper set
> > > of extensions for normal and for isolated worlds.
> > 
> > Doesn't scale how? Do you foresee many other parameters being added here?
> 
> There seems to be room for expansion in the rest of the patch:
> 
> + enum IsolatedContextTag { IsolatedContextOnly };

That was more of a way to use the type system to register an extension for a different purpose.

> struct V8ExtensionInfo {
>     String scheme;
>     bool isolatedContextOnly;
>     v8::Extension* extension;
> };
> 
> What if we want to add extensions only for contexts created with certain
> permissions?  etc.

True.  I had another idea that would have addressed this use case.  What about:

  registerExtension(v8::Extension);
  registerExtension(v8::Extension, int contextTypeBitfield);
  evaluateInNewWorld(int contextType);

and then the consumer could group contexts as he pleases.  For content scripts, we would do:

  registerExtension(foo, CONTENT_SCRIPT_CONTEXT);
  registerExtension(bar, CONTENT_SCRIPT_CONTEXT | SOME_OTHER_TYPE);
  registerExtension(baz, SOME_OTHER_TYPE);
  ...
  evaluateInNewWorld(CONTENT_SCRIPT_CONTEXT);  // would enable foo and bar

Thoughts?

[Adam Barth]

> Thoughts?

That seems better because it doesn't tie the isolated world mechanism directly to the extension system.  For example, you could imagine the Web Inspector using an isolated world and not wanting to see the v8extensions appropriate for content scripts.

We might be over engineering.  Perhaps you're right and we should use the minimal mechanism for our current use cases.

[Matt Perry]

(In reply to comment #6)
> > Thoughts?
> 
> We might be over engineering.  Perhaps you're right and we should use the
> minimal mechanism for our current use cases.

That's why I changed my mind and eventually settled on the current patch :).

[Matt Perry]

(In reply to comment #5)
> True.  I had another idea that would have addressed this use case.  What about:
> 
>   registerExtension(v8::Extension);
>   registerExtension(v8::Extension, int contextTypeBitfield);
>   evaluateInNewWorld(int contextType);

I'm going to give this approach a shot. The only weirdness is that the embedder (chromium) would be the one to define the bitfields. That means webkit can't use the bitfield feature on its own, or would have to reserve a bit range.

[Matt Perry]

Created attachment 33733 [details]
rework of above using an extensionGroup tag

Here's the rework using a tag for contexts. You can see how it's used in the chromium code review I linked earlier.

[Adam Barth]

Comment on attachment 33733 [details]
rework of above using an extensionGroup tag

This looks good.  The below are only minor style issues:

1) Sometimes we call the value extensionGroup and sometimes extensionFlags.  We should probably make this consistent.

2) We should put the parameter name in the headers.  Having a bare "int" is very ambiguous.

Thanks!

[Matt Perry]

Created attachment 33734 [details]
fixed up the naming

[Adam Barth]

Comment on attachment 33734 [details]
fixed up the naming

Great.  Thanks.

[Adam Barth]

I believe this requires a simultaneous downstream change during landing.

[Matt Perry]

Committed http://trac.webkit.org/changeset/46594.