[inspector] introduced v8::debug::EntriesPreview for inspector

[inspector] introduced v8::debug::EntriesPreview for inspector

- entries preview available even if debugger agent is disabled,
- less deprecated mirrors usage in debugger-script.js
- no usage of debugger context - zero probability of leaking it.
- better test coverage.

BUG=v8:5510
R=yangguo@chromium.org,jgruber@chromium.org,alph@chromium.org,luoe@chromium.org

Review-Url: https://codereview.chromium.org/2672213002
Cr-Commit-Position: refs/heads/master@{#42978}
Committed: https://chromium.googlesource.com/v8/v8/+/6e17719e7963b7ab56a57a1425e6e8c40bad9831

[kozy, 2017-02-06 08:09:18]

please take a look!

[kozy, 2017-02-06 08:10:04]

FYI: https://codereview.chromium.org/2674143003/ - related change in layout
tests expectations.

[kozy, 2017-02-06 08:10:09]

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

[commit-bot: I haz the power, 2017-02-06 08:10:17]

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, 2017-02-06 08:31:26]

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

[commit-bot: I haz the power, 2017-02-06 08:31:29]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[kozy, 2017-02-06 08:41:58]

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

[commit-bot: I haz the power, 2017-02-06 08:42:04]

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, 2017-02-06 08:56:54]

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

[commit-bot: I haz the power, 2017-02-06 08:56:55]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)

[kozy, 2017-02-06 09:09:22]

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

[commit-bot: I haz the power, 2017-02-06 09:09:30]

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

[kozy, 2017-02-06 09:09:39]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-02-06 09:35:00]

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

[commit-bot: I haz the power, 2017-02-06 09:35:01]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-02-06 10:41:30]

lgtm with comments.

https://codereview.chromium.org/2672213002/diff/120001/src/runtime/runtime-co...
File src/runtime/runtime-collections.cc (right):

https://codereview.chromium.org/2672213002/diff/120001/src/runtime/runtime-co...
src/runtime/runtime-collections.cc:259:
RUNTIME_FUNCTION(Runtime_GetWeakMapEntries) {
Can we move that to JSWeakMap?

https://codereview.chromium.org/2672213002/diff/120001/src/runtime/runtime-co...
src/runtime/runtime-collections.cc:348: Handle<JSArray>
Runtime::GetWeakSetEntries(Isolate* isolate,
Can we move that to JSWeakSet?

[jgruber, 2017-02-06 12:40:39]

Very nice, lgtm.

https://codereview.chromium.org/2672213002/diff/120001/src/runtime/runtime-co...
File src/runtime/runtime-collections.cc (right):

https://codereview.chromium.org/2672213002/diff/120001/src/runtime/runtime-co...
src/runtime/runtime-collections.cc:259:
RUNTIME_FUNCTION(Runtime_GetWeakMapEntries) {
On 2017/02/06 10:41:30, Yang wrote:
> Can we move that to JSWeakMap?

+1

[kozy, 2017-02-06 18:39:26]

I generalized and moved GetEntries for WeakSet and WeakMap to 
JSWeakCollection.

Alexey or Erik, please take a look. :)

https://codereview.chromium.org/2672213002/diff/120001/src/runtime/runtime-co...
File src/runtime/runtime-collections.cc (right):

https://codereview.chromium.org/2672213002/diff/120001/src/runtime/runtime-co...
src/runtime/runtime-collections.cc:259:
RUNTIME_FUNCTION(Runtime_GetWeakMapEntries) {
On 2017/02/06 10:41:30, Yang wrote:
> Can we move that to JSWeakMap?

Done.

https://codereview.chromium.org/2672213002/diff/120001/src/runtime/runtime-co...
src/runtime/runtime-collections.cc:348: Handle<JSArray>
Runtime::GetWeakSetEntries(Isolate* isolate,
On 2017/02/06 10:41:30, Yang wrote:
> Can we move that to JSWeakSet?

Done.

[kozy, 2017-02-06 18:39:35]

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

[commit-bot: I haz the power, 2017-02-06 18:39: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, 2017-02-06 19:09:25]

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

[commit-bot: I haz the power, 2017-02-06 19:09:26]

Dry run: This issue passed the CQ dry run.

[luoe, 2017-02-06 22:38:50]

Why do we allow entryPreviews to be shown when debugger agent is disabled, but
not other previews?  Are we assuming that entryPreviews are less common?

The rest looks good!  lgtm

https://codereview.chromium.org/2672213002/diff/140001/src/inspector/v8-debug...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2672213002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:69: if (!entries->Get(context, i +
1).ToLocal(&value)) continue;
If we get to this continue, that means there is a key but no value.  How could
this happen?

[kozy, 2017-02-06 22:58:35]

On 2017/02/06 22:38:50, luoe wrote:
> Why do we allow entryPreviews to be shown when debugger agent is disabled, but
> not other previews?  Are we assuming that entryPreviews are less common?

We'd like to show as much as possible previews when Debugger is disabled.
Current implementation requires enabled debugger because it uses mirrors API and
debugger-script.js (isn't available when debugger is disabled) to get entries
preview. With this CL there are three kinds of previews which are available only
with debugger left:
- generator scopes,
- function scopes,
- generator location (this one could be migrated easily:
https://codereview.chromium.org/2678143002/).

[kozy, 2017-02-06 22:58:41]

https://codereview.chromium.org/2672213002/diff/140001/src/inspector/v8-debug...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2672213002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:69: if (!entries->Get(context, i +
1).ToLocal(&value)) continue;
On 2017/02/06 22:38:50, luoe wrote:
> If we get to this continue, that means there is a key but no value.  How could
> this happen?

ToLocal here could fail if something wrong happens during array[i+1] call,
usually it means some rare cases like out-of-memory. We have three possible
options here:
- ignore value and and add key only,
- ignore entire entry.
I think about this and will change this code to ignore entire entry to provide
some guarantees for injected-script-source.js code.

[kozy, 2017-02-06 23:04:42]

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

[commit-bot: I haz the power, 2017-02-06 23:04:59]

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, 2017-02-06 23:33:23]

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

[commit-bot: I haz the power, 2017-02-06 23:33:24]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-02-07 00:53:33]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-02-07 00:53:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, jgruber@chromium.org, luoe@chromium.org
Link to the patchset: https://codereview.chromium.org/2672213002/#ps160001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-02-07 00:53:45]

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, 2017-02-07 02:54:13]

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

