harmony-scoping: Implement debugger support for script scope.

harmony-scoping: Implement debugger support for script scope.

We add a new ScopeType, ScopeType.Script. The scope with
ScopeType.Script is always present in the scope chain (ScopeIterator
fakes it if neededi - i.e. if ScriptContext for a script has not been
allocated since that script has no lexical declarations).
ScriptScope reflects ScriptContextTable.

R=yurys@chromium.org,yangguo@chromium.org
BUG=v8:3690
LOG=N

Committed: https://chromium.googlesource.com/v8/v8/+/28dd2e5e435eafe68737695a868cb46f1f9557f6

[Dmitry Lomov (no reviews), 2014-11-13 19:28:40]

PTAL

[Dmitry Lomov (no reviews), 2014-11-14 11:47:16]

dslomov@chromium.org changed reviewers:
- yangguo@chromium.org

[Dmitry Lomov (no reviews), 2014-11-14 11:49:15]

Looks like Yang is OOO today. Yury, maybe you could take a look? Thanks

[yurys, 2014-11-14 12:09:49]

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

[yurys, 2014-11-14 12:09:50]

+aandrey who worked on the debugger stuff recently.

[yurys, 2014-11-14 12:25:32]

https://codereview.chromium.org/726643002/diff/20001/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/726643002/diff/20001/src/mirror-debugger.js#n...
src/mirror-debugger.js:198: Script: 1,
This will break DevTools code as we rely on these constants in [1]. Please add
the new constant to the end of the list. Also I think DevTools code should be
prepared to work with the new scope type before landing this change. Andrey, can
you take care of this?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

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

https://codereview.chromium.org/726643002/diff/20001/src/runtime/runtime-debu...
src/runtime/runtime-debug.cc:1280: DCHECK(context_->IsScriptContext() ||
context_->IsNativeContext());
Why not return ScopeTypeScript only in case context_->IsScriptContext() ?

https://codereview.chromium.org/726643002/diff/20001/src/runtime/runtime-debu...
src/runtime/runtime-debug.cc:1297: // If we are at the native context and have
not yet seen script scope,
Can you educate me on how this works, I thought that ScriptContext will always
be before NativeContext in the chain, isn't that the case?

[aandrey, 2014-11-14 12:35:22]

https://codereview.chromium.org/726643002/diff/20001/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/726643002/diff/20001/src/mirror-debugger.js#n...
src/mirror-debugger.js:198: Script: 1,
Can you add the new scope at the end. Our code depends on the numbers:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Also please add a comment that enumeration values should be backward compatible.

[Dmitry Lomov (no reviews), 2014-11-14 13:56:51]

Addressed comments.

https://codereview.chromium.org/726643002/diff/20001/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/726643002/diff/20001/src/mirror-debugger.js#n...
src/mirror-debugger.js:198: Script: 1,
On 2014/11/14 12:25:32, yurys wrote:
> This will break DevTools code as we rely on these constants in [1]. Please add
> the new constant to the end of the list. Also I think DevTools code should be
> prepared to work with the new scope type before landing this change. Andrey,
can
> you take care of this?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Done.

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

https://codereview.chromium.org/726643002/diff/20001/src/runtime/runtime-debu...
src/runtime/runtime-debug.cc:1297: // If we are at the native context and have
not yet seen script scope,
On 2014/11/14 12:25:32, yurys wrote:
> Can you educate me on how this works, I thought that ScriptContext will always
> be before NativeContext in the chain, isn't that the case?

No. I tried to explain this in CL description but maybe it was not clear enough.
Let me try again.
At runtime, we only allocate ScriptContexts in case when they are not empty,
i.e. when scrips have top-level lexical declarations ('let' or 'const').
However, for the debugger we always create script scopes, since top-level
lexical declarations from previously loaded scripts should be visible from all
scripts. 
Here is an example: imagine a page with two scripts:
<html>
<script>let x = 1</script>
<script>console.log(x)</script>
</html>
The first script will have  ScriptContext allocated since it declares 'x' at a
top level.
The second script declares nothing at the top level so its ScriptContext would
be empty, therefore it is not allocated. However it still sees an 'x' declared
in the first script.
At runtime, when second script is executed, 'x' is looked up in
ScriptContextTable (which contains a reference to first script's ScriptContext).
At debug time, if user breaks inside the second script, this code creates a
Script scope for it even though there is no ScriptContext, and that scope
mirrors ScriptContextTable).

[yurys, 2014-11-14 15:02:02]

LGTM.

I filed a bug on supporting this in DevTools:
https://code.google.com/p/chromium/issues/detail?id=433324.

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

