Introduce PromiseRejectCallback.

Introduce PromiseRejectCallback.

R=aandrey@chromium.org, yurys@chromium.org, rossberg@chromium.org
API=v8::Isolate::SetPromiseRejectCallback, v8::Promise::HasHandler
LOG=Y
BUG=v8:3093
Committed: https://code.google.com/p/v8/source/detail?r=24335

[Yang, 2014-09-29 14:52:48]

On 2014/09/29 14:52:19, Yang wrote:

This is the successor to https://codereview.chromium.org/607913002/.

[domenic_domenicdenicola.com, 2014-09-30 10:30:41]

domenic@domenicdenicola.com changed reviewers:
+ domenic@domenicdenicola.com

[domenic_domenicdenicola.com, 2014-09-30 10:30:42]

https://codereview.chromium.org/600723005/diff/1/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/600723005/diff/1/src/promise.js#newcode212
src/promise.js:212: SET_PRIVATE(promise, promiseHasHandler, true);
This doesn't seem correct. The promise doesn't have any handlers yet, and it
*should* trigger an is-rejected callback. (It may trigger a rejection-revoked
callback later within the same turn, but if someone does `var p =
Promise.reject(new Error("boo!"))` and never handles that, we need to notify of
that.)

[Yang, 2014-09-30 10:38:51]

https://codereview.chromium.org/600723005/diff/1/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/600723005/diff/1/src/promise.js#newcode212
src/promise.js:212: SET_PRIVATE(promise, promiseHasHandler, true);
On 2014/09/30 10:30:42, domenic wrote:
> This doesn't seem correct. The promise doesn't have any handlers yet, and it
> *should* trigger an is-rejected callback. (It may trigger a rejection-revoked
> callback later within the same turn, but if someone does `var p =
> Promise.reject(new Error("boo!"))` and never handles that, we need to notify
of
> that.)

If I reject a promise by returning a rejected promise like this:

var p = new Promise(function() { return Promise.reject('foo'); });

I would get two rejected promises, which is kind of weird imo. I would argue
that someone who uses Promise.reject to create a promise expects this and would
not be caught by surprise if the promise he gets is indeed rejected.

In my example, while p may be handled later on, the promise that is returned is
never handled because it is used merely to signal a reject. Not handling it is
completely intentional.

I do agree though that this special case is less than ideal.

[Yang, 2014-09-30 11:09:01]

On 2014/09/30 10:38:51, Yang wrote:
> https://codereview.chromium.org/600723005/diff/1/src/promise.js
> File src/promise.js (right):
> 
> https://codereview.chromium.org/600723005/diff/1/src/promise.js#newcode212
> src/promise.js:212: SET_PRIVATE(promise, promiseHasHandler, true);
> On 2014/09/30 10:30:42, domenic wrote:
> > This doesn't seem correct. The promise doesn't have any handlers yet, and it
> > *should* trigger an is-rejected callback. (It may trigger a
rejection-revoked
> > callback later within the same turn, but if someone does `var p =
> > Promise.reject(new Error("boo!"))` and never handles that, we need to notify
> of
> > that.)
> 
> If I reject a promise by returning a rejected promise like this:
> 
> var p = new Promise(function() { return Promise.reject('foo'); });
> 
> I would get two rejected promises, which is kind of weird imo. I would argue
> that someone who uses Promise.reject to create a promise expects this and
would
> not be caught by surprise if the promise he gets is indeed rejected.
> 
> In my example, while p may be handled later on, the promise that is returned
is
> never handled because it is used merely to signal a reject. Not handling it is
> completely intentional.
> 
> I do agree though that this special case is less than ideal.

New patch set to not omit the callback for Promise.reject().

[yurys, 2014-09-30 12:41:47]

https://codereview.chromium.org/600723005/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/600723005/diff/20001/include/v8.h#newcode2859
include/v8.h:2859: bool HasRejectHandler();
Would it make sense to rename this method to HasDerivedPromise or HasHandler to
better match its actual behavior?

[Yang, 2014-09-30 13:09:29]

On 2014/09/30 12:41:47, yurys wrote:
> https://codereview.chromium.org/600723005/diff/20001/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/600723005/diff/20001/include/v8.h#newcode2859
> include/v8.h:2859: bool HasRejectHandler();
> Would it make sense to rename this method to HasDerivedPromise or HasHandler
to
> better match its actual behavior?

Renamed.

I also changed the previous patch set as not to fail the debugger tests:
Promise.reject() did not trigger a debugger event, and this behavior should not
change.
Without the fix in patch set 3, calling Promise.reject() would always cause an
uncaught promise reject, and we don't have a debug event for revoke to that. Not
sure if we want to change that though.

[yurys, 2014-09-30 13:35:26]

lgtm

[aandrey, 2014-09-30 14:08:24]

lgtm

