Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: cherry-pick Swallowed Rejection Hook from V8 #21838

Closed
wants to merge 3 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Jul 16, 2018

Cherry-picks two commits:

Refs: nodejs/promises-debugging#8

Being an API change, they are not eligible for upstream back-merge.

/cc @nodejs/v8-update @MayaLekova @nodejs/promises-debugging

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Jul 16, 2018
@targos
Copy link
Member Author

targos commented Jul 16, 2018

I could cherry-pick the commits without a conflict but got a compilation error:

In file included from ../deps/v8/src/debug/debug.h:19,
                 from ../deps/v8/src/runtime/runtime-promise.cc:10:
../deps/v8/src/frames.h: In member function ‘void v8::internal::InnerPointerToCodeCache::Flush()’:
../deps/v8/src/frames.h:47:41: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct v8::internal::InnerPointerToCodeCache::InnerPointerToCodeCacheEntry’; use assignment or value-initialization instead [-Wclass-memaccess]
     memset(&cache_[0], 0, sizeof(cache_));
                                         ^
../deps/v8/src/frames.h:36:10: note: ‘struct v8::internal::InnerPointerToCodeCache::InnerPointerToCodeCacheEntry’ declared here
   struct InnerPointerToCodeCacheEntry {
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../deps/v8/src/runtime/runtime-promise.cc: In function ‘v8::internal::Object* v8::internal::__RT_impl_Runtime_PromiseRejectAfterResolved(v8::internal::Arguments, v8::internal::Isolate*)’:
../deps/v8/src/runtime/runtime-promise.cc:48:10: error: ‘ReadOnlyRoots’ was not declared in this scope
   return ReadOnlyRoots(isolate).undefined_value();
          ^~~~~~~~~~~~~
../deps/v8/src/runtime/runtime-promise.cc:48:10: note: suggested alternative: ‘ReadOnlySpace’
   return ReadOnlyRoots(isolate).undefined_value();
          ^~~~~~~~~~~~~
          ReadOnlySpace
../deps/v8/src/runtime/runtime-promise.cc: In function ‘v8::internal::Object* v8::internal::__RT_impl_Runtime_PromiseResolveAfterResolved(v8::internal::Arguments, v8::internal::Isolate*)’:
../deps/v8/src/runtime/runtime-promise.cc:58:10: error: ‘ReadOnlyRoots’ was not declared in this scope
   return ReadOnlyRoots(isolate).undefined_value();
          ^~~~~~~~~~~~~
../deps/v8/src/runtime/runtime-promise.cc:58:10: note: suggested alternative: ‘ReadOnlySpace’
   return ReadOnlyRoots(isolate).undefined_value();
          ^~~~~~~~~~~~~
          ReadOnlySpace

@targos
Copy link
Member Author

targos commented Jul 16, 2018

Compilation fixed.
Now a few of our tests are failing. It looks like there needs to be a change in node to support the new hook.
/cc @bmeurer is that expected?

Repro:

$ ./node -e "new Promise((resolve, reject) => { resolve(true); reject(false); })"
 1: 0x8afb30 node::Abort() [./node]
 2: 0x87e77b  [./node]
 3: 0xf0a88d v8::internal::Isolate::ReportPromiseReject(v8::internal::Handle<v8::internal::JSPromise>, v8::internal::Handle<v8::internal::Object>, v8::PromiseRejectEvent) [./node]
 4: 0x10e1bfd v8::internal::Runtime_PromiseRejectAfterResolved(int, v8::internal::Object**, v8::internal::Isolate*) [./node]
 5: 0x2793eb2841bd
[1]    29190 abort (core dumped)  ./node -e

kPromiseHandlerAddedAfterReject = 1
kPromiseHandlerAddedAfterReject = 1,
kPromiseRejectAfterResolved = 2,
kPromiseResolveAfterResolved = 3,

This comment was marked as resolved.

@devsnek
Copy link
Member

devsnek commented Jul 16, 2018

@targos

UNREACHABLE();

@BridgeAR
Copy link
Member

@targos just a few days ago in the @nodejs/promises-debugging group we discussed this together and I asked @MayaLekova if she could backport that upstream. Should we maybe wait for that? :-)

In general I am happy to have it in Node and I have no strong opinion but I thought it is best to have it upstream.

@bmeurer
Copy link
Member

