Bug 149380 – Promise constructor should throw when not called with "new"

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jordan Harband]

Per http://www.ecma-international.org/ecma-262/6.0/#sec-promise-executor step 1 - if `newTarget` is undefined, a TypeError should be thrown.

Even if `new.target` isn't yet implemented, this can be 99% fixed with the equivalent of the line `if (!(this instanceof Promise)) { throw new TypeError(this + ' is not a Promise'); }`. If there's internal slots you can check, checking those would be cross-realm-safe as well.

This is the sole Promise violation the es6-shim is currently aware of in Safari 9/WebKit Nightly, and when this is fixed, es6-shim won't have to patch it at all.

[Jordan Harband]

Bumping importance since this is a spec violation and nobody's responded yet.

[Yusuke Suzuki]

Personally, I think Blocker means crashing bug.

[Yusuke Suzuki]

I think the ieal way to handle this is,

1. Implement Promise constructor in JS
2. Handles newTarget in JS

But still not implemented these features.
I think implementing the above in C++ is good workaround for now.

[Yusuke Suzuki]

Created attachment 263393 [details]
Patch

[Yusuke Suzuki]

Created attachment 263394 [details]
Patch

[Jordan Harband]

Thanks for fixing the severity, as well as the code patch!

[Build Bot]

Comment on attachment 263394 [details]
Patch

Attachment 263394 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/301949

New failing tests:
js/dom/Promise-types.html

[Build Bot]

Created attachment 263398 [details]
Archive of layout-test-results from ews102 for mac-mavericks

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

[Build Bot]

Comment on attachment 263394 [details]
Patch

Attachment 263394 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/301956

New failing tests:
js/dom/Promise-types.html

[Build Bot]

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

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

[Yusuke Suzuki]

Created attachment 263400 [details]
Patch

[Yusuke Suzuki]

(In reply to comment #6)
> Thanks for fixing the severity, as well as the code patch!

:D

[Build Bot]

Comment on attachment 263400 [details]
Patch

Attachment 263400 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/302071

New failing tests:
js/Promise-types.html

[Build Bot]

Created attachment 263401 [details]
Archive of layout-test-results from ews100 for mac-mavericks

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

[Build Bot]

Comment on attachment 263400 [details]
Patch

Attachment 263400 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/302074

New failing tests:
js/Promise-types.html

[Build Bot]

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

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

[Yusuke Suzuki]

Created attachment 263408 [details]
Patch

[Darin Adler]

Comment on attachment 263408 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:93
> +    if (!newTarget || newTarget.isUndefined())

Why do we have to check for both of these? Are both really possible? When do we get one rather than the other?

[Yusuke Suzuki]

Comment on attachment 263408 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:93
>> +    if (!newTarget || newTarget.isUndefined())
> 
> Why do we have to check for both of these? Are both really possible? When do we get one rather than the other?

For now, I've aligned this part to the existing code like IntlCollatorConstructor.cpp because they do the same thing for `new.target`.
But, `!newTarget` won't become true. newTarget should not become JSEmpty if the callee is called as a constructor.
On the other hand, newTarget can be undefined if we execute `Reflect.construct(Promise, [], undefined)`.
This `Reflect.construct` is not implemented yet in WebKit JSC, but it may be implemented in the future.

Is it better that dropping `!newTarget` for JSPromiseConstructor in this patch? Or is it better that dropping all the `!newTarget` in the tree in the different patch?

[Yusuke Suzuki]

(In reply to comment #19)
> Comment on attachment 263408 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263408&action=review
> 
> >> Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:93
> >> +    if (!newTarget || newTarget.isUndefined())
> > 
> > Why do we have to check for both of these? Are both really possible? When do we get one rather than the other?
> 
> For now, I've aligned this part to the existing code like
> IntlCollatorConstructor.cpp because they do the same thing for `new.target`.
> But, `!newTarget` won't become true. newTarget should not become JSEmpty if
> the callee is called as a constructor.
> On the other hand, newTarget can be undefined if we execute
> `Reflect.construct(Promise, [], undefined)`.
> This `Reflect.construct` is not implemented yet in WebKit JSC, but it may be
> implemented in the future.
> 
> Is it better that dropping `!newTarget` for JSPromiseConstructor in this
> patch? Or is it better that dropping all the `!newTarget` in the tree in the
> different patch?

Personally, I prefer aligning to the existing code and fixing all of them in the separate patch.

[Darin Adler]

(In reply to comment #20)
> Personally, I prefer aligning to the existing code and fixing all of them in
> the separate patch.

Fine with me.

Long run I’d like to see it cleaned up because I don’t like code that branches for situations that can never happen. It’s a form of dead code and untestable code path.

[Yusuke Suzuki]

Committed r191276: <http://trac.webkit.org/changeset/191276>

[Yusuke Suzuki]

(In reply to comment #21)
> (In reply to comment #20)
> > Personally, I prefer aligning to the existing code and fixing all of them in
> > the separate patch.
> 
> Fine with me.
> 
> Long run I’d like to see it cleaned up because I don’t like code that
> branches for situations that can never happen. It’s a form of dead code and
> untestable code path.

Thanks for your review :)
I'll open the issue to drop these unnecessary path.

[Alexey Shvayka]

*** Bug 149477 has been marked as a duplicate of this bug. ***