Work in progress: allow pause-on-exceptions to work in Object.observe callbacks

Work in progress: allow pause-on-exceptions to work in Object.observe callbacks

NOT YET READY FOR LANDING

BUG=chromium:335660
LOG=y

[Yang, 2014-10-30 10:34:22]

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

[Yang, 2014-10-30 10:34:23]

This is looking good. Though maybe we can find something more meaningful than
null as sentinel. But without introducing something new, the only thing I see
would be the observationState value. That would also be a JSObject, which saves
changing the type. Probably not what we want though. Otherwise please add
comments to the places where we use null and check for null.

[aandrey, 2014-10-30 10:53:01]

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

[aandrey, 2014-10-30 10:53:01]

https://codereview.chromium.org/653023004/diff/1/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/653023004/diff/1/src/object-observe.js#newcod...
src/object-observe.js:574: %DebugPushPromise(null);
But should not we patch Isolate::DoThrow() instead?

Maybe we just call smth like %SetVerboseNextTryCatch() before entering the
try-catch block to make it alike the verbose v8::TryCatch? Message reporting and
debug event will work naturally w/o any changes.

[Yang, 2014-10-30 10:59:46]

On 2014/10/30 10:53:01, aandrey wrote:
> https://codereview.chromium.org/653023004/diff/1/src/object-observe.js
> File src/object-observe.js (right):
> 
>
https://codereview.chromium.org/653023004/diff/1/src/object-observe.js#newcod...
> src/object-observe.js:574: %DebugPushPromise(null);
> But should not we patch Isolate::DoThrow() instead?
> 
> Maybe we just call smth like %SetVerboseNextTryCatch() before entering the
> try-catch block to make it alike the verbose v8::TryCatch? Message reporting
and
> debug event will work naturally w/o any changes.

Something like %SetVerboseNextTryCatch is not easily implemented. We push
try-catch handlers in generated code onto the stack, so to implement something
like verbosity, we would have to involve the parser and change
platform-dependent code. At this point, I also have no idea what would happen in
turbo fan code. So it's just a lot easier to piggyback on the existing
infrastructure for promises.

If performance becomes a problem, we could implement %DebugPushPromise and
%DebugPopPromise in generated code and change the data structure of the promise
stack.

[Yang, 2014-10-30 11:04:28]

On 2014/10/30 10:59:46, Yang wrote:
> On 2014/10/30 10:53:01, aandrey wrote:
> > https://codereview.chromium.org/653023004/diff/1/src/object-observe.js
> > File src/object-observe.js (right):
> > 
> >
>
https://codereview.chromium.org/653023004/diff/1/src/object-observe.js#newcod...
> > src/object-observe.js:574: %DebugPushPromise(null);
> > But should not we patch Isolate::DoThrow() instead?
> > 
> > Maybe we just call smth like %SetVerboseNextTryCatch() before entering the
> > try-catch block to make it alike the verbose v8::TryCatch? Message reporting
> and
> > debug event will work naturally w/o any changes.
> 
> Something like %SetVerboseNextTryCatch is not easily implemented. We push
> try-catch handlers in generated code onto the stack, so to implement something
> like verbosity, we would have to involve the parser and change
> platform-dependent code. At this point, I also have no idea what would happen
in
> turbo fan code. So it's just a lot easier to piggyback on the existing
> infrastructure for promises.
> 
> If performance becomes a problem, we could implement %DebugPushPromise and
> %DebugPopPromise in generated code and change the data structure of the
promise
> stack.

Also, implementing %DebugPushPromise and %DebugPopPromise in generated code
would basically be the same as supporting verbose try-catch scopes without
introducing a custom javascript syntax and having to support it in the
parser/code generator, except that it's explicit instead of implicit.

For console output and window.onerror we plan to introduce something similar to
the PromiseRejectCallback. It's similar to it in the way that throwing in the
deliver-loop does not stop execution. That callback would be implemented as a
runtime function called in the catch block.

[aandrey, 2014-10-30 11:05:32]

On 2014/10/30 10:59:46, Yang wrote:
> On 2014/10/30 10:53:01, aandrey wrote:
> > https://codereview.chromium.org/653023004/diff/1/src/object-observe.js
> > File src/object-observe.js (right):
> > 
> >
>
https://codereview.chromium.org/653023004/diff/1/src/object-observe.js#newcod...
> > src/object-observe.js:574: %DebugPushPromise(null);
> > But should not we patch Isolate::DoThrow() instead?
> > 
> > Maybe we just call smth like %SetVerboseNextTryCatch() before entering the
> > try-catch block to make it alike the verbose v8::TryCatch? Message reporting
> and
> > debug event will work naturally w/o any changes.
> 
> Something like %SetVerboseNextTryCatch is not easily implemented. We push
> try-catch handlers in generated code onto the stack, so to implement something
> like verbosity, we would have to involve the parser and change
> platform-dependent code. At this point, I also have no idea what would happen
in
> turbo fan code. So it's just a lot easier to piggyback on the existing
> infrastructure for promises.
> 
> If performance becomes a problem, we could implement %DebugPushPromise and
> %DebugPopPromise in generated code and change the data structure of the
promise
> stack.

