Make sure debugger is ready for breakpoins when we process 'debugger' statement.

Make sure debugger is ready for breakpoins when we process 'debugger' statement.

On 'debugger' statement, if anything in debugger calls 'EnsureDebugInfo'
on a function, EnsureDebugInfo would compile and substitute code without
debug break slots. This causes weird behavior later when stepping fails
to work (see added test as an example).
This fix is to make sure the debugger is prepared for breakpoints in
that case as well.

Also adds extra testing for bug 468661.

R=yangguo@chromium.org,yurys@chromium.orh
BUG=v8:3990, chromium:468661
LOG=N

Committed: https://crrev.com/15ef61d46801eaf31ff6ebaa34dfe9f9d9697a47
Cr-Commit-Position: refs/heads/master@{#27502}

[Dmitry Lomov (no reviews), 2015-03-26 14:37:31]

PTAL

[Dmitry Lomov (no reviews), 2015-03-26 14:54:03]

dslomov@chromium.org changed reviewers:
+ yurys@chromium.org
- yurys@chromium.orh

[Yang, 2015-03-26 15:10:48]

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

https://codereview.chromium.org/1032353002/diff/1/src/debug.cc#newcode3085
src/debug.cc:3085: }
Can we put all of this into Runtime_DebugBreak?

[Dmitry Lomov (no reviews), 2015-03-26 15:51:17]

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

https://codereview.chromium.org/1032353002/diff/1/src/debug.cc#newcode3085
src/debug.cc:3085: }
On 2015/03/26 15:10:48, Yang wrote:
> Can we put all of this into Runtime_DebugBreak?

Done.

[yurys, 2015-03-26 19:44:49]

https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:24: !isolate->stack_guard()->CheckDebugBreak();
RequestDebugBreak can be called on another thread. It means that in the
beginning of this method CheckDebugBreak could be false while here it is already
true and is_debugger_statement will be false even though this method is called
on debugger; statement. We need a better condition for this or probably drop it
and always call PrepareForBreakPoints below.

[Dmitry Lomov (no reviews), 2015-03-27 08:50:33]

On 2015/03/26 19:44:49, yurys wrote:
>
https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
> File src/runtime/runtime-debug.cc (right):
> 
>
https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
> src/runtime/runtime-debug.cc:24: !isolate->stack_guard()->CheckDebugBreak();
> RequestDebugBreak can be called on another thread. It means that in the
> beginning of this method CheckDebugBreak could be false while here it is
already
> true and is_debugger_statement will be false even though this method is called
> on debugger; statement. We need a better condition for this or probably drop
it
> and always call PrepareForBreakPoints below.

Does it mean that the checks in HandleDebugBreak are also false:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/debug.cc&rc...

[Yang, 2015-03-27 09:10:21]

https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:24: !isolate->stack_guard()->CheckDebugBreak();
On 2015/03/26 19:44:48, yurys wrote:
> RequestDebugBreak can be called on another thread. It means that in the
> beginning of this method CheckDebugBreak could be false while here it is
already
> true and is_debugger_statement will be false even though this method is called
> on debugger; statement. We need a better condition for this or probably drop
it
> and always call PrepareForBreakPoints below.

How about we just check whether the debugger is active here, and if yes,
PrepareForBreakPoints here, and then call HandleDebugBreak? We can only end up
here via debugger statement, so we don't care about interrupts at all.

[Yang, 2015-03-27 09:44:10]

On 2015/03/27 09:10:21, Yang wrote:
>
https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
> File src/runtime/runtime-debug.cc (right):
> 
>
https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
> src/runtime/runtime-debug.cc:24: !isolate->stack_guard()->CheckDebugBreak();
> On 2015/03/26 19:44:48, yurys wrote:
> > RequestDebugBreak can be called on another thread. It means that in the
> > beginning of this method CheckDebugBreak could be false while here it is
> already
> > true and is_debugger_statement will be false even though this method is
called
> > on debugger; statement. We need a better condition for this or probably drop
> it
> > and always call PrepareForBreakPoints below.
> 
> How about we just check whether the debugger is active here, and if yes,
> PrepareForBreakPoints here, and then call HandleDebugBreak? We can only end up
> here via debugger statement, so we don't care about interrupts at all.

