Trigger debug event on not yet caught exception in promises.

Trigger debug event on not yet caught exception in promises.

R=aandrey@chromium.org, rossberg@chromium.org, yurys@chromium.org
BUG=v8:3093
LOG=Y

Committed: https://code.google.com/p/v8/source/detail?r=20956

[Yang, 2014-04-23 14:57:33]

First draft of this. I still have to write a test case for a custom deferred for
which the reject throws. Maybe some more test cases that I can't think of are
missing?

[rossberg, 2014-04-24 07:27:36]

Mostly ok, but please rename all those 'deferred_promise' into plain 'promise'.

https://codereview.chromium.org/249503002/diff/1/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/249503002/diff/1/src/debug-debugger.js#newcod...
src/debug-debugger.js:1097: function MakeExceptionEvent(exec_state, exception,
uncaught, deferred_promise) {
And here...

https://codereview.chromium.org/249503002/diff/1/src/debug-debugger.js#newcod...
src/debug-debugger.js:1130: ExceptionEvent.prototype.deferred_promise =
function() {
And so on...

https://codereview.chromium.org/249503002/diff/1/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/249503002/diff/1/src/debug.cc#newcode2671
src/debug.cc:2671: Handle<Object> deferred_promise) {
And here.

https://codereview.chromium.org/249503002/diff/1/src/debug.h
File src/debug.h (right):

https://codereview.chromium.org/249503002/diff/1/src/debug.h#newcode798
src/debug.h:798: Handle<Object> deferred_promise);
Nit: any reason not to just name this 'promise'?

https://codereview.chromium.org/249503002/diff/1/src/debug.h#newcode806
src/debug.h:806: Handle<Object> deferred_promise = Handle<Object>::null());
Same here.

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

https://codereview.chromium.org/249503002/diff/1/src/promise.js#newcode200
src/promise.js:200: // Reject handler threw (must be a custom one).
"must have been"

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc#newcode5657
src/runtime.cc:5657: HandleScope scope(isolate);
Why have both a SHS and a HS in the same function?

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc#newcode5659
src/runtime.cc:5659: CONVERT_ARG_HANDLE_CHECKED(Object, deferred_promise, 1);
One more.

https://codereview.chromium.org/249503002/diff/1/test/mjsunit/es6/debug-promi...
File test/mjsunit/es6/debug-promises-caught.js (right):

https://codereview.chromium.org/249503002/diff/1/test/mjsunit/es6/debug-promi...
test/mjsunit/es6/debug-promises-caught.js:39: print(e + "\n" + e.stack);
Maybe add "Unexpected exception: ".

https://codereview.chromium.org/249503002/diff/1/test/mjsunit/es6/debug-promi...
File test/mjsunit/es6/debug-promises-uncaught.js (right):

https://codereview.chromium.org/249503002/diff/1/test/mjsunit/es6/debug-promi...
test/mjsunit/es6/debug-promises-uncaught.js:36: assertEquals('object', typeof
event_data.deferred_promise());
You could more specifically test 'event_data.promise() instanceof Promise' here.

https://codereview.chromium.org/249503002/diff/1/test/mjsunit/es6/debug-promi...
test/mjsunit/es6/debug-promises-uncaught.js:45: print(e + "\n" + e.stack);
Maybe add "Unexpected exception: ".

[yurys, 2014-04-24 07:48:43]

Looks OK overall, +aandrey who may have some input.

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc#newcode5660
src/runtime.cc:5660: isolate->debugger()->OnException(exception, false,
deferred_promise);
It might make sense to report the exception as uncaught=true here in case there
is no catch handler on the promise yet.

https://codereview.chromium.org/249503002/diff/1/test/mjsunit/es6/debug-promi...
File test/mjsunit/es6/debug-promises-caught.js (right):

