[debugger] simplify fetching scripts for inspector.

[debugger] simplify fetching scripts for inspector.

The old code path is going to be removed with the debug context api.

R=kozyatinskiy@chromium.org

Committed: https://crrev.com/581614eeea743dc64739f14a982f396dfda852ba
Cr-Commit-Position: refs/heads/master@{#40768}

[kozy, 2016-10-31 14:59:11]

Thanks!

I think that you need to rebaseline
test/inspector/debugger/script-on-after-compile.js, because before we call gc
before GetCompiledScripts and collect "for (let i = 1; i < 20; ++i)
eval(`foo${i} = undefined`); gc();" script.
We got some issues from users when they can't find scripts when DevTools is
shown after page load so not calling gc looks good to me.

https://codereview.chromium.org/2465833002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2465833002/diff/1/src/api.cc#newcode8957
src/api.cc:8957: while (!(script =
i::Handle<i::Script>(iterator.Next())).is_null()) {
I think that i::Handle<i::Script>(Object*) tries to get isolate from passed
Object [1] and it will produce a crash. And will it work correctly if we pass
isolate as second argument to avoid mentioned crash? It looks for me that
is_null() check returns true only if handle was created with cstor without
arguments, otherwise it just allocates one more handle in handle scope, event if
it's null.

And I've question about how V8 works. Can we just not create a handle and
iterate through raw pointers?
i::Script* script = nullptr;
while ((script = iterator.Next()) != nullptr) {
...
if (script->HasValidSource())
scripts.Append(ToApiHandle<Script>(i::handle(script)));
}
It looks for me like nothing in loop code can call gc to move objects in memory.
What functions call are potentially can run gc?

[1] https://cs.chromium.org/chromium/src/v8/src/handles.h?rcl=1477898213&l=99

[Yang, 2016-11-02 12:25:55]

https://codereview.chromium.org/2465833002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2465833002/diff/1/src/api.cc#newcode8957
src/api.cc:8957: while (!(script =
i::Handle<i::Script>(iterator.Next())).is_null()) {
On 2016/10/31 14:59:11, kozyatinskiy wrote:
> I think that i::Handle<i::Script>(Object*) tries to get isolate from passed
> Object [1] and it will produce a crash. And will it work correctly if we pass
> isolate as second argument to avoid mentioned crash? It looks for me that
> is_null() check returns true only if handle was created with cstor without
> arguments, otherwise it just allocates one more handle in handle scope, event
if
> it's null.
> 
> And I've question about how V8 works. Can we just not create a handle and
> iterate through raw pointers?
> i::Script* script = nullptr;
> while ((script = iterator.Next()) != nullptr) {
> ...
> if (script->HasValidSource())
> scripts.Append(ToApiHandle<Script>(i::handle(script)));
> }
> It looks for me like nothing in loop code can call gc to move objects in
memory.
> What functions call are potentially can run gc?
> 
> [1] https://cs.chromium.org/chromium/src/v8/src/handles.h?rcl=1477898213&l=99

Good point. I changed the code after running tests. But we at least need the
handle to pass to ToApiHandle.

[Yang, 2016-11-02 12:26:17]

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

[commit-bot: I haz the power, 2016-11-02 12:26:23]

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-02 12:30:33]

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

[commit-bot: I haz the power, 2016-11-02 12:30:33]

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

[Yang, 2016-11-02 12:58:07]

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

[commit-bot: I haz the power, 2016-11-02 12:58:18]

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-02 13:02:47]

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

[commit-bot: I haz the power, 2016-11-02 13:02:48]

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

[kozy, 2016-11-03 01:23:36]

lgtm % comment in test

https://codereview.chromium.org/2465833002/diff/40001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2465833002/diff/40001/src/api.cc#newcode8951
src/api.cc:8951: i::DisallowHeapAllocation no_gc;
I'm just wondering. What this disallow heap allocation scope actually do? Will
it crash on heap allocation, or heap allocation will return something like empty
handler?

And I noticed that i::WeakFixedArray inside of i::Script::Iterator contains
i::DissalowHeapAllocation but only in debug build.

https://codereview.chromium.org/2465833002/diff/40001/test/inspector/debugger...
File test/inspector/debugger/script-on-after-compile.js (left):

https://codereview.chromium.org/2465833002/diff/40001/test/inspector/debugger...
test/inspector/debugger/script-on-after-compile.js:54: .then(() =>
Protocol.Runtime.evaluate({ expression: "for (let i = 1; i < 20; ++i)
eval(`foo${i} = undefined`); gc();" }))
I added this gc call here to check that v8-debugger-agent doesn't retain
scripts. Please revert it or remove following part of the test.

[Yang, 2016-11-03 07:47:09]

I fixed the failures. Turned out I forgot a GC in the new implementation.

https://codereview.chromium.org/2465833002/diff/40001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2465833002/diff/40001/src/api.cc#newcode8951
src/api.cc:8951: i::DisallowHeapAllocation no_gc;
On 2016/11/03 01:23:36, kozyatinskiy wrote:
> I'm just wondering. What this disallow heap allocation scope actually do? Will
> it crash on heap allocation, or heap allocation will return something like
empty
> handler?
> 
> And I noticed that i::WeakFixedArray inside of i::Script::Iterator contains
> i::DissalowHeapAllocation but only in debug build.

The DisallowHeapAllocation scope causes assertion failure in debug mode if
allocation happens inside the scope.

[kozy, 2016-11-03 17:24:36]

On 2016/11/03 07:47:09, Yang wrote:
> I fixed the failures. Turned out I forgot a GC in the new implementation.

Thank you!

Could we not run GC on get loaded scripts?
We got some issues from users when they open DevTools after page load finished
and can not find some scripts in sources panel. I think we can slightly improve
this case.

[Yang, 2016-11-03 17:29:28]

On 2016/11/03 17:24:36, kozyatinskiy wrote:
> On 2016/11/03 07:47:09, Yang wrote:
> > I fixed the failures. Turned out I forgot a GC in the new implementation.
> 
> Thank you!
> 
> Could we not run GC on get loaded scripts?
> We got some issues from users when they open DevTools after page load finished
> and can not find some scripts in sources panel. I think we can slightly
improve
> this case.

Should I then simply delete the part of the test that tests the gc behavior?

[kozy, 2016-11-03 17:40:03]

On 2016/11/03 17:29:28, Yang wrote:
> On 2016/11/03 17:24:36, kozyatinskiy wrote:
> > On 2016/11/03 07:47:09, Yang wrote:
> > > I fixed the failures. Turned out I forgot a GC in the new implementation.
> > 
> > Thank you!
> > 
> > Could we not run GC on get loaded scripts?
> > We got some issues from users when they open DevTools after page load
finished
> > and can not find some scripts in sources panel. I think we can slightly
> improve
> > this case.
> 
> Should I then simply delete the part of the test that tests the gc behavior?

My original idea was to check that we don't hold reported scripts on inspector
side. But I'll change this behavior in patch about getPossibleBreakpoints [1] so
you can remove it here or I'll remove it in my CL.

[1] https://codereview.chromium.org/2465553003/

[Yang, 2016-11-04 10:37:22]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-11-04 10:37:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org
Link to the patchset: https://codereview.chromium.org/2465833002/#ps100001
(title: "address comment")

[commit-bot: I haz the power, 2016-11-04 10:37:33]

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-04 11:06:42]

Committed patchset #6 (id:100001)

[Michael Achenbach, 2016-11-04 12:59:32]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2473273005/ by machenbach@chromium.org.

The reason for reverting is:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...

Rule of thumb: As long as the tests have not migrated yet, you most certainly
need a blink rebase whenever you change expectations..

[Michael Achenbach, 2016-11-04 13:01:11]

On 2016/11/04 12:59:32, machenbach (slow) wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/2473273005/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is:
>
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...
> 
> Rule of thumb: As long as the tests have not migrated yet, you most certainly
> need a blink rebase whenever you change expectations..

Though, looking at the changed blink expectations, it's not really obvious...

[Yang, 2016-11-04 13:05:38]

Description was changed from

==========
[debugger] simplify fetching scripts for inspector.

The old code path is going to be removed with the debug context api.

R=kozyatinskiy@chromium.org
==========

to

==========
[debugger] simplify fetching scripts for inspector.

The old code path is going to be removed with the debug context api.

R=kozyatinskiy@chromium.org
==========

[Yang, 2016-11-04 13:05:43]

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

[commit-bot: I haz the power, 2016-11-04 13:05:56]

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

[Yang, 2016-11-04 13:06:07]

On 2016/11/04 13:01:11, machenbach (slow) wrote:
> On 2016/11/04 12:59:32, machenbach (slow) wrote:
> > A revert of this CL (patchset #6 id:100001) has been created in
> > https://codereview.chromium.org/2473273005/ by
mailto:machenbach@chromium.org.
> > 
> > The reason for reverting is:
> >
>
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...
> > 
> > Rule of thumb: As long as the tests have not migrated yet, you most
certainly
> > need a blink rebase whenever you change expectations..
> 
> Though, looking at the changed blink expectations, it's not really obvious...

Alexey, I'm re-adding the GC since removing it seems to break blink layout
tests.

[commit-bot: I haz the power, 2016-11-04 13:29:45]

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

[commit-bot: I haz the power, 2016-11-04 13:29:48]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-11-04 13:53:10]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-11-04 13:53:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org
Link to the patchset: https://codereview.chromium.org/2465833002/#ps140001
(title: "add comment")

[commit-bot: I haz the power, 2016-11-04 13:53:20]

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-04 13:55:04]

Description was changed from

==========
[debugger] simplify fetching scripts for inspector.

The old code path is going to be removed with the debug context api.

R=kozyatinskiy@chromium.org
==========

to

==========
[debugger] simplify fetching scripts for inspector.

The old code path is going to be removed with the debug context api.

R=kozyatinskiy@chromium.org
==========

[commit-bot: I haz the power, 2016-11-04 13:55:05]

Committed patchset #8 (id:140001)

[kozy, 2016-11-04 15:05:44]

On 2016/11/04 13:06:07, Yang wrote:
> On 2016/11/04 13:01:11, machenbach (slow) wrote:
> > On 2016/11/04 12:59:32, machenbach (slow) wrote:
> > > A revert of this CL (patchset #6 id:100001) has been created in
> > > https://codereview.chromium.org/2473273005/ by
> mailto:machenbach@chromium.org.
> > > 
> > > The reason for reverting is:
> > >
> >
>
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...
> > > 
> > > Rule of thumb: As long as the tests have not migrated yet, you most
> certainly
> > > need a blink rebase whenever you change expectations..
> > 
> > Though, looking at the changed blink expectations, it's not really
obvious...
> 
> Alexey, I'm re-adding the GC since removing it seems to break blink layout
> tests.

thanks, lgtm

[commit-bot: I haz the power, 2016-11-17 22:22:28]

Description was changed from

==========
[debugger] simplify fetching scripts for inspector.

The old code path is going to be removed with the debug context api.

R=kozyatinskiy@chromium.org
==========

to

==========
[debugger] simplify fetching scripts for inspector.

The old code path is going to be removed with the debug context api.

R=kozyatinskiy@chromium.org

Committed: https://crrev.com/7cfdd66afabc8416aa8853a40543a89f3a175396
Cr-Commit-Position: refs/heads/master@{#40759}
==========

[commit-bot: I haz the power, 2016-11-17 22:22:29]

Patchset 8 (id:??) landed as
https://crrev.com/7cfdd66afabc8416aa8853a40543a89f3a175396
Cr-Commit-Position: refs/heads/master@{#40759}

[commit-bot: I haz the power, 2016-11-17 22:22:57]

Description was changed from

==========
[debugger] simplify fetching scripts for inspector.

The old code path is going to be removed with the debug context api.

R=kozyatinskiy@chromium.org

Committed: https://crrev.com/7cfdd66afabc8416aa8853a40543a89f3a175396
Cr-Commit-Position: refs/heads/master@{#40759}
==========

to

==========
[debugger] simplify fetching scripts for inspector.

The old code path is going to be removed with the debug context api.

R=kozyatinskiy@chromium.org

Committed: https://crrev.com/581614eeea743dc64739f14a982f396dfda852ba
Cr-Commit-Position: refs/heads/master@{#40768}
==========

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

Patchset 8 (id:??) landed as
https://crrev.com/581614eeea743dc64739f14a982f396dfda852ba
Cr-Commit-Position: refs/heads/master@{#40768}