[inspector] Expose scopes for suspended generator objects

[inspector] Expose scopes for suspended generator objects

This exposes scopes for suspended generator objects by adding a
[[Scopes]] internal property to generator objects, similar to how
scopes for functions currently not on the stack are handled.

BUG=chromium:667286
Committed: https://crrev.com/f0d3cf5baeb9df05c29dafa1b1840eb0595d7abc
Cr-Commit-Position: refs/heads/master@{#41244}

[jgruber, 2016-11-21 13:51:34]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 13:51:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 14:26:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 14:26:51]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-11-21 15:03:57]

jgruber@chromium.org changed reviewers:
+ kozyatinskiy@chromium.org, yangguo@chromium.org

[jgruber, 2016-11-21 15:03:57]

This exposes suspended generator scopes similar to how scopes for function
variables are exposed. 

Ideally, we would exposed these as Debugger.Scope instances in the future.

PTAL

[kozy, 2016-11-21 16:13:16]

Please mark as NeedsManualRebaseline
inspector/sources/debugger-ui/function-generator-details.html test before
landing.
And could you add inspector test? We prefer to have inspector test at least for
every CL that changes protocol.

[dgozman, 2016-11-21 17:26:58]

On 2016/11/21 16:13:16, kozyatinskiy wrote:
> Please mark as NeedsManualRebaseline
> inspector/sources/debugger-ui/function-generator-details.html test before
> landing.
> And could you add inspector test? We prefer to have inspector test at least
for
> every CL that changes protocol.

Alexey, do we need additional support on frontend for this? We'll also need a
test which dumps ObjectPropertiesSection with generator scopes as well.

[kozy, 2016-11-21 17:50:07]

On 2016/11/21 17:26:58, dgozman wrote:
> On 2016/11/21 16:13:16, kozyatinskiy wrote:
> > Please mark as NeedsManualRebaseline
> > inspector/sources/debugger-ui/function-generator-details.html test before
> > landing.
> > And could you add inspector test? We prefer to have inspector test at least
> for
> > every CL that changes protocol.
> 
> Alexey, do we need additional support on frontend for this? We'll also need a
> test which dumps ObjectPropertiesSection with generator scopes as well.

We mark scopes array and each scope with special internal subtype and already
have logic to render them correctly on frontend side.
inspector/sources/debugger-ui/function-generator-details.html test checks that
we output something like Scopes[2] for scope array in generator object. We can
tune this test to output expanded Scopes property.

[Yang, 2016-11-22 09:29:49]

On 2016/11/21 17:50:07, kozyatinskiy wrote:
> On 2016/11/21 17:26:58, dgozman wrote:
> > On 2016/11/21 16:13:16, kozyatinskiy wrote:
> > > Please mark as NeedsManualRebaseline
> > > inspector/sources/debugger-ui/function-generator-details.html test before
> > > landing.
> > > And could you add inspector test? We prefer to have inspector test at
least
> > for
> > > every CL that changes protocol.
> > 
> > Alexey, do we need additional support on frontend for this? We'll also need
a
> > test which dumps ObjectPropertiesSection with generator scopes as well.
> 
> We mark scopes array and each scope with special internal subtype and already
> have logic to render them correctly on frontend side.
> inspector/sources/debugger-ui/function-generator-details.html test checks that
> we output something like Scopes[2] for scope array in generator object. We can
> tune this test to output expanded Scopes property.

lgtm.

[jgruber, 2016-11-22 14:53:25]

On 2016/11/21 16:13:16, kozyatinskiy wrote:
> Please mark as NeedsManualRebaseline
> inspector/sources/debugger-ui/function-generator-details.html test before
> landing.

https://codereview.chromium.org/2521823004/

> And could you add inspector test? We prefer to have inspector test at least
for
> every CL that changes protocol.

Done.

[jgruber, 2016-11-22 14:53:26]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 14:53:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2016-11-22 14:57:32]

https://codereview.chromium.org/2516973003/diff/20001/test/inspector/debugger...
File test/inspector/debugger/suspended-generator-scopes.js (right):

https://codereview.chromium.org/2516973003/diff/20001/test/inspector/debugger...
test/inspector/debugger/suspended-generator-scopes.js:58:
InspectorTest.logMessage(scopes);
If we attempt to get the properties of e.g. the innermost scope here, inspector
comes back with an error. I suspect the object is already gone by the time we
try to fetch its details. Any ideas?


            var inner_scope = scopes[0].value;
            print(inner_scope.objectId);
            return Protocol.Runtime.getProperties(
                { objectId : inner_scope.objectId });

