Chromium Code Reviews

Issue 1411083003: Debugger: correctly report uncaught rejections in Promise.all and Promise.race. (Closed)

Created:
5 years, 2 months ago by Yang
Modified:
5 years, 2 months ago
Reviewers:
Hannes Payer (out of office), rossberg
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

Debugger: correctly report uncaught rejections in Promise.all and Promise.race. The debugger calls PromiseHasUserDefinedRejectHandler to recursively search the tree of dependent promises for user-defined reject handlers. If no such reject handler exists, rejecting the promise is considered an uncaught exception. Promise.race and Promise.all interupt the link of promise dependency wrt the search. This change fixes that link. R=rossberg@chromium.org BUG=chromium:439585 LOG=N Committed: https://crrev.com/8be20eee3ba63b1d78eab065d9b6787b624cfff2 Cr-Commit-Position: refs/heads/master@{#31392}

Patch Set 1 #

Total comments: 2

Patch Set 2 : move deferred from promise to handler #

Total comments: 1
Unified diffs Side-by-side diffs Stats (+244 lines, -7 lines)
M src/heap/heap.h View 1 chunk +1 line, -0 lines 0 comments
M src/js/promise.js View 5 chunks +17 lines, -7 lines 1 comment
A test/mjsunit/es6/debug-promises/promise-all-caught.js View 1 chunk +40 lines, -0 lines 0 comments
A test/mjsunit/es6/debug-promises/promise-all-uncaught.js View 1 chunk +73 lines, -0 lines 0 comments
A test/mjsunit/es6/debug-promises/promise-race-caught.js View 1 chunk +40 lines, -0 lines 0 comments
A test/mjsunit/es6/debug-promises/promise-race-uncaught.js View 1 chunk +73 lines, -0 lines 0 comments

Messages

Total messages: 16 (5 generated)
Yang
5 years, 2 months ago (2015-10-19 08:24:13 UTC) #1
rossberg
lgtm https://codereview.chromium.org/1411083003/diff/1/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/1411083003/diff/1/src/js/promise.js#newcode355 src/js/promise.js:355: if (PromiseHasUserDefinedRejectHandlerRecursive(deferred.promise)) { Why the extra conditional and ...
5 years, 2 months ago (2015-10-19 14:24:29 UTC) #2
Yang
https://codereview.chromium.org/1411083003/diff/1/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/1411083003/diff/1/src/js/promise.js#newcode355 src/js/promise.js:355: if (PromiseHasUserDefinedRejectHandlerRecursive(deferred.promise)) { On 2015/10/19 14:24:29, rossberg wrote: > ...
5 years, 2 months ago (2015-10-19 16:24:09 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411083003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411083003/1
5 years, 2 months ago (2015-10-19 16:24:26 UTC) #5
commit-bot: I haz the power
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/6810)
5 years, 2 months ago (2015-10-19 16:26:48 UTC) #7
Yang
Hannes, please look at the heap changes.
5 years, 2 months ago (2015-10-19 16:30:57 UTC) #9
Yang
https://codereview.chromium.org/1411083003/diff/20001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/1411083003/diff/20001/src/js/promise.js#newcode333 src/js/promise.js:333: this.resolve(value).then(function(x) { deferred.resolve(x) }, reject); then() may not even ...
5 years, 2 months ago (2015-10-20 05:15:49 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411083003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411083003/20001
5 years, 2 months ago (2015-10-20 05:16:04 UTC) #13
Hannes Payer (out of office)
heap LGTM
5 years, 2 months ago (2015-10-20 05:17:17 UTC) #14
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 2 months ago (2015-10-20 05:40:06 UTC) #15
commit-bot: I haz the power
5 years, 2 months ago (2015-10-20 05:40:21 UTC) #16
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/8be20eee3ba63b1d78eab065d9b6787b624cfff2
Cr-Commit-Position: refs/heads/master@{#31392}

Powered by Google App Engine