Can we remove the try-catch completely in favor of:
- extending %_CallFunction() to make a runtime call in a v8::TryCatch scope
- introducing smth like %_CallFunctionInTryCatch()
- introducing an intermediate runtime function that would create a verbose
TryCatch and do whatever %_CallFunction() does
?

[aandrey, 2014-10-30 11:12:35]

On 2014/10/30 11:05:32, aandrey wrote:
> On 2014/10/30 10:59:46, Yang wrote:
> > On 2014/10/30 10:53:01, aandrey wrote:
> > > https://codereview.chromium.org/653023004/diff/1/src/object-observe.js
> > > File src/object-observe.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/653023004/diff/1/src/object-observe.js#newcod...
> > > src/object-observe.js:574: %DebugPushPromise(null);
> > > But should not we patch Isolate::DoThrow() instead?
> > > 
> > > Maybe we just call smth like %SetVerboseNextTryCatch() before entering the
> > > try-catch block to make it alike the verbose v8::TryCatch? Message
reporting
> > and
> > > debug event will work naturally w/o any changes.
> > 
> > Something like %SetVerboseNextTryCatch is not easily implemented. We push
> > try-catch handlers in generated code onto the stack, so to implement
something
> > like verbosity, we would have to involve the parser and change
> > platform-dependent code. At this point, I also have no idea what would
happen
> in
> > turbo fan code. So it's just a lot easier to piggyback on the existing
> > infrastructure for promises.
> > 
> > If performance becomes a problem, we could implement %DebugPushPromise and
> > %DebugPopPromise in generated code and change the data structure of the
> promise
> > stack.
> 
> Can we remove the try-catch completely in favor of:
> - extending %_CallFunction() to make a runtime call in a v8::TryCatch scope
> - introducing smth like %_CallFunctionInTryCatch()
> - introducing an intermediate runtime function that would create a verbose
> TryCatch and do whatever %_CallFunction() does
> ?

Also note, that the problem with the %DebugPushPromise solution is that it does
not handle message reporting (i.e. window.onerror callback).
That's why we had to introduce Isolate::SetPromiseRejectCallback for promises,
for example.
But we don't want to introduce yet another API for exceptions in O.o callbacks,
do we?

I think message reporting should be solved first, and debug event will be
naturally resolved as a side effect.

[aandrey, 2014-10-30 11:16:06]

On 2014/10/30 11:05:32, aandrey wrote:
> On 2014/10/30 10:59:46, Yang wrote:
> > On 2014/10/30 10:53:01, aandrey wrote:
> > > https://codereview.chromium.org/653023004/diff/1/src/object-observe.js
> > > File src/object-observe.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/653023004/diff/1/src/object-observe.js#newcod...
> > > src/object-observe.js:574: %DebugPushPromise(null);
> > > But should not we patch Isolate::DoThrow() instead?
> > > 
> > > Maybe we just call smth like %SetVerboseNextTryCatch() before entering the
> > > try-catch block to make it alike the verbose v8::TryCatch? Message
reporting
> > and
> > > debug event will work naturally w/o any changes.
> > 
> > Something like %SetVerboseNextTryCatch is not easily implemented. We push
> > try-catch handlers in generated code onto the stack, so to implement
something
> > like verbosity, we would have to involve the parser and change
> > platform-dependent code. At this point, I also have no idea what would
happen
> in
> > turbo fan code. So it's just a lot easier to piggyback on the existing
> > infrastructure for promises.
> > 
> > If performance becomes a problem, we could implement %DebugPushPromise and
> > %DebugPopPromise in generated code and change the data structure of the
> promise
> > stack.
> 
> Can we remove the try-catch completely in favor of:
> - extending %_CallFunction() to make a runtime call in a v8::TryCatch scope
> - introducing smth like %_CallFunctionInTryCatch()
> - introducing an intermediate runtime function that would create a verbose
> TryCatch and do whatever %_CallFunction() does
> ?

- introducing %PushTryCatch(bool verbose)/%PopTryCatch to push/pop v8::TryCatch
to the stack.

[Yang, 2014-10-30 11:47:13]

