Bug 106800 - [V8] Make an Isolate parameter mandatory in SerializedScriptValue methods
Summary: [V8] Make an Isolate parameter mandatory in SerializedScriptValue methods
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-14 09:00 PST by Kentaro Hara
Modified: 2014-12-16 00:48 PST (History)
3 users (show)

See Also:


Attachments
Patch (9.19 KB, patch)
2013-01-14 09:01 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2013-01-14 09:00:10 PST
SerializedScriptValue methods are used by both V8 bindings and WebCore. So they should support both an Isolate-version method and an non-Isolate version method.

There are two ways to accomplish that:

[1] Use an optional Isolate parameter.

[2] Implement two versions and delegate the non-Isolate version method to the Isolate version method.

I would prefer the approach [2], because I'd like to statically make sure that we never passe a 0 Isolate. If we take the approach [1], we need to insert ASSERT(isolate) here and there.
Comment 1 Kentaro Hara 2013-01-14 09:01:45 PST
Created attachment 182588 [details]
Patch
Comment 2 Adam Barth 2013-01-14 09:28:09 PST
Comment on attachment 182588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182588&action=review

Ok, but we should be able to remove the non-isolate version of many of these methods once we've updated all the callers.  (Notice that the JSC versions pass an ExecState.)

> Source/WebCore/ChangeLog:10
> +        SerializedScriptValue methods are used by both V8 bindings
> +        and WebCore. So they should support both an Isolate-version
> +        method and an non-Isolate version method.

The JSC version needs an ExecState, which plays a similar roll to the isolate in the V8 version.

> Source/WebCore/ChangeLog:20
> +        make sure that we never passe a 0 Isolate. If we take the approach

passe -> pass
Comment 3 Kentaro Hara 2013-01-15 05:52:43 PST
Comment on attachment 182588 [details]
Patch

Landed in r139735.
Comment 4 Kentaro Hara 2013-01-15 06:05:40 PST
(In reply to comment #2)
> Ok, but we should be able to remove the non-isolate version of many of these methods once we've updated all the callers.  (Notice that the JSC versions pass an ExecState.)

Yes. Given that these methods can be called from WebCore with a ScriptState parameter, one way to solve the problem would be to implement a mechanism to retrieve an Isolate from a ScriptState. JSC already implemented it (i.e. toJS(toRef(ScriptState*) gives us an ExecState).
Comment 5 Adam Barth 2013-01-15 12:22:12 PST
Yeah, I think we're going to want to keep the Isolate in the ScriptState for a bunch of other cases as well.
Comment 6 Brian Burg 2014-12-16 00:48:15 PST
Closing some V8-related work items.