Bug 159357 - ExecState should be passed by reference in JS bindings generator for custom constructors
Summary: ExecState should be passed by reference in JS bindings generator for custom c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nael Ouedraogo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-01 10:09 PDT by Nael Ouedraogo
Modified: 2016-07-08 08:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch (32.57 KB, patch)
2016-07-01 10:23 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (38.46 KB, patch)
2016-07-04 04:01 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (38.64 KB, patch)
2016-07-04 05:10 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (51.44 KB, patch)
2016-07-07 07:44 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (764.09 KB, application/zip)
2016-07-07 08:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (879.23 KB, application/zip)
2016-07-07 08:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.38 MB, application/zip)
2016-07-07 08:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (569.61 KB, application/zip)
2016-07-07 08:49 PDT, Build Bot
no flags Details
Patch (39.86 KB, patch)
2016-07-08 01:35 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch for landing (39.80 KB, patch)
2016-07-08 08:24 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nael Ouedraogo 2016-07-01 10:09:54 PDT
ExecState should be passed by reference in JS bindings generator for custom constructors.
Comment 1 Nael Ouedraogo 2016-07-01 10:23:01 PDT
Created attachment 282560 [details]
Patch
Comment 2 Nael Ouedraogo 2016-07-04 04:01:44 PDT
Created attachment 282704 [details]
Patch
Comment 3 Nael Ouedraogo 2016-07-04 05:10:18 PDT
Created attachment 282709 [details]
Patch
Comment 4 youenn fablet 2016-07-04 07:52:54 PDT
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?
Comment 5 Brady Eidson 2016-07-04 08:21:42 PDT
> 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.
Comment 6 Nael Ouedraogo 2016-07-07 07:44:34 PDT
Created attachment 283009 [details]
Patch
Comment 7 Nael Ouedraogo 2016-07-07 07:46:49 PDT
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 ?
Comment 8 Build Bot 2016-07-07 08:39:11 PDT
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
Comment 9 Build Bot 2016-07-07 08:39:14 PDT
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
Comment 10 Build Bot 2016-07-07 08:41:16 PDT
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
Comment 11 Build Bot 2016-07-07 08:41:20 PDT
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
Comment 12 Build Bot 2016-07-07 08:49:01 PDT
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
Comment 13 Build Bot 2016-07-07 08:49:05 PDT
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
Comment 14 Build Bot 2016-07-07 08:49:24 PDT
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
Comment 15 Build Bot 2016-07-07 08:49:27 PDT
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
Comment 16 Nael Ouedraogo 2016-07-07 10:28:39 PDT
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?
Comment 17 youenn fablet 2016-07-07 11:40:10 PDT
(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.
Comment 18 Nael Ouedraogo 2016-07-08 01:35:10 PDT
Created attachment 283123 [details]
Patch
Comment 19 Nael Ouedraogo 2016-07-08 02:22:04 PDT
(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.
Comment 20 youenn fablet 2016-07-08 06:54:33 PDT
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.
Comment 21 Nael Ouedraogo 2016-07-08 08:24:15 PDT
Created attachment 283163 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2016-07-08 08:53:37 PDT
Comment on attachment 283163 [details]
Patch for landing

Clearing flags on attachment: 283163

Committed r202981: <http://trac.webkit.org/changeset/202981>
Comment 23 WebKit Commit Bot 2016-07-08 08:53:42 PDT
All reviewed patches have been landed.  Closing bug.