On 2014/10/30 11:16:06, aandrey wrote:
> On 2014/10/30 11:05:32, aandrey wrote:
> > On 2014/10/30 10:59:46, Yang wrote:
> > > On 2014/10/30 10:53:01, aandrey wrote:
> > > > https://codereview.chromium.org/653023004/diff/1/src/object-observe.js
> > > > File src/object-observe.js (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/653023004/diff/1/src/object-observe.js#newcod...
> > > > src/object-observe.js:574: %DebugPushPromise(null);
> > > > But should not we patch Isolate::DoThrow() instead?
> > > > 
> > > > Maybe we just call smth like %SetVerboseNextTryCatch() before entering
the
> > > > try-catch block to make it alike the verbose v8::TryCatch? Message
> reporting
> > > and
> > > > debug event will work naturally w/o any changes.
> > > 
> > > Something like %SetVerboseNextTryCatch is not easily implemented. We push
> > > try-catch handlers in generated code onto the stack, so to implement
> something
> > > like verbosity, we would have to involve the parser and change
> > > platform-dependent code. At this point, I also have no idea what would
> happen
> > in
> > > turbo fan code. So it's just a lot easier to piggyback on the existing
> > > infrastructure for promises.
> > > 
> > > If performance becomes a problem, we could implement %DebugPushPromise and
> > > %DebugPopPromise in generated code and change the data structure of the
> > promise
> > > stack.
> > 
> > Can we remove the try-catch completely in favor of:
> > - extending %_CallFunction() to make a runtime call in a v8::TryCatch scope
> > - introducing smth like %_CallFunctionInTryCatch()
> > - introducing an intermediate runtime function that would create a verbose
> > TryCatch and do whatever %_CallFunction() does
> > ?
> 
> - introducing %PushTryCatch(bool verbose)/%PopTryCatch to push/pop
v8::TryCatch
> to the stack.

The idea with %CallFunctionInTryCatch() is definitely something we should try,
since it would solve the message delivery issue. But I would like to decouple
this CL from that.

[Yang, 2014-10-30 11:59:29]

On 2014/10/30 11:47:13, Yang wrote:
> On 2014/10/30 11:16:06, aandrey wrote:
> > On 2014/10/30 11:05:32, aandrey wrote:
> > > On 2014/10/30 10:59:46, Yang wrote:
> > > > On 2014/10/30 10:53:01, aandrey wrote:
> > > > > https://codereview.chromium.org/653023004/diff/1/src/object-observe.js
> > > > > File src/object-observe.js (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/653023004/diff/1/src/object-observe.js#newcod...
> > > > > src/object-observe.js:574: %DebugPushPromise(null);
> > > > > But should not we patch Isolate::DoThrow() instead?
> > > > > 
> > > > > Maybe we just call smth like %SetVerboseNextTryCatch() before entering
> the
> > > > > try-catch block to make it alike the verbose v8::TryCatch? Message
> > reporting
> > > > and
> > > > > debug event will work naturally w/o any changes.
> > > > 
> > > > Something like %SetVerboseNextTryCatch is not easily implemented. We
push
> > > > try-catch handlers in generated code onto the stack, so to implement
> > something
> > > > like verbosity, we would have to involve the parser and change
> > > > platform-dependent code. At this point, I also have no idea what would
> > happen
> > > in
> > > > turbo fan code. So it's just a lot easier to piggyback on the existing
> > > > infrastructure for promises.
> > > > 
> > > > If performance becomes a problem, we could implement %DebugPushPromise
and
> > > > %DebugPopPromise in generated code and change the data structure of the
> > > promise
> > > > stack.
> > > 
> > > Can we remove the try-catch completely in favor of:
> > > - extending %_CallFunction() to make a runtime call in a v8::TryCatch
scope
> > > - introducing smth like %_CallFunctionInTryCatch()
> > > - introducing an intermediate runtime function that would create a verbose
> > > TryCatch and do whatever %_CallFunction() does
> > > ?
> > 
> > - introducing %PushTryCatch(bool verbose)/%PopTryCatch to push/pop
> v8::TryCatch
> > to the stack.
> 
> The idea with %CallFunctionInTryCatch() is definitely something we should try,
> since it would solve the message delivery issue. But I would like to decouple
> this CL from that.

One thing that %CallFunctionInTryCatch() wouldn't solve is when a Promise is
rejected not by throwing, but by calling reject or returning a rejected Promise.
That way, no try-catch is actually involved, and reject still needs to trigger
an API callback, which cannot use v8::TryCatch to get the message. :/

[aandrey, 2014-11-01 15:31:42]

Please look at my version here: https://codereview.chromium.org/692313003/