Bug 159357 – ExecState should be passed by reference in JS bindings generator for custom constructors

• Component: Bindings
• Status: RESOLVED FIXED

[Nael Ouedraogo]

ExecState should be passed by reference in JS bindings generator for custom constructors.

[Nael Ouedraogo]

Created attachment 282560 [details]
Patch

[Nael Ouedraogo]

Created attachment 282704 [details]
Patch

[Nael Ouedraogo]

Created attachment 282709 [details]
Patch

[youenn fablet]

Comment on attachment 282709 [details]
Patch

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

Seems fine to me. Can you use more uncheckedArguments and toJSNewlyCreated before landing this patch.
For the other points, it might be easier to tackle them as follow-ups.

> Source/WebCore/bindings/js/JSAudioContextCustom.cpp:51
> +        return throwVMError(&exec, createReferenceError(&exec, "AudioContext constructor callee is unavailable"));

Can this really happen? I don't think binding generator is checking that. And neither constructors.
It might be good to evaluate whether an ASSERT is good enough or not and refactor the code accordingly as a later patch.

> Source/WebCore/bindings/js/JSAudioContextCustom.cpp:55
> +        return throwVMError(&exec, createReferenceError(&exec, "AudioContext constructor script execution context is unavailable"));

This error message is not consistent with binding generated code (see for instance JSXMLHttpRequest.cpp) which uses throwConstructorDocumentUnavailableError
Can we use it instead?

> Source/WebCore/bindings/js/JSAudioContextCustom.cpp:72
>          if (!audioContext.get())

Remove .get() ?

> Source/WebCore/bindings/js/JSAudioContextCustom.cpp:85
> +        float sampleRate = exec.argument(2).toFloat(&exec);

Can you use uncheckedArgument here?

> Source/WebCore/bindings/js/JSAudioContextCustom.cpp:109
> +        return throwVMError(&exec, createReferenceError(&exec, "Error creating AudioContext"));

This line is probably only relevant for the else block. Maybe want to move it into that block.

> Source/WebCore/bindings/js/JSBlobCustom.cpp:65
> +    DOMConstructorObject* jsConstructor = jsCast<DOMConstructorObject*>(exec.callee());

We do not check whether jsConstructor is null or not. Might want to add an ASSERT. Related to first point above.

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

Use uncheckedArgument?

> Source/WebCore/bindings/js/JSBlobCustom.cpp:84
> +        JSValue blobPropertyBagValue = exec.argument(1);

Use uncheckedArgument?

> Source/WebCore/bindings/js/JSBlobCustom.cpp:127
> +            String string = item.toString(&exec)->value(&exec);

Maybe use toWTFString?

> Source/WebCore/bindings/js/JSDOMFormDataCustom.cpp:55
> +    return JSValue::encode(asObject(toJS(&exec, jsConstructor->globalObject(), domFormData)));

We should probably use toJSNewlyCreated.

> Source/WebCore/bindings/js/JSDataCueCustom.cpp:56
> +    double startTime(exec.argument(0).toNumber(&exec));

Use uncheckedArgument?

> Source/WebCore/bindings/js/JSDataCueCustom.cpp:60
> +    double endTime(exec.argument(1).toNumber(&exec));

Ditto.

> Source/WebCore/bindings/js/JSDataCueCustom.cpp:70
> +    if (exec.argumentCount() > 3) {

Ditto.

> Source/WebCore/bindings/js/JSDataCueCustom.cpp:77
> +    JSValue valueArgument = exec.argument(2);

Ditto.

> Source/WebCore/bindings/js/JSDataCueCustom.cpp:97
> +        return JSValue::encode(asObject(toJS(&exec, castedThis->globalObject(), object.get())));

We should probably use toJSNewlyCreated in lieu of toJS.

> Source/WebCore/bindings/js/JSDataCueCustom.cpp:104
> +    return JSValue::encode(asObject(toJS(&exec, castedThis->globalObject(), object.get())));

Ditto and remove object variable if possible.

> Source/WebCore/bindings/js/JSFileCustom.cpp:62
> +        return throwVMError(&exec, createTypeError(&exec, "Second argument to File constructor must be a valid string, was undefined"));

I am not sure the error message names are consistent with binding generated code here.

> Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:41
> +EncodedJSValue JSC_HOST_CALL constructJSHTMLElement(ExecState& exec)

Ah, I see you are renaming state into exec :)
I do not have strong feelings on this one but since there is no consensus, it might be best to not change anything here.
Except maybe to make the file consistent on its own.

> Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:43
> +    auto* jsConstructor = jsCast<DOMConstructorObject*>(exec.callee());

Adding ASSERT maybe.

> Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:47
> +        return throwConstructorDocumentUnavailableError(exec, "HTMLElement");

This is what the binding generated code looks like.
Maybe we should use that method in all methods above.

> Source/WebCore/bindings/js/JSMediaSessionCustom.cpp:-44
> -    auto* castedThis = jsCast<DOMConstructorObject*>(exec->callee());

Ditto for an ASSERT

> Source/WebCore/bindings/js/JSMediaSessionCustom.cpp:51
> +        JSString* kindString = exec.argument(0).toString(&exec);

Use uncheckedArgument?

> Source/WebCore/bindings/js/JSMutationObserverCustom.cpp:50
> +    JSObject* object = exec.argument(0).getObject();

uncheckedArgument

> Source/WebCore/bindings/js/JSMutationObserverCustom.cpp:57
> +    JSObject* jsObserver = asObject(toJS(&exec, jsConstructor->globalObject(), MutationObserver::create(WTFMove(callback))));

Probably can use toJSNewlyCreated.

> Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:37
> +EncodedJSValue JSC_HOST_CALL constructJSReadableStreamController(ExecState& exec)

Ditto for exec/state

> Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:46
> +        return throwVMError(&exec, createTypeError(&exec, ASCIILiteral("ReadableStreamReader constructor parameter is not a ReadableStream")));

We could probably make the error message consistent here.

> Source/WebCore/bindings/js/JSWebKitPointCustom.cpp:42
> +        x = static_cast<float>(exec.argument(0).toNumber(&exec));

uncheckedArgument

> Source/WebCore/bindings/js/JSWebKitPointCustom.cpp:49
> +    return JSValue::encode(asObject(toJS(&exec, jsConstructor->globalObject(), WebKitPoint::create(x, y))));

Use toJSNewlyCreated?

> Source/WebCore/bindings/js/JSWorkerCustom.cpp:70
> +    return JSValue::encode(asObject(toJS(&exec, jsConstructor->globalObject(), *worker)));

Use toJSNewlyCreated?

[Brady Eidson]

> Ah, I see you are renaming state into exec :)
> I do not have strong feelings on this one but since there is no consensus, it might be best to not change anything here.
Except maybe to make the file consistent on its own.

While there is no formal consensus, as do have strong feelings about this.

State is a poor variable name in general. Either "exec" or "execState" is better.

I support the rename in places touched for any other reason, even if it leaves the file inconsistent.

If we decide to formalize a style on it, we can do the rest of the project in one big shot.

[Nael Ouedraogo]

Created attachment 283009 [details]
Patch

[Nael Ouedraogo]

Thanks for the reviews.

I uploaded a new patch with the modifications proposed in the reviews.

The patch is bigger and modifies tests and expectations. Is it OK to land this patch as this or should it be split in two smaller patches?
The new patch would address only error messages consistency in custom constructor and binding generated code.

> State is a poor variable name in general. Either "exec" or "execState" is better.
It seems that ExecState handle execution context. Is "context" or "ctx" or "execCtx" better ?

[Build Bot]

Comment on attachment 283009 [details]
Patch

Attachment 283009 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1641515

New failing tests:
storage/indexeddb/modern/blob-simple-workers.html
http/tests/websocket/tests/hybi/workers/send-blob.html
fast/files/workers/worker-read-blob-async.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob-worker.html
fast/files/workers/worker-read-blob-sync.html

[Build Bot]

Created attachment 283012 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283009 [details]
Patch