-->

{"injectedScriptId":1,"id":29}

{
    error : {
        code : -32000
        message : Could not find object with given id
    }
    id : <messageId>
}

[commit-bot: I haz the power, 2016-11-22 15:21:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 15:21:09]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-11-22 19:26:37]

https://codereview.chromium.org/2516973003/diff/20001/test/inspector/debugger...
File test/inspector/debugger/suspended-generator-scopes.js (right):

https://codereview.chromium.org/2516973003/diff/20001/test/inspector/debugger...
test/inspector/debugger/suspended-generator-scopes.js:58:
InspectorTest.logMessage(scopes);
On 2016/11/22 14:57:32, jgruber wrote:
> If we attempt to get the properties of e.g. the innermost scope here,
inspector
> comes back with an error. I suspect the object is already gone by the time we
> try to fetch its details. Any ideas?
> 
> 
>             var inner_scope = scopes[0].value;
>             print(inner_scope.objectId);
>             return Protocol.Runtime.getProperties(
>                 { objectId : inner_scope.objectId });
> 
> -->
> 
> {"injectedScriptId":1,"id":29}
> 
> {
>     error : {
>         code : -32000
>         message : Could not find object with given id
>     }
>     id : <messageId>
> }

I think the problem here that you call Debugger.resume before
Runtime.getProperties is finished. On Debugger.resume we clear "backtrace"
object group that contains all objects related to pause, e.g. scopes.
To check this I wrote test: http://pastebin.com/raw/2gWSNE92 feel free to use
it.

And could you add a test that not on pause we can get scopes too?

[jgruber, 2016-11-23 08:47:29]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 08:47:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2016-11-23 08:53:26]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 08:53:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2016-11-23 08:58:59]

On 2016/11/22 19:26:37, kozyatinskiy wrote:
>
https://codereview.chromium.org/2516973003/diff/20001/test/inspector/debugger...
> File test/inspector/debugger/suspended-generator-scopes.js (right):
> 
>
https://codereview.chromium.org/2516973003/diff/20001/test/inspector/debugger...
> test/inspector/debugger/suspended-generator-scopes.js:58:
> InspectorTest.logMessage(scopes);
> On 2016/11/22 14:57:32, jgruber wrote:
> > If we attempt to get the properties of e.g. the innermost scope here,
> inspector
> > comes back with an error. I suspect the object is already gone by the time
we
> > try to fetch its details. Any ideas?
> > 
> > 
> >             var inner_scope = scopes[0].value;
> >             print(inner_scope.objectId);
> >             return Protocol.Runtime.getProperties(
> >                 { objectId : inner_scope.objectId });
> > 
> > -->
> > 
> > {"injectedScriptId":1,"id":29}
> > 
> > {
> >     error : {
> >         code : -32000
> >         message : Could not find object with given id
> >     }
> >     id : <messageId>
> > }
> 
> I think the problem here that you call Debugger.resume before
> Runtime.getProperties is finished. On Debugger.resume we clear "backtrace"
> object group that contains all objects related to pause, e.g. scopes.
> To check this I wrote test: http://pastebin.com/raw/2gWSNE92 feel free to use
> it.
> 
> And could you add a test that not on pause we can get scopes too?


Thanks, that seems to have been the case. I refactored the test and added one
for 
the non-paused state in which the generator is inspected through
Runtime.evaluate.

[commit-bot: I haz the power, 2016-11-23 09:17:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 09:17:59]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-11-23 15:55:01]

thanks! lgtm.

[jgruber, 2016-11-24 07:30:28]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2016-11-24 07:30:29]

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/2516973003/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2016-11-24 07:30:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 07:32:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479972628701270,
"parent_rev": "8bc2e30553691a965c9fd1ab27f5c8ba5fb051d9", "commit_rev":
"9840bdc377234d384ee2122df6871fe4df022d57"}

[commit-bot: I haz the power, 2016-11-24 07:32:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-24 07:32:46]

Description was changed from

==========
[inspector] Expose scopes for suspended generator objects

This exposes scopes for suspended generator objects by adding a
[[Scopes]] internal property to generator objects, similar to how
scopes for functions currently not on the stack are handled.

BUG=chromium:667286
==========

to

==========
[inspector] Expose scopes for suspended generator objects

This exposes scopes for suspended generator objects by adding a
[[Scopes]] internal property to generator objects, similar to how
scopes for functions currently not on the stack are handled.

BUG=chromium:667286

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

[commit-bot: I haz the power, 2016-11-24 07:32:47]

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