after offline discussion, first patch set lgtm.

[yurys, 2015-03-27 09:57:15]

On 2015/03/27 08:50:33, Dmitry Lomov (chromium) wrote:
> On 2015/03/26 19:44:49, yurys wrote:
> >
>
https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
> > File src/runtime/runtime-debug.cc (right):
> > 
> >
>
https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
> > src/runtime/runtime-debug.cc:24: !isolate->stack_guard()->CheckDebugBreak();
> > RequestDebugBreak can be called on another thread. It means that in the
> > beginning of this method CheckDebugBreak could be false while here it is
> already
> > true and is_debugger_statement will be false even though this method is
called
> > on debugger; statement. We need a better condition for this or probably drop
> it
> > and always call PrepareForBreakPoints below.
> 
> Does it mean that the checks in HandleDebugBreak are also false:
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/debug.cc&rc...

Yes, they is also a race.

[yurys, 2015-03-27 09:58:31]

On 2015/03/27 09:10:21, Yang wrote:
>
https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
> File src/runtime/runtime-debug.cc (right):
> 
>
https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
> src/runtime/runtime-debug.cc:24: !isolate->stack_guard()->CheckDebugBreak();
> On 2015/03/26 19:44:48, yurys wrote:
> > RequestDebugBreak can be called on another thread. It means that in the
> > beginning of this method CheckDebugBreak could be false while here it is
> already
> > true and is_debugger_statement will be false even though this method is
called
> > on debugger; statement. We need a better condition for this or probably drop
> it
> > and always call PrepareForBreakPoints below.
> 
> How about we just check whether the debugger is active here, and if yes,
> PrepareForBreakPoints here, and then call HandleDebugBreak? We can only end up
> here via debugger statement, so we don't care about interrupts at all.

This sounds like a more robust approach to me, what are the downsides that made
you prefer PS#1?

[Dmitry Lomov (no reviews), 2015-03-27 10:03:52]

On 2015/03/27 09:44:10, Yang wrote:
> On 2015/03/27 09:10:21, Yang wrote:
> >
>
https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
> > File src/runtime/runtime-debug.cc (right):
> > 
> >
>
https://codereview.chromium.org/1032353002/diff/20001/src/runtime/runtime-deb...
> > src/runtime/runtime-debug.cc:24: !isolate->stack_guard()->CheckDebugBreak();
> > On 2015/03/26 19:44:48, yurys wrote:
> > > RequestDebugBreak can be called on another thread. It means that in the
> > > beginning of this method CheckDebugBreak could be false while here it is
> > already
> > > true and is_debugger_statement will be false even though this method is
> called
> > > on debugger; statement. We need a better condition for this or probably
drop
> > it
> > > and always call PrepareForBreakPoints below.
> > 
> > How about we just check whether the debugger is active here, and if yes,
> > PrepareForBreakPoints here, and then call HandleDebugBreak? We can only end
up
> > here via debugger statement, so we don't care about interrupts at all.
> 
> after offline discussion, first patch set lgtm.

Just to expand on offline discussion:
1. We should only prepare for breakpoints if all the tests up to this line in
Debug::HandleDebugBreak succeed:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/debug.cc&l=...
2. Any races with RequestDebugBreak seem to be as bad as what we already have:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/debug.cc&l=...

This is a fix for the actual bug. It seems to be that it would be nice to
revisit the architecture of this entire interation at some point.

[yurys, 2015-03-27 13:43:32]

LGTM. Thanks for explaining this, Dmitry!

[Dmitry Lomov (no reviews), 2015-03-27 17:57:11]

layout tests are happy, landing.

[Dmitry Lomov (no reviews), 2015-03-27 17:57:16]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-03-27 17:57:16]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1032353002/#ps60001
(title: "TODO to clean-up threading issues added")

[commit-bot: I haz the power, 2015-03-27 17:57:29]

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

[commit-bot: I haz the power, 2015-03-27 18:33:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-27 18:33:25]

Patchset 4 (id:??) landed as
https://crrev.com/15ef61d46801eaf31ff6ebaa34dfe9f9d9697a47
Cr-Commit-Position: refs/heads/master@{#27502}