https://codereview.chromium.org/726643002/diff/20001/src/runtime/runtime-debu...
src/runtime/runtime-debug.cc:1297: // If we are at the native context and have
not yet seen script scope,
On 2014/11/14 13:56:51, Dmitry Lomov (chromium) wrote:
> On 2014/11/14 12:25:32, yurys wrote:
> > Can you educate me on how this works, I thought that ScriptContext will
always
> > be before NativeContext in the chain, isn't that the case?
> 
> No. I tried to explain this in CL description but maybe it was not clear
enough.
> Let me try again.
> At runtime, we only allocate ScriptContexts in case when they are not empty,
> i.e. when scrips have top-level lexical declarations ('let' or 'const').
> However, for the debugger we always create script scopes, since top-level
> lexical declarations from previously loaded scripts should be visible from all
> scripts. 
> Here is an example: imagine a page with two scripts:
> <html>
> <script>let x = 1</script>
> <script>console.log(x)</script>
> </html>
> The first script will have  ScriptContext allocated since it declares 'x' at a
> top level.
> The second script declares nothing at the top level so its ScriptContext would
> be empty, therefore it is not allocated. However it still sees an 'x' declared
> in the first script.
> At runtime, when second script is executed, 'x' is looked up in
> ScriptContextTable (which contains a reference to first script's
ScriptContext).
> At debug time, if user breaks inside the second script, this code creates a
> Script scope for it even though there is no ScriptContext, and that scope
> mirrors ScriptContextTable).

Gotcha, thanks for explaining!

https://codereview.chromium.org/726643002/diff/40001/src/runtime/runtime-debu...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/726643002/diff/40001/src/runtime/runtime-debu...
src/runtime/runtime-debug.cc:1257: seen_script_scope_ = scope_type ==
ScopeTypeScript;
You can move this out of the if.

[aandrey, 2014-11-14 15:58:47]

let me update DevTools code before landing

https://codereview.chromium.org/726643002/diff/20001/test/cctest/test-debug.cc
File test/cctest/test-debug.cc (right):

https://codereview.chromium.org/726643002/diff/20001/test/cctest/test-debug.c...
test/cctest/test-debug.cc:7634: CHECK_EQ(1, result->Int32Value());
nit: you can use ExpectInt32() here and below

https://codereview.chromium.org/726643002/diff/20001/test/mjsunit/debug-funct...
File test/mjsunit/debug-function-scopes.js (right):

https://codereview.chromium.org/726643002/diff/20001/test/mjsunit/debug-funct...
test/mjsunit/debug-function-scopes.js:47: Script: 1,
please update this also

https://codereview.chromium.org/726643002/diff/20001/test/mjsunit/harmony/deb...
File test/mjsunit/harmony/debug-function-scopes.js (right):

https://codereview.chromium.org/726643002/diff/20001/test/mjsunit/harmony/deb...
test/mjsunit/harmony/debug-function-scopes.js:50: Script: 1,
update

[Dmitry Lomov (no reviews), 2014-11-14 16:37:27]

Addressed nits and rebased patch for landing.
Will wait until DevTools are ready.

https://codereview.chromium.org/726643002/diff/20001/test/cctest/test-debug.cc
File test/cctest/test-debug.cc (right):

https://codereview.chromium.org/726643002/diff/20001/test/cctest/test-debug.c...
test/cctest/test-debug.cc:7634: CHECK_EQ(1, result->Int32Value());
On 2014/11/14 15:58:47, aandrey wrote:
> nit: you can use ExpectInt32() here and below

Done.

https://codereview.chromium.org/726643002/diff/20001/test/mjsunit/debug-funct...
File test/mjsunit/debug-function-scopes.js (right):

https://codereview.chromium.org/726643002/diff/20001/test/mjsunit/debug-funct...
test/mjsunit/debug-function-scopes.js:47: Script: 1,
On 2014/11/14 15:58:47, aandrey wrote:
> please update this also

All updated in patch set 3

https://codereview.chromium.org/726643002/diff/40001/src/runtime/runtime-debu...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/726643002/diff/40001/src/runtime/runtime-debu...
src/runtime/runtime-debug.cc:1257: seen_script_scope_ = scope_type ==
ScopeTypeScript;
On 2014/11/14 15:02:02, yurys wrote:
> You can move this out of the if.

Done.

[aandrey, 2014-11-14 19:50:41]

lgtm

https://codereview.chromium.org/726643002/diff/60001/src/runtime/runtime-debu...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/726643002/diff/60001/src/runtime/runtime-debu...
src/runtime/runtime-debug.cc:1257: seen_script_scope_ = scope_type ==
ScopeTypeScript;
can it change now from true to false?
maybe:
  if (scope_type == ScopeTypeScript) seen_script_scope_ = true;
?

[Dmitry Lomov (no reviews), 2014-11-14 20:29:11]

https://codereview.chromium.org/726643002/diff/60001/src/runtime/runtime-debu...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/726643002/diff/60001/src/runtime/runtime-debu...
src/runtime/runtime-debug.cc:1257: seen_script_scope_ = scope_type ==
ScopeTypeScript;
On 2014/11/14 19:50:41, aandrey wrote:
> can it change now from true to false?
> maybe:
>   if (scope_type == ScopeTypeScript) seen_script_scope_ = true;
> ?

No it actually cannot since it is always Native after Script, but fixed for
clarity.

[aandrey, 2014-11-17 10:20:08]

Now you can land this.

[Dmitry Lomov (no reviews), 2014-11-17 17:45:58]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2014-11-17 17:46:38]

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

[commit-bot: I haz the power, 2014-11-17 17:58:04]

Committed patchset #5 (id:80001)