https://codereview.chromium.org/600723005/diff/40001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/600723005/diff/40001/src/debug.cc#newcode2516
src/debug.cc:2516: OnException(value, false, promise);
I think we should move to here:
  JSObject::SetProperty(jspromise, key, key, STRICT);

Also maybe move
  uncaught = has_reject_handler->IsFalse();
as well?

https://codereview.chromium.org/600723005/diff/40001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/600723005/diff/40001/src/promise.js#newcode209
src/promise.js:209: %PromiseRejectEvent(promise, r, false);
a comment why using "false" but not DEBUG_IS_ACTIVE?

[Yang, 2014-09-30 14:29:48]

Addressed/replied to comments.

https://codereview.chromium.org/600723005/diff/40001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/600723005/diff/40001/src/debug.cc#newcode2516
src/debug.cc:2516: OnException(value, false, promise);
On 2014/09/30 14:08:24, aandrey wrote:
> I think we should move to here:
>   JSObject::SetProperty(jspromise, key, key, STRICT);
> 
> Also maybe move
>   uncaught = has_reject_handler->IsFalse();
> as well?

That would not work. When a promise is rejected via throw, we trigger the debug
event at the throw site via OnThrow. Later when the try-catch in PromiseHandle
or the Promise constructor catches that throw, the reject closure is called,
which would trigger a second debug event. To prevent such a duplicate event, we
simply mark it the first time we see it.

I moved it to OnThrow instead.

Regarding uncaught: on both code paths (OnThrow and OnPromiseReject) we could
see either caught or uncaught promise rejects. So to avoid code duplication I
would just leave the call to PromiseHasUserDefinedRejectHandler in OnException.

https://codereview.chromium.org/600723005/diff/40001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/600723005/diff/40001/src/promise.js#newcode209
src/promise.js:209: %PromiseRejectEvent(promise, r, false);
On 2014/09/30 14:08:24, aandrey wrote:
> a comment why using "false" but not DEBUG_IS_ACTIVE?

Done.

[aandrey, 2014-09-30 14:43:27]