https://codereview.chromium.org/249503002/diff/1/test/mjsunit/es6/debug-promi...
test/mjsunit/es6/debug-promises-caught.js:29: assertEquals(["resolve", "end
main", "throw"], log);
Just curious: I'd expect "end main" to be logged before "resolve" or does
Promise evaluate synchronously in this case?

[Yang, 2014-04-24 07:59:59]

Test case for custom deferred promise still missing. Addressed comments for now.

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc#newcode5660
src/runtime.cc:5660: isolate->debugger()->OnException(exception, false,
deferred_promise);
On 2014/04/24 07:48:43, yurys wrote:
> It might make sense to report the exception as uncaught=true here in case
there
> is no catch handler on the promise yet.

Done.

Alternatively, we could always fire this runtime function, and set uncaught
depending on whether we already have a catch handler. Devtools would then filter
out caught ones. That would give you guys more flexibility (not sure if
necessary).

https://codereview.chromium.org/249503002/diff/1/test/mjsunit/es6/debug-promi...
File test/mjsunit/es6/debug-promises-caught.js (right):

https://codereview.chromium.org/249503002/diff/1/test/mjsunit/es6/debug-promi...
test/mjsunit/es6/debug-promises-caught.js:29: assertEquals(["resolve", "end
main", "throw"], log);
On 2014/04/24 07:48:43, yurys wrote:
> Just curious: I'd expect "end main" to be logged before "resolve" or does
> Promise evaluate synchronously in this case?

Yes. The function in line 12 is executed immediately. However, the chained
function in line 18 is put into the micro task queue and executed after this
test script is finished executing.

[rossberg, 2014-04-24 08:08:38]

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc#newcode5657
src/runtime.cc:5657: HandleScope scope(isolate);
On 2014/04/24 07:27:36, rossberg wrote:
> Why have both a SHS and a HS in the same function?

Seems like you missed this one.

https://codereview.chromium.org/249503002/diff/20001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-uncaught.js (right):

https://codereview.chromium.org/249503002/diff/20001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-uncaught.js:48: print("Unexpected exception:");
Nit: don't need a newline after this

[yurys, 2014-04-24 08:49:49]

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc#newcode5660
src/runtime.cc:5660: isolate->debugger()->OnException(exception, false,
deferred_promise);
On 2014/04/24 07:59:59, Yang wrote:
> On 2014/04/24 07:48:43, yurys wrote:
> > It might make sense to report the exception as uncaught=true here in case
> there
> > is no catch handler on the promise yet.
> 
> Done.
> 
> Alternatively, we could always fire this runtime function, and set uncaught
> depending on whether we already have a catch handler. Devtools would then
filter
> out caught ones. That would give you guys more flexibility (not sure if
> necessary).
I believe this is not necessary. We have three modes in DevTools:
1) don't pause on exception
2) pause on uncaught exceptions only (controlled by
Debug.setBreakOnUncaughtException();)
3) pause on all exceptions (controlled by  Debug.setBreakOnException();)
All of them are handled in v8 at the moment and I believe the promises should
follow that logic too.

https://codereview.chromium.org/249503002/diff/20001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-uncaught.js (right):

https://codereview.chromium.org/249503002/diff/20001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-uncaught.js:30: if (event ==
Debug.DebugEvent.Exception) {
Would be nice to also test that this branch is not executed if
Debug.setBreakOnUncaughtException(); was called instead of
Debug.setBreakOnException(); at line 54. That would match "Pause on uncaught
exceptions only" in DevTools.

[Yang, 2014-04-24 10:40:12]

I added the test case for custom deferred promise and the ones for
setBreakOnUncaughtException.

[Yang, 2014-04-24 10:42:04]

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/249503002/diff/1/src/runtime.cc#newcode5659
src/runtime.cc:5659: CONVERT_ARG_HANDLE_CHECKED(Object, deferred_promise, 1);
On 2014/04/24 07:27:36, rossberg wrote:
> One more.

Done.

https://codereview.chromium.org/249503002/diff/20001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-uncaught.js (right):

https://codereview.chromium.org/249503002/diff/20001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-uncaught.js:48: print("Unexpected exception:");
On 2014/04/24 08:08:38, rossberg wrote:
> Nit: don't need a newline after this

Done.

[yurys, 2014-04-24 11:01:15]

On 2014/04/24 10:40:12, Yang wrote:
> I added the test case for custom deferred promise and the ones for
> setBreakOnUncaughtException.

Awesome, thank you, LGTM!

[aandrey, 2014-04-24 11:04:58]

lgtm

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

https://codereview.chromium.org/249503002/diff/40001/src/debug.cc#newcode2700
src/debug.cc:2700: promise = isolate_->factory()->undefined_value();
maybe set undefined to promise by default instead of null? then this line will
go away naturally

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

https://codereview.chromium.org/249503002/diff/40001/src/promise.js#newcode199
src/promise.js:199: try { deferred.reject(e) } catch(e) {
nit: expand try { ... } ?

https://codereview.chromium.org/249503002/diff/40001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-caught.js (right):

https://codereview.chromium.org/249503002/diff/40001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-caught.js:9: var resolve_fun;
remove

https://codereview.chromium.org/249503002/diff/40001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-uncaught.js (right):

https://codereview.chromium.org/249503002/diff/40001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-uncaught.js:9: var resolve_fun;
remove

[Yang, 2014-04-24 11:13:47]

Uploaded a new patch set. The rest of the comments are already dealt with in the
previous patch set.

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

https://codereview.chromium.org/249503002/diff/40001/src/debug.cc#newcode2700
src/debug.cc:2700: promise = isolate_->factory()->undefined_value();
On 2014/04/24 11:04:59, aandrey wrote:
> maybe set undefined to promise by default instead of null? then this line will
> go away naturally

Done.

[Yang, 2014-04-24 13:40:43]

On 2014/04/24 11:13:47, Yang wrote:
> Uploaded a new patch set. The rest of the comments are already dealt with in
the
> previous patch set.
> 
> https://codereview.chromium.org/249503002/diff/40001/src/debug.cc
> File src/debug.cc (right):
> 
> https://codereview.chromium.org/249503002/diff/40001/src/debug.cc#newcode2700
> src/debug.cc:2700: promise = isolate_->factory()->undefined_value();
> On 2014/04/24 11:04:59, aandrey wrote:
> > maybe set undefined to promise by default instead of null? then this line
will
> > go away naturally
> 
> Done.

Slightly changed the way we deal with custom deferred promises. Awaiting
Andreas' review.

[rossberg, 2014-04-24 14:33:47]

https://codereview.chromium.org/249503002/diff/100001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/249503002/diff/100001/src/promise.js#newcode185
src/promise.js:185: 
nit: spurious new line

https://codereview.chromium.org/249503002/diff/100001/src/promise.js#newcode197
src/promise.js:197: if (!IS_SPEC_FUNCTION(deferred.reject)) {
Why do you have to test this case? It will throw in the 'try' below anyway (and
be handled appropriately).

https://codereview.chromium.org/249503002/diff/100001/test/mjsunit/es6/debug-...
File test/mjsunit/es6/debug-promises-caught-uncaught.js (right):

https://codereview.chromium.org/249503002/diff/100001/test/mjsunit/es6/debug-...
test/mjsunit/es6/debug-promises-caught-uncaught.js:32: } else if (event ==
Debug.DebugEvent.PendingExceptionInPromise) {
Use a single if with ||

[Yang, 2014-04-24 14:44:53]

https://codereview.chromium.org/249503002/diff/100001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/249503002/diff/100001/src/promise.js#newcode185
src/promise.js:185: 
On 2014/04/24 14:33:47, rossberg wrote:
> nit: spurious new line

Done.

https://codereview.chromium.org/249503002/diff/100001/src/promise.js#newcode197
src/promise.js:197: if (!IS_SPEC_FUNCTION(deferred.reject)) {
On 2014/04/24 14:33:47, rossberg wrote:
> Why do you have to test this case? It will throw in the 'try' below anyway
(and
> be handled appropriately).

Yeah, it will throw, with a TypeError 'undefined is not a function' as
exception. The original exception is then gone. That seems unintuitive.

https://codereview.chromium.org/249503002/diff/100001/test/mjsunit/es6/debug-...
File test/mjsunit/es6/debug-promises-caught-uncaught.js (right):

https://codereview.chromium.org/249503002/diff/100001/test/mjsunit/es6/debug-...
test/mjsunit/es6/debug-promises-caught-uncaught.js:32: } else if (event ==
Debug.DebugEvent.PendingExceptionInPromise) {
On 2014/04/24 14:33:47, rossberg wrote:
> Use a single if with ||

Done.

[rossberg, 2014-04-24 14:56:51]

https://codereview.chromium.org/249503002/diff/100001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/249503002/diff/100001/src/promise.js#newcode197
src/promise.js:197: if (!IS_SPEC_FUNCTION(deferred.reject)) {
On 2014/04/24 14:44:53, Yang wrote:
> On 2014/04/24 14:33:47, rossberg wrote:
> > Why do you have to test this case? It will throw in the 'try' below anyway
> (and
> > be handled appropriately).
> 
> Yeah, it will throw, with a TypeError 'undefined is not a function' as
> exception. The original exception is then gone. That seems unintuitive.

That may be so, but that's equally true for other errors that could come up from
the call. Not sure why this one justifies an exception (no pun intended). When
delivering to the handler fails I'd say either consistently keep the original
exception, or consistently use the inner one.  (Also, this case probably never
comes up in practice -- and I dislike introducing random special case logic for
cases that don't even matter. ;) )

[Yang, 2014-04-24 15:05:33]

On 2014/04/24 14:56:51, rossberg wrote:
> https://codereview.chromium.org/249503002/diff/100001/src/promise.js
> File src/promise.js (right):
> 
>
https://codereview.chromium.org/249503002/diff/100001/src/promise.js#newcode197
> src/promise.js:197: if (!IS_SPEC_FUNCTION(deferred.reject)) {
> On 2014/04/24 14:44:53, Yang wrote:
> > On 2014/04/24 14:33:47, rossberg wrote:
> > > Why do you have to test this case? It will throw in the 'try' below anyway
> > (and
> > > be handled appropriately).
> > 
> > Yeah, it will throw, with a TypeError 'undefined is not a function' as
> > exception. The original exception is then gone. That seems unintuitive.
> 
> That may be so, but that's equally true for other errors that could come up
from
> the call. Not sure why this one justifies an exception (no pun intended). When
> delivering to the handler fails I'd say either consistently keep the original
> exception, or consistently use the inner one.  (Also, this case probably never
> comes up in practice -- and I dislike introducing random special case logic
for
> cases that don't even matter. ;) )

Alright then. Let's always use the original exception.

[Yang, 2014-04-24 15:08:44]

On 2014/04/24 15:05:33, Yang wrote:
> On 2014/04/24 14:56:51, rossberg wrote:
> > https://codereview.chromium.org/249503002/diff/100001/src/promise.js
> > File src/promise.js (right):
> > 
> >
>
https://codereview.chromium.org/249503002/diff/100001/src/promise.js#newcode197
> > src/promise.js:197: if (!IS_SPEC_FUNCTION(deferred.reject)) {
> > On 2014/04/24 14:44:53, Yang wrote:
> > > On 2014/04/24 14:33:47, rossberg wrote:
> > > > Why do you have to test this case? It will throw in the 'try' below
anyway
> > > (and
> > > > be handled appropriately).
> > > 
> > > Yeah, it will throw, with a TypeError 'undefined is not a function' as
> > > exception. The original exception is then gone. That seems unintuitive.
> > 
> > That may be so, but that's equally true for other errors that could come up
> from
> > the call. Not sure why this one justifies an exception (no pun intended).
When
> > delivering to the handler fails I'd say either consistently keep the
original
> > exception, or consistently use the inner one.  (Also, this case probably
never
> > comes up in practice -- and I dislike introducing random special case logic
> for
> > cases that don't even matter. ;) )
> 
> Alright then. Let's always use the original exception.

new patch set uploaded btw.

[rossberg, 2014-04-24 15:10:42]

lgtm

[Yang, 2014-04-25 07:03:23]

Committed patchset #8 manually as r20956 (presubmit successful).

[aandrey, 2014-04-29 13:04:36]

https://codereview.chromium.org/249503002/diff/140001/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/249503002/diff/140001/src/debug-debugger.js#n...
src/debug-debugger.js:46: PendingExceptionInPromise: 7 };
Is it OK that this does not match the DebugEvent enum name
"UncaughtExceptionInPromise" defined in v8-debug.h?