[liveedit]: fail to patch if target is outside of async function on stack

[liveedit]: fail to patch if target is outside of async function on stack

If an active generator is found on the stack (FUNCTION_BLOCKED_UNDER_GENERATOR),
and the target function is not found on top of that generator, add the error.

Based on test by wingo@igalia.com and littledan@chromium.org
(https://codereview.chromium.org/2035643003/)

LOG=N
BUG=v8:4483
R=yangguo@chromium.org, littledan@chromium.org

Committed: https://crrev.com/fd4d385b6d5cd710921332310204dfcd5c7ab762
Cr-Commit-Position: refs/heads/master@{#37000}

[caitp (gmail), 2016-06-10 13:06:22]

PTAL ---

This does not actually fix the issue with the getEvalPosition accessor, it's
still possible for that to cause problems if an exception is thrown in an eval'd
AsyncFunction, as far as I can tell.

But it does make the test behave more or less as expected, I think

[caitp (gmail), 2016-06-10 13:08:17]

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

https://codereview.chromium.org/2058733002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:1629: void set_status(LiveEdit::FunctionPatchabilityStatus
status) {
this stuff might actually not be necessary, since the test turned out to just be
wrong (but I'm pretty sure the 3rd assertThrows() will still fail without this)

[Dan Ehrenberg, 2016-06-10 13:14:41]

Do you think you could write a test for the case you're concerned about?

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

https://codereview.chromium.org/2058733002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:1629: void set_status(LiveEdit::FunctionPatchabilityStatus
status) {
On 2016/06/10 at 13:08:16, caitp wrote:
> this stuff might actually not be necessary, since the test turned out to just
be wrong (but I'm pretty sure the 3rd assertThrows() will still fail without
this)

What do you mean the test turned out to be wrong?

[caitp (gmail), 2016-06-10 13:18:45]

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

https://codereview.chromium.org/2058733002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:1629: void set_status(LiveEdit::FunctionPatchabilityStatus
status) {
On 2016/06/10 13:14:41, Dan Ehrenberg wrote:
> On 2016/06/10 at 13:08:16, caitp wrote:
> > this stuff might actually not be necessary, since the test turned out to
just
> be wrong (but I'm pretty sure the 3rd assertThrows() will still fail without
> this)
> 
> What do you mean the test turned out to be wrong?

The assertion that the last promise value would be "'Koala'" is in fact
incorrect, since it (correctly) resolves the Promise with "'Capybara'" (the last
edited string in asyncfn)

I fixed that and added a new assertion to verify that fun_inside was patched as
expected

But that said, yeah, this crash can still happen, this change just makes the
test behave as expected.

[caitp (gmail), 2016-06-14 15:02:08]

Ping --- I have a fix for the CHECK failure in the latest CL.

The issue seems to be caused by the following:

1. function producing the exception is compiled from source (eval)
2. source of function must change (I couldn't reproduce this without liveedit)
3. exception is thrown after resumption of the generator (the stack trace is
empty of JS frames, so there's nothing to get a valid position from --- could be
fixed better later on once Zones are a thing, if that happens)

---

The other changes to liveedit.cc are intended to make this behave more in line
with what I think is expected (but you tell me).

[Dan Ehrenberg, 2016-06-14 16:36:01]

https://codereview.chromium.org/2058733002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/2058733002/diff/20001/src/builtins.cc#newcode...
src/builtins.cc:4514: USE(position);
Should we be doing this for generators as well? Can a generator's execution be
the only JS frame on the stack and meet all of these conditions?

[caitp (gmail), 2016-06-14 16:48:51]

On 2016/06/14 16:36:01, Dan Ehrenberg wrote:
> https://codereview.chromium.org/2058733002/diff/20001/src/builtins.cc
> File src/builtins.cc (right):
> 
>
https://codereview.chromium.org/2058733002/diff/20001/src/builtins.cc#newcode...
> src/builtins.cc:4514: USE(position);
> Should we be doing this for generators as well? Can a generator's execution be
> the only JS frame on the stack and meet all of these conditions?

The difference with generators is, when they're resumed asynchronously, there
should always be a "real" javascript frame on the stack, so I don't think you
can repro this with generators. At least I wasn't able to by converting the test
to use generators + Promises from JS

[Dan Ehrenberg, 2016-06-14 17:07:00]

lgtm

[Yang, 2016-06-15 09:09:38]

On 2016/06/14 17:07:00, Dan Ehrenberg wrote:
> lgtm

lgtm.

[caitp (gmail), 2016-06-15 12:21:11]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2016-06-15 12:21:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2058733002/20001

[commit-bot: I haz the power, 2016-06-15 12:23:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-15 12:23:10]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-15 12:25:36]

Description was changed from

==========
[liveedit]: fail to patch if target is outside of async function on stack

If an active generator is found on the stack (FUNCTION_BLOCKED_UNDER_GENERATOR),
and the target function is not found on top of that generator, add the error.

Based on test by wingo@igalia.com and littledan@chromium.org
(https://codereview.chromium.org/2035643003/)

LOG=N
BUG=v8:4483
R=yangguo@chromium.org, littledan@chromium.org
==========

to

==========
[liveedit]: fail to patch if target is outside of async function on stack

If an active generator is found on the stack (FUNCTION_BLOCKED_UNDER_GENERATOR),
and the target function is not found on top of that generator, add the error.

Based on test by wingo@igalia.com and littledan@chromium.org
(https://codereview.chromium.org/2035643003/)

LOG=N
BUG=v8:4483
R=yangguo@chromium.org, littledan@chromium.org

Committed: https://crrev.com/fd4d385b6d5cd710921332310204dfcd5c7ab762
Cr-Commit-Position: refs/heads/master@{#37000}
==========

[commit-bot: I haz the power, 2016-06-15 12:25:37]

Patchset 2 (id:??) landed as
https://crrev.com/fd4d385b6d5cd710921332310204dfcd5c7ab762
Cr-Commit-Position: refs/heads/master@{#37000}