Fix leak in debug mirror cache.

Fix leak in debug mirror cache.

When fetching loaded scripts, mirror objects are created and cached.
If the cache is not cleared, it holds script objects alive.

This also fixes a minor issue with script unloading.

R=ulan@chromium.org
BUG=376534
LOG=N

Committed: https://code.google.com/p/v8/source/detail?r=21477

[ulan, 2014-05-23 14:53:09]

lgtm

[Yang, 2014-05-26 07:06:06]

Committed patchset #3 manually as r21477 (presubmit successful).

[yurys, 2014-06-03 08:07:21]

https://codereview.chromium.org/296953005/diff/40001/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/296953005/diff/40001/src/debug-debugger.js#ne...
src/debug-debugger.js:491: return %DebugGetLoadedScripts();
This change looks wrong to me. By design of V8 debugging protocol mirror cache
should be kept while we are staying on a breakpoint so that the client could
access corresponding object by its mirror id. After this change, however, the
cache will be cleared on any call to GetLoadedScripts() and the remote id will
become invalid.

In case of blink the problem is that we create mirror objects not only when we
stay on a breakpoint and I'm not sure we clear them properly. Also we don't use
id->mirror map in blink and I believe a right way to fix this would be to
disable mirror cache in blink completely. WDYT?

[Yang, 2014-06-03 08:12:46]

https://codereview.chromium.org/296953005/diff/40001/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/296953005/diff/40001/src/debug-debugger.js#ne...
src/debug-debugger.js:491: return %DebugGetLoadedScripts();
On 2014/06/03 08:07:21, yurys wrote:
> This change looks wrong to me. By design of V8 debugging protocol mirror cache
> should be kept while we are staying on a breakpoint so that the client could
> access corresponding object by its mirror id. After this change, however, the
> cache will be cleared on any call to GetLoadedScripts() and the remote id will
> become invalid.
> 
> In case of blink the problem is that we create mirror objects not only when we
> stay on a breakpoint and I'm not sure we clear them properly. Also we don't
use
> id->mirror map in blink and I believe a right way to fix this would be to
> disable mirror cache in blink completely. WDYT?

Not sure I understand. Which code path does blink use to get loaded scripts?

[yurys, 2014-06-03 09:29:00]

On 2014/06/03 08:12:46, Yang wrote:
> https://codereview.chromium.org/296953005/diff/40001/src/debug-debugger.js
> File src/debug-debugger.js (right):
> 
>
https://codereview.chromium.org/296953005/diff/40001/src/debug-debugger.js#ne...
> src/debug-debugger.js:491: return %DebugGetLoadedScripts();
> On 2014/06/03 08:07:21, yurys wrote:
> > This change looks wrong to me. By design of V8 debugging protocol mirror
cache
> > should be kept while we are staying on a breakpoint so that the client could
> > access corresponding object by its mirror id. After this change, however,
the
> > cache will be cleared on any call to GetLoadedScripts() and the remote id
will
> > become invalid.
> > 
> > In case of blink the problem is that we create mirror objects not only when
we
> > stay on a breakpoint and I'm not sure we clear them properly. Also we don't
> use
> > id->mirror map in blink and I believe a right way to fix this would be to
> > disable mirror cache in blink completely. WDYT?
> 
> Not sure I understand. Which code path does blink use to get loaded scripts?

Blink adds a bunch of convenience methods into the debug context all of which
are defined in DebuggerScript.js [1] and they leverage mirror infrastructure to
inspected user objects (one important difference compared to v8 debugger as I
said is that we never need to resolve id into morror object so the cache is
useless in case of Blink). To collect current scripts PageScriptDebugServer.cpp
calls [2] DebuggerScript.getScripts() which in turn calls Debug.scripts() [3]
but all of this happens without entering debugger as simple function call using
v8 api. As we don't enter debugger in that case ClearMirrorCache may never be
called. This might be a bug in this particular case and we should use
v8::Debug::Call for invoking DebuggerScript.getScripts() to make sure
EnterDebugger is called and mirror cache is cleared after the call. But in
general to avoid bugs like this we could skip mirror cache for the mirrors
created by Blink.

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