[commit-bot: I haz the power, 2017-02-07 02:54:14]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_TIMED_OUT, no build URL)

[kozy, 2017-02-07 02:55:41]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2017-02-07 02:55:44]

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, 2017-02-07 04:56:06]

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

[commit-bot: I haz the power, 2017-02-07 04:56:07]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_TIMED_OUT, no build URL)

[kozy, 2017-02-07 05:30:21]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2017-02-07 05:30:27]

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, 2017-02-07 07:31:00]

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

[commit-bot: I haz the power, 2017-02-07 07:31:02]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_TIMED_OUT, no build URL)

[Michael Achenbach, 2017-02-07 07:43:15]

The CQ bit was checked by machenbach@chromium.org

[commit-bot: I haz the power, 2017-02-07 07:43:24]

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, 2017-02-07 07:46:22]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486453395297600,
"parent_rev": "a5298c6f525cb90c46e8f16966e162f38117c012", "commit_rev":
"6e17719e7963b7ab56a57a1425e6e8c40bad9831"}

[commit-bot: I haz the power, 2017-02-07 07:46:27]

Description was changed from

==========
[inspector] introduced v8::debug::EntriesPreview for inspector

- entries preview available even if debugger agent is disabled,
- less deprecated mirrors usage in debugger-script.js
- no usage of debugger context - zero probability of leaking it.
- better test coverage.

BUG=v8:5510
R=yangguo@chromium.org,jgruber@chromium.org,alph@chromium.org,luoe@chromium.org
==========

to

==========
[inspector] introduced v8::debug::EntriesPreview for inspector

- entries preview available even if debugger agent is disabled,
- less deprecated mirrors usage in debugger-script.js
- no usage of debugger context - zero probability of leaking it.
- better test coverage.

BUG=v8:5510
R=yangguo@chromium.org,jgruber@chromium.org,alph@chromium.org,luoe@chromium.org

Review-Url: https://codereview.chromium.org/2672213002
Cr-Commit-Position: refs/heads/master@{#42978}
Committed:
https://chromium.googlesource.com/v8/v8/+/6e17719e7963b7ab56a57a1425e6e8c40ba...
==========

[commit-bot: I haz the power, 2017-02-07 07:46:29]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/v8/v8/+/6e17719e7963b7ab56a57a1425e6e8c40ba...