https://codereview.chromium.org/600723005/diff/40002/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/600723005/diff/40002/src/debug.cc#newcode2523
src/debug.cc:2523: OnException(value, false, promise);
But we should also call JSObject::SetProperty(Handle<JSObject>::cast(promise)
here? i.e. revert the last patch set? (sorry for misleading you)

[Yang, 2014-09-30 14:49:58]

On 2014/09/30 14:43:27, aandrey wrote:
> https://codereview.chromium.org/600723005/diff/40002/src/debug.cc
> File src/debug.cc (right):
> 
> https://codereview.chromium.org/600723005/diff/40002/src/debug.cc#newcode2523
> src/debug.cc:2523: OnException(value, false, promise);
> But we should also call JSObject::SetProperty(Handle<JSObject>::cast(promise)
> here? i.e. revert the last patch set? (sorry for misleading you)

done.

[aandrey, 2014-09-30 14:55:15]

https://codereview.chromium.org/600723005/diff/90001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/600723005/diff/90001/src/debug.cc#newcode2503
src/debug.cc:2503: Handle<Object> promise =
isolate_->GetPromiseOnStackOnThrow();
revert

[Yang, 2014-09-30 14:57:57]

On 2014/09/30 14:55:15, aandrey wrote:
> https://codereview.chromium.org/600723005/diff/90001/src/debug.cc
> File src/debug.cc (right):
> 
> https://codereview.chromium.org/600723005/diff/90001/src/debug.cc#newcode2503
> src/debug.cc:2503: Handle<Object> promise =
> isolate_->GetPromiseOnStackOnThrow();
> revert

Thanks. I was a bit unattentive there. Fixed.

[aandrey, 2014-09-30 15:00:36]

still lgtm

[Yang, 2014-09-30 15:29:34]

Committed patchset #7 (id:110001) manually as 24335 (presubmit successful).

[yurys, 2014-10-01 12:41:18]

https://codereview.chromium.org/600723005/diff/110001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/600723005/diff/110001/include/v8.h#newcode4212
include/v8.h:4212: typedef void (*PromiseRejectCallback)(Handle<Promise>
promise,
I've just realized that it may be convenient to also pass isolate as a
parameter. Otherwise always embedder has to call Isolate::GetCurrent(). I'm not
sure why v8 API uses C-style callbacks rather than abstract classes, that would
be another solution for the problem.

[Sven Panne, 2014-10-01 12:49:29]

svenpanne@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Sven Panne, 2014-10-01 12:49:30]

https://codereview.chromium.org/600723005/diff/110001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/600723005/diff/110001/include/v8.h#newcode4212
include/v8.h:4212: typedef void (*PromiseRejectCallback)(Handle<Promise>
promise,
On 2014/10/01 12:41:18, yurys wrote:
> I've just realized that it may be convenient to also pass isolate as a
> parameter. Otherwise always embedder has to call Isolate::GetCurrent(). I'm
not
> sure why v8 API uses C-style callbacks rather than abstract classes, that
would
> be another solution for the problem.

[ Sven's Isolate::GetCurrent() alarm bells ringing... ;-) ]

I didn't look into this in detail, but I think that there are 2 options:

   * Add an Isolate* parameter to the callback.

   * Add an GetIsolate() member function to Promise.

I think that the 2nd alternative is a bit nicer, but I don't have very strong
preferences. Anyway, this should be fixed quickly, forcing the use of
GetCurrent() would be very bad.

[yurys, 2014-10-01 13:01:52]

On 2014/10/01 12:49:30, Sven Panne wrote:
> https://codereview.chromium.org/600723005/diff/110001/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/600723005/diff/110001/include/v8.h#newcode4212
> include/v8.h:4212: typedef void (*PromiseRejectCallback)(Handle<Promise>
> promise,
> On 2014/10/01 12:41:18, yurys wrote:
> > I've just realized that it may be convenient to also pass isolate as a
> > parameter. Otherwise always embedder has to call Isolate::GetCurrent(). I'm
> not
> > sure why v8 API uses C-style callbacks rather than abstract classes, that
> would
> > be another solution for the problem.
> 
> [ Sven's Isolate::GetCurrent() alarm bells ringing... ;-) ]
> 
> I didn't look into this in detail, but I think that there are 2 options:
> 
>    * Add an Isolate* parameter to the callback.
> 
>    * Add an GetIsolate() member function to Promise.
> 
> I think that the 2nd alternative is a bit nicer, but I don't have very strong
> preferences. Anyway, this should be fixed quickly, forcing the use of
> GetCurrent() would be very bad.
FWIW, v8::V8::AddMessageListener also accepts a callback which doesn't accept
isolate as a parameter and so it is forced to call Isolate::GetCurrent() on
Blink side.

If we choose the second option I'd rather we added Object::GetIsolate which
would be more generic.

[Yang, 2014-10-01 13:01:58]

On 2014/10/01 12:49:30, Sven Panne wrote:
> https://codereview.chromium.org/600723005/diff/110001/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/600723005/diff/110001/include/v8.h#newcode4212
> include/v8.h:4212: typedef void (*PromiseRejectCallback)(Handle<Promise>
> promise,
> On 2014/10/01 12:41:18, yurys wrote:
> > I've just realized that it may be convenient to also pass isolate as a
> > parameter. Otherwise always embedder has to call Isolate::GetCurrent(). I'm
> not
> > sure why v8 API uses C-style callbacks rather than abstract classes, that
> would
> > be another solution for the problem.
> 
> [ Sven's Isolate::GetCurrent() alarm bells ringing... ;-) ]
> 
> I didn't look into this in detail, but I think that there are 2 options:
> 
>    * Add an Isolate* parameter to the callback.
> 
>    * Add an GetIsolate() member function to Promise.
> 
> I think that the 2nd alternative is a bit nicer, but I don't have very strong
> preferences. Anyway, this should be fixed quickly, forcing the use of
> GetCurrent() would be very bad.

As discussed offline, I'll add a v8::Object::GetIsolate()

[aandrey, 2014-10-01 13:20:03]

On 2014/10/01 13:01:58, Yang wrote:
> On 2014/10/01 12:49:30, Sven Panne wrote:
> > https://codereview.chromium.org/600723005/diff/110001/include/v8.h
> > File include/v8.h (right):
> > 
> >
https://codereview.chromium.org/600723005/diff/110001/include/v8.h#newcode4212
> > include/v8.h:4212: typedef void (*PromiseRejectCallback)(Handle<Promise>
> > promise,
> > On 2014/10/01 12:41:18, yurys wrote:
> > > I've just realized that it may be convenient to also pass isolate as a
> > > parameter. Otherwise always embedder has to call Isolate::GetCurrent().
I'm
> > not
> > > sure why v8 API uses C-style callbacks rather than abstract classes, that
> > would
> > > be another solution for the problem.
> > 
> > [ Sven's Isolate::GetCurrent() alarm bells ringing... ;-) ]
> > 
> > I didn't look into this in detail, but I think that there are 2 options:
> > 
> >    * Add an Isolate* parameter to the callback.
> > 
> >    * Add an GetIsolate() member function to Promise.
> > 
> > I think that the 2nd alternative is a bit nicer, but I don't have very
strong
> > preferences. Anyway, this should be fixed quickly, forcing the use of
> > GetCurrent() would be very bad.
> 
> As discussed offline, I'll add a v8::Object::GetIsolate()

or v8::Value::GetIsolate() ?

[Sven Panne, 2014-10-01 14:07:22]

On 2014/10/01 13:20:03, aandrey wrote:
> or v8::Value::GetIsolate() ?

Nope... Value can be a Smi, and we can't get an Isolate from it.