Bug 149380 - Promise constructor should throw when not called with "new"
Summary: Promise constructor should throw when not called with "new"
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: http://www.ecma-international.org/ecm...
Keywords:
: 149477 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-09-19 19:01 PDT by Jordan Harband
Modified: 2020-04-03 11:04 PDT (History)
12 users (show)

See Also:


Attachments
Patch (5.48 KB, patch)
2015-10-17 17:46 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (5.56 KB, patch)
2015-10-17 17:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (693.81 KB, application/zip)
2015-10-17 18:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (711.16 KB, application/zip)
2015-10-17 18:27 PDT, Build Bot
no flags Details
Patch (18.94 KB, patch)
2015-10-17 18:40 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (630.31 KB, application/zip)
2015-10-17 19:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (702.45 KB, application/zip)
2015-10-17 19:18 PDT, Build Bot
no flags Details
Patch (18.93 KB, patch)
2015-10-17 21:46 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jordan Harband 2015-09-19 19:01:05 PDT
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.
Comment 1 Jordan Harband 2015-10-03 11:02:41 PDT
Bumping importance since this is a spec violation and nobody's responded yet.
Comment 2 Yusuke Suzuki 2015-10-17 16:23:42 PDT
Personally, I think Blocker means crashing bug.
Comment 3 Yusuke Suzuki 2015-10-17 16:27:43 PDT
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.
Comment 4 Yusuke Suzuki 2015-10-17 17:46:38 PDT
Created attachment 263393 [details]
Patch
Comment 5 Yusuke Suzuki 2015-10-17 17:48:49 PDT
Created attachment 263394 [details]
Patch
Comment 6 Jordan Harband 2015-10-17 17:58:10 PDT
Thanks for fixing the severity, as well as the code patch!
Comment 7 Build Bot 2015-10-17 18:22:27 PDT
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
Comment 8 Build Bot 2015-10-17 18:22:30 PDT
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
Comment 9 Build Bot 2015-10-17 18:27:45 PDT
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
Comment 10 Build Bot 2015-10-17 18:27:48 PDT
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
Comment 11 Yusuke Suzuki 2015-10-17 18:40:01 PDT
Created attachment 263400 [details]
Patch
Comment 12 Yusuke Suzuki 2015-10-17 18:41:06 PDT
(In reply to comment #6)
> Thanks for fixing the severity, as well as the code patch!

:D
Comment 13 Build Bot 2015-10-17 19:12:39 PDT
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
Comment 14 Build Bot 2015-10-17 19:12:43 PDT
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
Comment 15 Build Bot 2015-10-17 19:18:06 PDT
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
Comment 16 Build Bot 2015-10-17 19:18:09 PDT
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
Comment 17 Yusuke Suzuki 2015-10-17 21:46:59 PDT
Created attachment 263408 [details]
Patch
Comment 18 Darin Adler 2015-10-17 23:06:30 PDT
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?
Comment 19 Yusuke Suzuki 2015-10-17 23:53:07 PDT
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?
Comment 20 Yusuke Suzuki 2015-10-18 00:22:25 PDT
(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.
Comment 21 Darin Adler 2015-10-18 15:12:14 PDT
(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.
Comment 22 Yusuke Suzuki 2015-10-18 17:52:07 PDT
Committed r191276: <http://trac.webkit.org/changeset/191276>
Comment 23 Yusuke Suzuki 2015-10-18 18:33:52 PDT
(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.
Comment 24 Alexey Shvayka 2020-04-03 11:04:04 PDT
*** Bug 149477 has been marked as a duplicate of this bug. ***