Bug 150895 – [Streams API] Shield implementation from user mangling Promise.reject and resolve methods

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

[Streams API] Shield implementation from user mangling Promise.reject and resolve methods

[Xabier Rodríguez Calvar]

Created attachment 264797 [details]
Patch

The idea of this patch is shielding the Streams implementation from any changes that the user can do to the Promise constructor or prototype. This case tackles the touching the constructor resolve and reject functions to create already vended promises. Given that there's other (known) way of getting the original implementation of those functions (prior to the user changing them), we keep them in an internal slot for later usage and in the code we avoid using @Promise.resolve and reject to use @Promise.@resolve and @reject.

[youenn fablet]

Comment on attachment 264797 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:87
> +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().rejectPrivateName(), JSFunction::createBuiltinFunction(vm, promiseConstructorRejectCodeGenerator(vm), globalObject, vm.propertyNames->builtinNames().rejectPrivateName().string()), DontEnum | DontDelete | ReadOnly);

No need for function name. Also, I think you can use JSC_BUILTIN_FUNCTION macro instead.

These private slots are probably not needed for InternalPromise.
We could move this init code to another function, but I think this is ok as is.

> LayoutTests/streams/streams-promises.html:10
>      Promise = function() { throw new Error("nasty things"); };

I would prefer having PromiseBackup inside the test block.
Promise can be set again at the end of the test using try/finally.

Also, instead of "throw new Error(...)", assert_unreached("...") can be used.

> LayoutTests/streams/streams-promises.html:13
>      const ws = new WritableStream(); // Does not throw.

Maybe remove const rs/const ws and the comments as well.

> LayoutTests/streams/streams-promises.html:19
> +    Promise.resolve = function() { throw new Error("nasty resolve things"); };

Ditto for assert_unreached.

> LayoutTests/streams/streams-promises.html:20
> +    Promise.reject = function() { throw new Error("nasty reject things"); };

Promise.reject is probably not tested here.
It would be good to add a specific test for it.

> LayoutTests/streams/streams-promises.html:23
> +    const ws = new WritableStream(); // Does not throw.

Ditto for rs/ws and comments.

We can also set again Promise.resolve and Promise.reject properly at the end of this test.

[Xabier Rodríguez Calvar]

Created attachment 264847 [details]
Patch

Honored Youenn's comments.

[Xabier Rodríguez Calvar]

Created attachment 264850 [details]
Patch

As spoken with Youenn on IRC, I fixed the third test, fixed the comments, improved the error messages and save/restore only the things that we mangle during the given test. I also made that the internal slots are not added to @InternalPromise as suggested for the previous and and improved the test names.

[youenn fablet]

Comment on attachment 264850 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:411
> +    promiseConstructor->addOwnInternalSlots(vm, this);

Can you move the call to addOwnInternalSlots within JSPromiseConstructor::create?

> Source/JavaScriptCore/runtime/JSPromiseConstructor.h:44
> +    void addOwnInternalSlots(VM&, JSGlobalObject*);

Can it be private?

[Xabier Rodríguez Calvar]

Created attachment 264851 [details]
Patch

Made addOwnInternalSlots private.

[WebKit Commit Bot]

Comment on attachment 264851 [details]
Patch

Clearing flags on attachment: 264851

Committed r192057: <http://trac.webkit.org/changeset/192057>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.