Bug 146229 – [ES6] Implement the latest Promise spec in JS

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jordan Harband]

`var count = 0, p = Promise.resolve(Object.defineProperty({}, 'then', { get() { count++; throw new RangeError(); } })); assert(count === 1, 'count'); p.catch(function (err) { assert(err instanceof RangeError, 'error'); assert(false, 'should fail here'); });` should reject synchronously, per http://www.ecma-international.org/ecma-262/6.0/#sec-promise.resolve step 6 which leads to http://www.ecma-international.org/ecma-262/6.0/#sec-promise-resolve-functions step 8 (step 12 is when it becomes asynchronous).

Thus, the promise should reject synchronously, and the first two assertions should be true, and the third should fail.

What happens now in nightly is that the first assertion fails, and that the promise ends up pending - it never resolves. However, the console inspection of p seems to flip back and forth between "pending" and "resolved".

So, I'm not really sure what's going on here - the console inspection stuff isn't specced, so it's just weird, but the assertions should work as described.

[Yusuke Suzuki]

Since the current Promise is based on a little bit old spec, there's no one-on-one correspondence between the spec and the implementation.
Adjusting the implementation to the spec is one solution to solve these problem like this issue.

Sam, what do you think about it? And what do you think about writing the part of Promise implementation in JS builtins?

[Sam Weinig]

(In reply to comment #1)
> Since the current Promise is based on a little bit old spec, there's no
> one-on-one correspondence between the spec and the implementation.
> Adjusting the implementation to the spec is one solution to solve these
> problem like this issue.
> 
> Sam, what do you think about it? 

I think we need to update to match the latest spec.

> And what do you think about writing the part of Promise implementation in JS builtins?

I am in favor of it.

[Yusuke Suzuki]

Created attachment 256118 [details]
WIP, implemented Promise abstract operations in JS

WIP

[Yusuke Suzuki]

Created attachment 256157 [details]
WIP

WIP; implemented. need to add the test for the original reported issue

[Yusuke Suzuki]

Created attachment 256159 [details]
WIP

WIP

[Yusuke Suzuki]

Created attachment 256173 [details]
Patch

[Yusuke Suzuki]

Created attachment 256174 [details]
Patch

[Build Bot]

Comment on attachment 256174 [details]
Patch

Attachment 256174 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4724177877598208

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 256175 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 256174 [details]
Patch

Attachment 256174 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5521457757028352

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 256176 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Yusuke Suzuki]

Created attachment 256182 [details]
Patch

[Sam Weinig]

Comment on attachment 256182 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:24
> +        * CMakeLists.txt:
> +        * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:
> +        * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:
> +        * JavaScriptCore.xcodeproj/project.pbxproj:
> +        * builtins/GlobalObject.js:
> +        (isObject):
> +        (isPromise):
> +        (newPromiseReaction):
> +        (newPromiseDeferred):

This ChangeLog could use some per-function comments.

> Source/JavaScriptCore/builtins/GlobalObject.js:55
> +function isObject(object)
> +{
> +    "use strict";
> +
> +    return (object !== null && typeof object === "object") || typeof object === "function";
> +}

It looks like we are being inconsistent with the naming in this file.  The functions above all start with a capital letter, but these below do not.  Should they be different?  They also have the starting brace on different lines.

> Source/JavaScriptCore/builtins/GlobalObject.js:74
> +function newPromiseReaction(capability, handler)
> +{
> +    "use strict";
> +
> +    return {
> +        @capabilities: capability,
> +        @handler: handler
> +    };
> +}

It seems like it is going to become a problem in the future if we have all the helper functions living in GlobalObject.js. Do we have some way to split it up?

> Source/JavaScriptCore/builtins/Promise.prototype.js:40
> +    // TODO: Fix this code when @@species well-known symbol is landed.

TODO: This should probably be a FIXME.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:33
> +    // TODO: Use @@species.

TODO: This should probably be a FIXME and match the one in then(). Even better, if there is a bug about species support, note it in the FIXME.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:89
> +    // TODO: Use @@species.
> +    var constructor = this;

Same as above.

[Yusuke Suzuki]

Comment on attachment 256182 [details]
Patch

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

Thank you for your review!

>> Source/JavaScriptCore/ChangeLog:24
>> +        (newPromiseDeferred):
> 
> This ChangeLog could use some per-function comments.

Added comments.

>> Source/JavaScriptCore/builtins/GlobalObject.js:55
>> +}
> 
> It looks like we are being inconsistent with the naming in this file.  The functions above all start with a capital letter, but these below do not.  Should they be different?  They also have the starting brace on different lines.

Changed ToLength to toLength. And ToInteger to toInteger.
And adjust the brace rules.

>> Source/JavaScriptCore/builtins/GlobalObject.js:74
>> +}
> 
> It seems like it is going to become a problem in the future if we have all the helper functions living in GlobalObject.js. Do we have some way to split it up?

Yup. I've splitted Promise related abstract operations to Operations.Promise.js.

>> Source/JavaScriptCore/builtins/Promise.prototype.js:40
>> +    // TODO: Fix this code when @@species well-known symbol is landed.
> 
> TODO: This should probably be a FIXME.

Fixed.

>> Source/JavaScriptCore/builtins/PromiseConstructor.js:33
>> +    // TODO: Use @@species.
> 
> TODO: This should probably be a FIXME and match the one in then(). Even better, if there is a bug about species support, note it in the FIXME.

Fixed the comment and noted https://bugs.webkit.org/show_bug.cgi?id=146624

>> Source/JavaScriptCore/builtins/PromiseConstructor.js:89
>> +    var constructor = this;
> 
> Same as above.

Fixed.

[Yusuke Suzuki]

Committed r186298: <http://trac.webkit.org/changeset/186298>

[Yusuke Suzuki]

several tests are failing, I'll fix this.
https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20%28Tests%29/builds/13985/steps/layout-test/logs/stdio

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 256191 [details]
Patch

[Yusuke Suzuki]

Committed r186300: <http://trac.webkit.org/changeset/186300>