bmeurer commented Jul 17, 2018

@BridgeAR we cannot backport on the V8 side since it requires changes in Chrome, which goes against our policy. You'll need to float this as patch on top of Node instead.

@MayaLekova
Copy link
Contributor

@BridgeAR I wasn't sure about the changes in Chrome when we talked, so yesterday after trying to backport this, we decided that the only option is to have it floated for Node.js. So @targos took the initiative.

@targos
Copy link
Member Author

targos commented Jul 17, 2018

@nodejs/promises-debugging would someone of you like to take it from here and make the necessary changes on Node's side? I'd also be happy to do it myself if you give me directions. I just haven't followed the original discussions and don't how you planned to use this hook.

@bmeurer
Copy link
Member

bmeurer commented Jul 17, 2018

@targos I'd suggest to go with v8@d11dafa initially and then with a later PR add actual functionality.

bmeurer and others added 3 commits July 17, 2018 11:55
This is done in preparation for landing

  https://chromium-review.googlesource.com/c/v8/v8/+/1126099

on the V8 side, which extends the existing PromiseRejectEvent mechanism
with new hooks for reject/resolve after a Promise was previously
resolved already.

Refs: nodejs/promises-debugging#8
Design: https://goo.gl/2stLUY
Original commit message:

    [turbofan] Remove optimization of default Promise capability functions.

    The JSCallReducer could in theory inline the default resolve and reject
    functions passed to the executor in the Promise constructor. But that
    inlining is almost never triggered because we don't have SFI based feedback
    in the CallIC. Also the use of the Promise constructor is discouraged,
    so we shouldn't really need to squeeze the last bit of performance out
    of this even in the future.

    Getting rid of this optimization will make significantly easier to
    implement the Swallowed Rejection Hook, as there's less churn on the
    TurboFan side then.

    Bug: v8:7919
    Change-Id: If0c54f1c6c7ce95686cd74232be6b8693ac688c9
    Reviewed-on: https://chromium-review.googlesource.com/1125926
    Commit-Queue: Benedikt Meurer <[email protected]>
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54210}

Refs: v8/v8@2075910
Original commit message:

    [promise] Implement Swallowed Rejection Hook.

    This extends the current Promise Rejection Hook with two new events

      kPromiseRejectAfterResolved
      kPromiseResolveAfterResolved

    which are used to detect (and signal) misuse of the Promise constructor.
    Specifically the common bug like

      new Promise((res, rej) => {
        res(1);
        throw new Error("something")
      });

    where the error is silently swallowed by the Promise constructor without
    the user ever noticing can be caught via this hook.

    Doc: https://goo.gl/2stLUY
    Bug: v8:7919
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I890a7e766cdd1be88db94844fb744f72823dba33
    Reviewed-on: https://chromium-review.googlesource.com/1126099
    Reviewed-by: Maya Lekova <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Benedikt Meurer <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54309}

Refs: v8/v8@907d7bc
@targos
Copy link
Member Author

targos commented Jul 17, 2018

@bmeurer I included your commit. This is ready for review

@targos
Copy link
Member Author

targos commented Jul 17, 2018

@devsnek devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 18, 2018
targos pushed a commit that referenced this pull request Jul 19, 2018
This is done in preparation for landing

  https://chromium-review.googlesource.com/c/v8/v8/+/1126099

on the V8 side, which extends the existing PromiseRejectEvent mechanism
with new hooks for reject/resolve after a Promise was previously
resolved already.

Refs: nodejs/promises-debugging#8
Design: https://goo.gl/2stLUY

PR-URL: #21838
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
targos added a commit that referenced this pull request Jul 19, 2018
Original commit message:

    [turbofan] Remove optimization of default Promise capability functions.

    The JSCallReducer could in theory inline the default resolve and reject
    functions passed to the executor in the Promise constructor. But that
    inlining is almost never triggered because we don't have SFI based feedback
    in the CallIC. Also the use of the Promise constructor is discouraged,
    so we shouldn't really need to squeeze the last bit of performance out
    of this even in the future.

    Getting rid of this optimization will make significantly easier to
    implement the Swallowed Rejection Hook, as there's less churn on the
    TurboFan side then.

    Bug: v8:7919
    Change-Id: If0c54f1c6c7ce95686cd74232be6b8693ac688c9
    Reviewed-on: https://chromium-review.googlesource.com/1125926
    Commit-Queue: Benedikt Meurer <[email protected]>
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#54210}