Attachment 283009 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1641519

New failing tests:
storage/indexeddb/modern/blob-simple-workers.html
http/tests/websocket/tests/hybi/workers/send-blob.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob-worker.html

[Build Bot]

Created attachment 283013 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283009 [details]
Patch

Attachment 283009 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1641521

New failing tests:
storage/indexeddb/modern/blob-simple-workers.html
http/tests/websocket/tests/hybi/workers/send-blob.html
fast/files/workers/worker-read-blob-async.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob-worker.html
fast/files/workers/worker-read-blob-sync.html

[Build Bot]

Created attachment 283014 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283009 [details]
Patch

Attachment 283009 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1641522

New failing tests:
http/tests/websocket/tests/hybi/workers/send-blob.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob-worker.html

[Build Bot]

Created attachment 283016 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Nael Ouedraogo]

Failed tests seem due to the fact that ScriptExecutionContext associated to JSBlob is not always a Document. Adding the possibility for ScriptExecutionContext to be a WorkerGlobalScope seems to fix test errors. 

I'm not very familiar with ScriptExecutionContext. Should other cases be considered?

[youenn fablet]

(In reply to comment #16)
> Failed tests seem due to the fact that ScriptExecutionContext associated to
> JSBlob is not always a Document. Adding the possibility for
> ScriptExecutionContext to be a WorkerGlobalScope seems to fix test errors. 
> 
> I'm not very familiar with ScriptExecutionContext. Should other cases be
> considered?

Blob is exposed both to worker and regular window.
ScriptExecutionContext is an abstraction to cover both cases.
You should be safe covering these two cases.

[Nael Ouedraogo]

Created attachment 283123 [details]
Patch

[Nael Ouedraogo]

(In reply to comment #17)
> Blob is exposed both to worker and regular window.
> ScriptExecutionContext is an abstraction to cover both cases.
> You should be safe covering these two cases.
Ok, thanks. I will fix this in a follow-up bug as you proposed in your first comment.

Modifications made in the uploaded patch consists in more uses of toJSNewlyCreated and uncheckedArguments and in making ExecState* parameter name more consistent in JSDOMFormDataCustom.cpp.

[youenn fablet]

Comment on attachment 283123 [details]
Patch

R=me.

Sounds good to split the issues in different bugs.
A couple of nits below.

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

> Source/WebCore/bindings/js/JSDOMFormDataCustom.cpp:55
> +    return JSValue::encode(asObject(toJSNewlyCreated(&exec, jsConstructor->globalObject(), WTFMove(domFormData))));

No need for asObject here.

> Source/WebCore/bindings/js/JSDataCueCustom.cpp:93
> +        return JSValue::encode(asObject(toJSNewlyCreated(&exec, castedThis->globalObject(), Ref<TextTrackCue>(DataCue::create(*context, MediaTime::createWithDouble(startTime), MediaTime::createWithDouble(endTime), *data, type)))));

Ditto here.
You might want to use CREATE_DOM_WRAPPER to remove the cast.

> Source/WebCore/bindings/js/JSDataCueCustom.cpp:99
> +    return JSValue::encode(asObject(toJSNewlyCreated(&exec, castedThis->globalObject(), Ref<TextTrackCue>(DataCue::create(*context, MediaTime::createWithDouble(startTime), MediaTime::createWithDouble(endTime), valueArgument, type)))));

Ditto

> Source/WebCore/bindings/js/JSMediaSessionCustom.cpp:61
> +    return JSValue::encode(asObject(toJSNewlyCreated(&exec, castedThis->globalObject(), MediaSession::create(*context, kind))));

Remove asObject is probably not needed.

> Source/WebCore/bindings/js/JSWorkerCustom.cpp:55
> +    String scriptURL = exec.argument(0).toWTFString(&exec);

uncheckedArgument here probably.

[Nael Ouedraogo]

Created attachment 283163 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 283163 [details]
Patch for landing

Clearing flags on attachment: 283163

Committed r202981: <http://trac.webkit.org/changeset/202981>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.