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.
Created attachment 259357 [details] Patch
Created attachment 259358 [details] Patch
Module Loader spec requires the use of the Promise internally. So we need some user-non-trappable Promises.
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 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 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 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 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 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 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.
Committed r188681: <http://trac.webkit.org/changeset/188681>