Bug 148136 - Add InternalPromise to use Promises safely in the internals
Summary: Add InternalPromise to use Promises safely in the internals
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on: 148118
Blocks:
  Show dependency treegraph
 
Reported: 2015-08-18 12:36 PDT by Yusuke Suzuki
Modified: 2015-08-19 21:43 PDT (History)
2 users (show)

See Also:


Attachments
Patch (78.20 KB, patch)
2015-08-18 22:32 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (78.13 KB, patch)
2015-08-18 22:39 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-08-18 12:36:35 PDT
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.
Comment 1 Yusuke Suzuki 2015-08-18 22:32:27 PDT
Created attachment 259357 [details]
Patch
Comment 2 Yusuke Suzuki 2015-08-18 22:39:29 PDT
Created attachment 259358 [details]
Patch
Comment 3 Yusuke Suzuki 2015-08-18 22:55:45 PDT
Module Loader spec requires the use of the Promise internally.
So we need some user-non-trappable Promises.
Comment 4 Yusuke Suzuki 2015-08-18 22:57:02 PDT
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.
Comment 5 Yusuke Suzuki 2015-08-19 00:28:07 PDT
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.
Comment 6 Yusuke Suzuki 2015-08-19 01:21:39 PDT
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.
Comment 7 Yusuke Suzuki 2015-08-19 11:33:19 PDT
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.
Comment 8 Yusuke Suzuki 2015-08-19 20:22:51 PDT
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.
Comment 9 Saam Barati 2015-08-19 20:43:00 PDT
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"
Comment 10 Yusuke Suzuki 2015-08-19 21:07:23 PDT
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.
Comment 11 Yusuke Suzuki 2015-08-19 21:43:31 PDT
Committed r188681: <http://trac.webkit.org/changeset/188681>