Refs: v8/v8@2075910

PR-URL: #21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
targos added a commit that referenced this pull request Jul 19, 2018
Original commit message:

    [promise] Implement Swallowed Rejection Hook.

    This extends the current Promise Rejection Hook with two new events

      kPromiseRejectAfterResolved
      kPromiseResolveAfterResolved

    which are used to detect (and signal) misuse of the Promise constructor.
    Specifically the common bug like

      new Promise((res, rej) => {
        res(1);
        throw new Error("something")
      });

    where the error is silently swallowed by the Promise constructor without
    the user ever noticing can be caught via this hook.

    Doc: https://goo.gl/2stLUY
    Bug: v8:7919
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I890a7e766cdd1be88db94844fb744f72823dba33
    Reviewed-on: https://chromium-review.googlesource.com/1126099
    Reviewed-by: Maya Lekova <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Benedikt Meurer <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#54309}

Refs: v8/v8@907d7bc

PR-URL: #21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
@targos
Copy link
Member Author

targos commented Jul 19, 2018

Landed in 756f63e 4ce35fa d0a545c

@targos targos closed this Jul 19, 2018
targos added a commit to targos/node that referenced this pull request Jul 25, 2018
Original commit message:

    [turbofan] Remove optimization of default Promise capability functions.

    The JSCallReducer could in theory inline the default resolve and reject
    functions passed to the executor in the Promise constructor. But that
    inlining is almost never triggered because we don't have SFI based feedback
    in the CallIC. Also the use of the Promise constructor is discouraged,
    so we shouldn't really need to squeeze the last bit of performance out
    of this even in the future.

    Getting rid of this optimization will make significantly easier to
    implement the Swallowed Rejection Hook, as there's less churn on the
    TurboFan side then.

    Bug: v8:7919
    Change-Id: If0c54f1c6c7ce95686cd74232be6b8693ac688c9
    Reviewed-on: https://chromium-review.googlesource.com/1125926
    Commit-Queue: Benedikt Meurer <[email protected]>
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54210}

Refs: v8/v8@2075910

PR-URL: nodejs#21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
targos added a commit to targos/node that referenced this pull request Jul 25, 2018
Original commit message:

    [promise] Implement Swallowed Rejection Hook.

    This extends the current Promise Rejection Hook with two new events

      kPromiseRejectAfterResolved
      kPromiseResolveAfterResolved

    which are used to detect (and signal) misuse of the Promise constructor.
    Specifically the common bug like

      new Promise((res, rej) => {
        res(1);
        throw new Error("something")
      });

    where the error is silently swallowed by the Promise constructor without
    the user ever noticing can be caught via this hook.

    Doc: https://goo.gl/2stLUY
    Bug: v8:7919
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I890a7e766cdd1be88db94844fb744f72823dba33
    Reviewed-on: https://chromium-review.googlesource.com/1126099
    Reviewed-by: Maya Lekova <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Benedikt Meurer <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54309}

Refs: v8/v8@907d7bc

PR-URL: nodejs#21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
targos added a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message:

    [turbofan] Remove optimization of default Promise capability functions.

    The JSCallReducer could in theory inline the default resolve and reject
    functions passed to the executor in the Promise constructor. But that
    inlining is almost never triggered because we don't have SFI based feedback
    in the CallIC. Also the use of the Promise constructor is discouraged,
    so we shouldn't really need to squeeze the last bit of performance out
    of this even in the future.

    Getting rid of this optimization will make significantly easier to
    implement the Swallowed Rejection Hook, as there's less churn on the
    TurboFan side then.

    Bug: v8:7919
    Change-Id: If0c54f1c6c7ce95686cd74232be6b8693ac688c9
    Reviewed-on: https://chromium-review.googlesource.com/1125926
    Commit-Queue: Benedikt Meurer <[email protected]>
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54210}

Refs: v8/v8@2075910

