Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578173004:
[promisehook] Fire init hook for promise subclass (Closed)
Description[promisehook] Fire init hook for promise subclass
Add test as well.
Add regression test for passing uninitialized promises to init hook
BUG=v8:4643
Review-Url: https://codereview.chromium.org/2578173004
Cr-Commit-Position: refs/heads/master@{#41982}
Committed: https://chromium.googlesource.com/v8/v8/+/df179704ffe34d6c49cffbec24fa7ba43090bfc1
Patch Set 1 #Patch Set 2 : extend test #
Total comments: 1
Patch Set 3 : Add init hook for subclass #Patch Set 4 : Fix test #
Total comments: 2
Patch Set 5 : use jspromise #
Total comments: 2
Patch Set 6 : add regression test #Messages
Total messages: 38 (26 generated)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gsathya@chromium.org changed reviewers: + adamk@chromium.org, littledan@chromium.org
https://codereview.chromium.org/2578173004/diff/20001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/2578173004/diff/20001/test/cctest/test-api.cc... test/cctest/test-api.cc:18433: + // No init hook for subclasses Is this behavior intentional? Maybe the Promise constructor should make the PromiseHookInit call before checking if new.target is modified.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/19802) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/20 16:29:38, Dan Ehrenberg wrote: > https://codereview.chromium.org/2578173004/diff/20001/test/cctest/test-api.cc > File test/cctest/test-api.cc (right): > > https://codereview.chromium.org/2578173004/diff/20001/test/cctest/test-api.cc... > test/cctest/test-api.cc:18433: + // No init hook for subclasses > Is this behavior intentional? Maybe the Promise constructor should make the > PromiseHookInit call before checking if new.target is modified. Yeah, I did intend for this behavior, it doesn't work well when you mix thenables (NewPromiseCapability doesn't fire init hooks for non native promises). Not sure if we want to support all cases of thenables? Should promise subclasses fire the hooks even? I thought the main use of this was native promises which can't be monkey patched. I've changed the init hook to fire for the promise subclass.
I think these semantics make more sense. Yes, it's impossible to handle arbitrary thenables, but since we have Promise subclassing, I think it makes sense to call it like this where it all falls out naturally. https://codereview.chromium.org/2578173004/diff/60001/src/isolate.h File src/isolate.h (right): https://codereview.chromium.org/2578173004/diff/60001/src/isolate.h#newcode1132 src/isolate.h:1132: void RunPromiseHook(PromiseHookType type, Handle<JSObject> promise, I don't see how the promise here would not be a JSPromise. Doesn't this patch only change subclass instantiation? Given that Promise is a base class, it's hard for me to see how it would end up not getting called on a real JSPromise.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2578173004/diff/60001/src/isolate.h File src/isolate.h (right): https://codereview.chromium.org/2578173004/diff/60001/src/isolate.h#newcode1132 src/isolate.h:1132: void RunPromiseHook(PromiseHookType type, Handle<JSObject> promise, On 2016/12/20 22:45:18, Dan Ehrenberg wrote: > I don't see how the promise here would not be a JSPromise. Doesn't this patch > only change subclass instantiation? Given that Promise is a base class, it's > hard for me to see how it would end up not getting called on a real JSPromise. You're right. I was under the wrong impression that subclasses aren't the same type. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/20 23:47:26, gsathya wrote: > https://codereview.chromium.org/2578173004/diff/60001/src/isolate.h > File src/isolate.h (right): > > https://codereview.chromium.org/2578173004/diff/60001/src/isolate.h#newcode1132 > src/isolate.h:1132: void RunPromiseHook(PromiseHookType type, Handle<JSObject> > promise, > On 2016/12/20 22:45:18, Dan Ehrenberg wrote: > > I don't see how the promise here would not be a JSPromise. Doesn't this patch > > only change subclass instantiation? Given that Promise is a base class, it's > > hard for me to see how it would end up not getting called on a real JSPromise. > > You're right. I was under the wrong impression that subclasses aren't the same > type. Thanks! friendly ping
Description was changed from ========== [promisehook] Add test for promise subclass BUG=v8:4643 ========== to ========== [promisehook] Fire init hook for promise subclass Add test as well BUG=v8:4643 ==========
lgtm Sorry for the delay in response. This version looks really great--I didn't realize we were previously exposing an uninitialized Promise to the hooks before, and it's nice that this patch makes that not happen anymore. If you feel like it, there could be regression tests to make sure that we don't expose uninitialized Promises like that, but this patch seems good to go as is. https://codereview.chromium.org/2578173004/diff/80001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2578173004/diff/80001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:615: debug_push(this), init(this); What is this change for?
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [promisehook] Fire init hook for promise subclass Add test as well BUG=v8:4643 ========== to ========== [promisehook] Fire init hook for promise subclass Add test as well. Add regression test for passing uninitialized promises to init hook BUG=v8:4643 ==========
Thanks for the review! https://codereview.chromium.org/2578173004/diff/80001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2578173004/diff/80001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:615: debug_push(this), init(this); On 2016/12/28 18:14:42, Dan Ehrenberg wrote: > What is this change for? The debug_push block is always visited with this change, there's no advantage in it being deferred
On 2016/12/28 18:59:38, gsathya wrote: > Thanks for the review! > > https://codereview.chromium.org/2578173004/diff/80001/src/builtins/builtins-p... > File src/builtins/builtins-promise.cc (right): > > https://codereview.chromium.org/2578173004/diff/80001/src/builtins/builtins-p... > src/builtins/builtins-promise.cc:615: debug_push(this), init(this); > On 2016/12/28 18:14:42, Dan Ehrenberg wrote: > > What is this change for? > > The debug_push block is always visited with this change, there's no advantage in > it being deferred Added regression test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by gsathya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482953359199860,
"parent_rev": "224d3764e52fd4df91ece9274cee1305c3677286", "commit_rev":
"df179704ffe34d6c49cffbec24fa7ba43090bfc1"}
Message was sent while issue was closed.
Description was changed from ========== [promisehook] Fire init hook for promise subclass Add test as well. Add regression test for passing uninitialized promises to init hook BUG=v8:4643 ========== to ========== [promisehook] Fire init hook for promise subclass Add test as well. Add regression test for passing uninitialized promises to init hook BUG=v8:4643 Review-Url: https://codereview.chromium.org/2578173004 Cr-Commit-Position: refs/heads/master@{#41982} Committed: https://chromium.googlesource.com/v8/v8/+/df179704ffe34d6c49cffbec24fa7ba4309... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/v8/v8/+/df179704ffe34d6c49cffbec24fa7ba4309... |
