Bug 148136 – Add InternalPromise to use Promises safely in the internals

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

https://bugs.webkit.org/show_bug.cgi?id=147942 solve the part of the problem.
But, since resolving function always looks up "then" function, by setting malicious "then" function, we can still trap it...
To make Promises safe, we'll introduce new constructor/prototype set for InternalPromise. That is the separated instance from the usual Promise.

[Yusuke Suzuki]

Created attachment 259357 [details]
Patch

[Yusuke Suzuki]

Created attachment 259358 [details]
Patch

[Yusuke Suzuki]

Module Loader spec requires the use of the Promise internally.
So we need some user-non-trappable Promises.

[Yusuke Suzuki]

Comment on attachment 259358 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:29
> +        to the user space.

The largest concern is this.

[Yusuke Suzuki]

Comment on attachment 259358 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:29
>> +        to the user space.
> 
> The largest concern is this.

One mitigation for the leak problem is

1. freezing InternalPromise.prototype
2. prevent InternalPromise instance extension

But they raise performance concerns I guess.

[Yusuke Suzuki]

Comment on attachment 259358 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        This patch implements InternalPromise.

First, I planed to fix the issue by adding @promiseInternal flag to the existing Promise instance to switch the behavior of the Promise operations.
This will make the modification small. However, if we take the above way, it breaks the Promises implementation's one-on-one correspondence to the spec.
And when the spec is changed, we need to change the existing code so much carefully not to break the internal promise system. I think that's not good.

So I changed my mind. That's why I introduced completely separately instantiated InternalPromises in this patch.

[Yusuke Suzuki]

Comment on attachment 259358 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSInternalPromiseDeferred.h:49
> +    JSInternalPromise* promise() const;

I'll add JS_EXPORT_PRIVATE later.

[Yusuke Suzuki]

Comment on attachment 259358 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:21
> +        the returned promise by "then" method, we construct the chain by executing `p2.then(resolving function)`.

Misleading, "the returned promise by "then" method" means `p3 = p1.then(...)`'s p3.

[Saam Barati]

Comment on attachment 259358 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSInternalPromiseConstructor.cpp:38
> +STATIC_ASSERT_IS_TRIVIALLY_DESTRUCTIBLE(JSInternalPromiseConstructor);

Nit: Why not have this in below namespace?

> Source/JavaScriptCore/runtime/JSPromise.h:52
> +    // This may raises the JS exception.

"raises the" => "raise a"

[Yusuke Suzuki]

Comment on attachment 259358 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/runtime/JSInternalPromiseConstructor.cpp:38
>> +STATIC_ASSERT_IS_TRIVIALLY_DESTRUCTIBLE(JSInternalPromiseConstructor);
> 
> Nit: Why not have this in below namespace?

Originally, this namespace is the place to

1. locate `STATIC_ASSERT_IS_TRIVIALLY_DESTRUCTIBLE(JSInternalPromiseConstructor);`
2. locate the forward declarations for C++ JS methods that will become the property of the focused object (like arrayPrototypeFuncJoin etc.)

In the case of this file, there's no C++ JS method. So just (1) remains.
(2) should be locate here because it should be declared before including "JSInternalPromiseConstructor.lut.h" (this can be included in the global namespace), but (1) can be locate after the "JSInternalPromiseConstructor.lut.h". So I'll move this before the JSInternalPromiseConstructor::s_info line.

>> Source/JavaScriptCore/runtime/JSPromise.h:52
>> +    // This may raises the JS exception.
> 
> "raises the" => "raise a"

Thanks! Fixed.

[Yusuke Suzuki]

Committed r188681: <http://trac.webkit.org/changeset/188681>