PR-URL: nodejs#21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
targos added a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message:

    [promise] Implement Swallowed Rejection Hook.

    This extends the current Promise Rejection Hook with two new events

      kPromiseRejectAfterResolved
      kPromiseResolveAfterResolved

    which are used to detect (and signal) misuse of the Promise constructor.
    Specifically the common bug like

      new Promise((res, rej) => {
        res(1);
        throw new Error("something")
      });

    where the error is silently swallowed by the Promise constructor without
    the user ever noticing can be caught via this hook.

    Doc: https://goo.gl/2stLUY
    Bug: v8:7919
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I890a7e766cdd1be88db94844fb744f72823dba33
    Reviewed-on: https://chromium-review.googlesource.com/1126099
    Reviewed-by: Maya Lekova <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Benedikt Meurer <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54309}

Refs: v8/v8@907d7bc

PR-URL: nodejs#21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
targos added a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message:

    [turbofan] Remove optimization of default Promise capability functions.

    The JSCallReducer could in theory inline the default resolve and reject
    functions passed to the executor in the Promise constructor. But that
    inlining is almost never triggered because we don't have SFI based feedback
    in the CallIC. Also the use of the Promise constructor is discouraged,
    so we shouldn't really need to squeeze the last bit of performance out
    of this even in the future.

    Getting rid of this optimization will make significantly easier to
    implement the Swallowed Rejection Hook, as there's less churn on the
    TurboFan side then.

    Bug: v8:7919
    Change-Id: If0c54f1c6c7ce95686cd74232be6b8693ac688c9
    Reviewed-on: https://chromium-review.googlesource.com/1125926
    Commit-Queue: Benedikt Meurer <[email protected]>
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54210}

Refs: v8/v8@2075910

PR-URL: nodejs#21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
targos added a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message:

    [promise] Implement Swallowed Rejection Hook.

    This extends the current Promise Rejection Hook with two new events

      kPromiseRejectAfterResolved
      kPromiseResolveAfterResolved

    which are used to detect (and signal) misuse of the Promise constructor.
    Specifically the common bug like

      new Promise((res, rej) => {
        res(1);
        throw new Error("something")
      });

    where the error is silently swallowed by the Promise constructor without
    the user ever noticing can be caught via this hook.

    Doc: https://goo.gl/2stLUY
    Bug: v8:7919
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I890a7e766cdd1be88db94844fb744f72823dba33
    Reviewed-on: https://chromium-review.googlesource.com/1126099
    Reviewed-by: Maya Lekova <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Benedikt Meurer <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54309}

Refs: v8/v8@907d7bc

PR-URL: nodejs#21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
@targos targos mentioned this pull request Jul 31, 2018
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

MylesBorins pushed a commit that referenced this pull request Aug 10, 2018
Original commit message:

    [turbofan] Remove optimization of default Promise capability functions.

    The JSCallReducer could in theory inline the default resolve and reject
    functions passed to the executor in the Promise constructor. But that
    inlining is almost never triggered because we don't have SFI based feedback
    in the CallIC. Also the use of the Promise constructor is discouraged,
    so we shouldn't really need to squeeze the last bit of performance out
    of this even in the future.

    Getting rid of this optimization will make significantly easier to
    implement the Swallowed Rejection Hook, as there's less churn on the
    TurboFan side then.

    Bug: v8:7919
    Change-Id: If0c54f1c6c7ce95686cd74232be6b8693ac688c9
    Reviewed-on: https://chromium-review.googlesource.com/1125926
    Commit-Queue: Benedikt Meurer <[email protected]>
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#54210}

Refs: v8/v8@2075910

Backport-PR-URL: #21668
PR-URL: #21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 10, 2018
Original commit message:

    [promise] Implement Swallowed Rejection Hook.

    This extends the current Promise Rejection Hook with two new events

      kPromiseRejectAfterResolved
      kPromiseResolveAfterResolved

    which are used to detect (and signal) misuse of the Promise constructor.
    Specifically the common bug like

      new Promise((res, rej) => {
        res(1);
        throw new Error("something")
      });

    where the error is silently swallowed by the Promise constructor without
    the user ever noticing can be caught via this hook.

    Doc: https://goo.gl/2stLUY
    Bug: v8:7919
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I890a7e766cdd1be88db94844fb744f72823dba33
    Reviewed-on: https://chromium-review.googlesource.com/1126099
    Reviewed-by: Maya Lekova <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Benedikt Meurer <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#54309}

Refs: v8/v8@907d7bc

Backport-PR-URL: #21668
PR-URL: #21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants