New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 830565 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Promise never resolves/rejects when thenable throws before callback

Reported by osswald...@gmail.com, Apr 9 2018

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3390.0 Safari/537.36

Steps to reproduce the problem:
Execute the following JavaScript Code with Chrome Version 67.0.3390.0

// Thenable throws before callback
// Promise rejects
var thenable = { then: function(resolve) {
  throw new TypeError('Throwing');
  resolve('Resolving');
}};

var p2 = Promise.resolve(thenable);
p2.then(function(v) {
  // not called
}, function(e) {
  console.log(e); // TypeError: Throwing
});

The code snippet is taken from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/resolve#Resolving_thenables_and_throwing_Errors

What is the expected behavior?
The promise should reject with:

TypeError: Throwing
    at Object.then (<anonymous>:4:9)

What went wrong?
The promise is never resolved or rejected.

Did this work before? Yes 65.0.3325.181

Chrome version: 67.0.3390.0  Channel: canary
OS Version: OS X 10.13.3
Flash Version: 

This has been detected in the SAPUI5 Testsuite:
https://sapui5.hana.ondemand.com/test-resources/sap/ui/core/qunit/jquery.sap.promise.qunit.html
 

Comment 1 by kojii@chromium.org, Apr 9 2018

Components: -Blink Blink>JavaScript
Status: Untriaged (was: Unconfirmed)
thennable-throws-830565.html
470 bytes View Download
Components: -Blink>JavaScript Blink>JavaScript>Language
Owner: bmeu...@chromium.org
Status: Assigned (was: Untriaged)
PTAL
Components: -Blink>JavaScript>Language Blink>JavaScript>Runtime
Status: Started (was: Assigned)
This regression has now landed in Chrome Stable (Version 66.0.3359.117).
Cc: gsat...@chromium.org hablich@chromium.org
Labels: Merge-Request-67 Merge-Request-66
Project Member

Comment 6 by sheriffbot@chromium.org, Apr 23 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
bmeurer@ - what needs to be merged to M66? 
Ah, the CL didn't make it through the CQ (https://chromium-review.googlesource.com/c/v8/v8/+/1023400). Looking now.
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/7f8e83b56d1d1120930afa512159e4232fdd51d2

commit 7f8e83b56d1d1120930afa512159e4232fdd51d2
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Tue Apr 24 07:55:00 2018

[builtins] Properly reject immediately throwing thenables.

Bug:  chromium:830565 
Change-Id: I1adab76e790a81f51f7b03165962992f5afecc99
Reviewed-on: https://chromium-review.googlesource.com/1023400
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52748}
[modify] https://crrev.com/7f8e83b56d1d1120930afa512159e4232fdd51d2/src/builtins/builtins-promise-gen.cc
[add] https://crrev.com/7f8e83b56d1d1120930afa512159e4232fdd51d2/test/mjsunit/regress/regress-crbug-830565.js
[modify] https://crrev.com/7f8e83b56d1d1120930afa512159e4232fdd51d2/test/webkit/fast/js/Promise-resolve-with-then-exception-expected.txt

Labels: -Merge-Review-66 Merge-Rejected-66
As per chat, let's target this for M67. 
Project Member

Comment 11 by sheriffbot@chromium.org, Apr 25 2018

Labels: -Merge-Request-67 Merge-Approved-67 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 25 2018

Labels: merge-merged-6.7
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/fd5fd02114ddc0bf608529bf7be181f22acaf7ee

commit fd5fd02114ddc0bf608529bf7be181f22acaf7ee
Author: Benedikt Meurer <bmeurer@google.com>
Date: Wed Apr 25 10:21:51 2018

Merged: [builtins] Properly reject immediately throwing thenables.

Revision: 7f8e83b56d1d1120930afa512159e4232fdd51d2

BUG= chromium:830565 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=verwaest@chromium.org

Change-Id: I6338ad673cddde78c09098229fb48e81b77b418d
Reviewed-on: https://chromium-review.googlesource.com/1027650
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.7@{#32}
Cr-Branched-From: 8457e810efd34381448d51d93f50079cf1f6a812-refs/heads/6.7.288@{#2}
Cr-Branched-From: e921be5c4f2c6407936bde750992dedbf47c1016-refs/heads/master@{#52547}
[modify] https://crrev.com/fd5fd02114ddc0bf608529bf7be181f22acaf7ee/src/builtins/builtins-promise-gen.cc
[add] https://crrev.com/fd5fd02114ddc0bf608529bf7be181f22acaf7ee/test/mjsunit/regress/regress-crbug-830565.js
[modify] https://crrev.com/fd5fd02114ddc0bf608529bf7be181f22acaf7ee/test/webkit/fast/js/Promise-resolve-with-then-exception-expected.txt

Labels: -Merge-Approved-67
Per comment #12, this is already merged to M67. So removing "Merge-Approved-67" label. Thank you.
Cc: petermarshall@chromium.org bmeu...@chromium.org sigurds@chromium.org
 Issue v8:7711  has been merged into this issue.
Cc: johnlenz@google.com
Labels: -Pri-2 -Merge-Rejected-66 Merge-Request-66 OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Windows Pri-1
Status: Fixed (was: Started)
See v8:7711, developers are affected by this.
Labels: -Merge-Request-66 Merge-Approved-66
Developers are working around this for now: https://github.com/zloirock/core-js/commit/8d6f675344b7c14d732b3a820a8e8554d025fb59 but would still be good to get this into 66 if possible.
Project Member

Comment 18 by bugdroid1@chromium.org, May 7 2018

Labels: merge-merged-6.6
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/40060be68ca4faf00f526c5e581148e1141f489d

commit 40060be68ca4faf00f526c5e581148e1141f489d
Author: Benedikt Meurer <bmeurer@google.com>
Date: Mon May 07 09:52:56 2018

Merged: [builtins] Properly reject immediately throwing thenables.

Revision: 7f8e83b56d1d1120930afa512159e4232fdd51d2

BUG= chromium:830565 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=jarin@chromium.org

Change-Id: I9c062024a317235252038d760e02a3fb0e4f0bb4
Reviewed-on: https://chromium-review.googlesource.com/1046665
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.6@{#61}
Cr-Branched-From: d500271571b92cb18dcd7b15885b51e8f437d640-refs/heads/6.6.346@{#1}
Cr-Branched-From: 265ef0b635f8761df7c89eb4e8ec9c1a6ebee184-refs/heads/master@{#51624}
[modify] https://crrev.com/40060be68ca4faf00f526c5e581148e1141f489d/src/builtins/builtins-promise-gen.cc
[add] https://crrev.com/40060be68ca4faf00f526c5e581148e1141f489d/test/mjsunit/regress/regress-crbug-830565.js
[modify] https://crrev.com/40060be68ca4faf00f526c5e581148e1141f489d/test/webkit/fast/js/Promise-resolve-with-then-exception-expected.txt

Project Member

Comment 19 by sheriffbot@chromium.org, May 7 2018

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Approved-66
Labels: NodeJS-Backport-Rejected
Not needed for active Node.js release lines.

Sign in to add a comment