Bug 159298 – toNative functions in JSDOMBinding.h should take an ExecState reference instead of pointer

• Component: Bindings
• Status: RESOLVED FIXED

[Nael Ouedraogo]

Modify toNative functions in JSDOMBinding.h to take an ExecState reference instead of pointer.

[Nael Ouedraogo]

Created attachment 282435 [details]
Patch

[Nael Ouedraogo]

Created attachment 282438 [details]
Patch

[Nael Ouedraogo]

This is a preliminary patch for Bug 158835 (c.f. Comment #10).

[Brady Eidson]

Comment on attachment 282438 [details]
Patch

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

> Source/WebCore/bindings/js/JSBlobCustom.cpp:75
> -    JSObject* blobParts = toJSSequence(exec, exec->argument(0), blobPartsLength);
> +    JSObject* blobParts = toJSSequence(*exec, exec->argument(0), blobPartsLength);

There's no null check for exec in this method, so this seems bogus.

> Source/WebCore/bindings/js/JSDOMBinding.h:480
> -inline JSC::JSObject* toJSSequence(JSC::ExecState* exec, JSC::JSValue value, unsigned& length)
> +inline JSC::JSObject* toJSSequence(JSC::ExecState& state, JSC::JSValue value, unsigned& length)

I'm not a fan of the rename.

We historically use "exec", "execState" and "state" for execState variables.

While we would ideally choose one and stick with it, I think "state" is the worse because it's the most generic and vague.

Please leave it as "exec" or change to "execState" if you don't like exec for some reason.

> Source/WebCore/bindings/js/JSDOMBinding.h:663
> -    static inline bool nativeValue(JSC::ExecState* exec, JSC::JSValue jsValue, String& indexedValue)
> +    static inline bool nativeValue(JSC::ExecState& state, JSC::JSValue jsValue, String& indexedValue)

Same comment on the variable rename here, and everywhere else you might do it.

> Source/WebCore/bindings/js/JSDictionary.cpp:156
> -    JSObject* object = toJSSequence(exec, value, length);
> +    JSObject* object = toJSSequence(*exec, value, length);

There's no null check for exec in this method, so this seems bogus.

> Source/WebCore/bindings/js/JSDictionary.cpp:219
> -    JSObject* object = toJSSequence(exec, value, length);
> +    JSObject* object = toJSSequence(*exec, value, length);

There's no null check for exec in this method, so this seems bogus.

> Source/WebCore/bindings/js/JSDictionary.cpp:287
> -    JSObject* object = toJSSequence(exec, value, length);
> +    JSObject* object = toJSSequence(*exec, value, length);

There's no null check for exec in this method, so this seems bogus.

> Source/WebCore/bindings/js/JSFileCustom.cpp:55
> -    JSObject* blobParts = toJSSequence(exec, arg, blobPartsLength);
> +    JSObject* blobParts = toJSSequence(*exec, arg, blobPartsLength);

There's no null check for exec in this method, so this seems bogus.

Let's pretend I make the same comment everywhere you dereference exec and the method does't have a prior null check.

> Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.cpp:458
> -    auto x = toNativeArray<int32_t>(state, state->argument(0));
> +    auto x = toNativeArray<int32_t>(*state, state->argument(0));

And I meant that everywhere, including in "test only" code.

[Brady Eidson]

Note that an eventual goal is that we're never passing ExecState*'s around in bindings code at all. This is an intermediate step along that path. The null checks will eventually be unnecessary.

[youenn fablet]

(In reply to comment #4)
> Comment on attachment 282438 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282438&action=review
> 
> > Source/WebCore/bindings/js/JSBlobCustom.cpp:75
> > -    JSObject* blobParts = toJSSequence(exec, exec->argument(0), blobPartsLength);
> > +    JSObject* blobParts = toJSSequence(*exec, exec->argument(0), blobPartsLength);
> 
> There's no null check for exec in this method, so this seems bogus.

constructJSBlob should actually take an ExecState reference not a pointer.
Migrating to ExecState references needs to start somewhere so this patch seems to go in the right direction to me.
As of the null check, I guess an ASSERT(exec) at the beginning of the function would be good enough.

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #4)
> > Comment on attachment 282438 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=282438&action=review
> > 
> > > Source/WebCore/bindings/js/JSBlobCustom.cpp:75
> > > -    JSObject* blobParts = toJSSequence(exec, exec->argument(0), blobPartsLength);
> > > +    JSObject* blobParts = toJSSequence(*exec, exec->argument(0), blobPartsLength);
> > 
> > There's no null check for exec in this method, so this seems bogus.
> 
> constructJSBlob should actually take an ExecState reference not a pointer.
> Migrating to ExecState references needs to start somewhere 

That was the crux of https://bugs.webkit.org/show_bug.cgi?id=159298#c5

> so this patch seems to go in the right direction to me.

Sure, me too.

> As of the null check, I guess an ASSERT(exec) at the beginning of the
> function would be good enough.

Let's pretend that Nael lands this patch, then immediately no longer has time to work on this task, and nobody else has time either.

So we're left with this patch in the tree for some time.

In that scenario, I'd rather see a proper null check along with signaling an error condition, wherever that is possible. (There's at least a few places where it is.)

Elsewhere, an ASSERT would be fine.

[youenn fablet]

> > constructJSBlob should actually take an ExecState reference not a pointer.
> > Migrating to ExecState references needs to start somewhere 
> 
> That was the crux of https://bugs.webkit.org/show_bug.cgi?id=159298#c5

FWIW, I used ASSERT() for JS builtin constructors in JSDOMConstructor.h
IIRC, passing an ExecState& to constructors requires changes in JSC. Maybe not complex but probably quite some edit changes to be done in one step.

A good step forward for WebCore might be to update the binding generator to pass an ExecState& for custom constructors. The binding generator would actually generate the ASSERT() just before calling the constructXX function.

Nael, wdyt?

[Nael Ouedraogo]

Created attachment 282559 [details]
Patch

[Nael Ouedraogo]

Thanks for the reviews.

Since the final goal is to pass ExecState reference in bindings code, using ASSERT() as interim solution seems good to me.

In the uploaded patch, I added assertions to check exec is not null when it is dereferenced.
To avoid a lot of modifications in this patch, I added a follow-up bug for the update of JS binding code generator for Custom Constructor (Bug 159357).
I will update this patch once Bug 159357 is resolved.

[Nael Ouedraogo]

Created attachment 283317 [details]
Patch

[Nael Ouedraogo]

Bug 159357 is now resolved: ExecState is passed by reference to custom constructors and so null checks are not needed anymore. 

ASSERT() is added in JSDictionary::convertValue() methods which use toJSequence() to check exec is not null.

[WebKit Commit Bot]

Comment on attachment 283317 [details]
Patch

Clearing flags on attachment: 283317

Committed r203065: <http://trac.webkit.org/changeset/203065>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.