So, could we simply skip the cache if the debugger is not entered? I would then set a flag on the global object of the debug context after entering the debugger, which guards caching. Leaving the debugger would clear the cache and clear the flag.
Since blink never enters the debugger, the mirror cache is not active. I would then remove the ClearMirrorCache call when getting scripts. WDYT? Yang On Tue, Jun 3, 2014 at 11:29 AM, <yu...@chromium.org> wrote: > 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#newcode491 > >> 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/Source/bindings/v8/DebuggerScript.js > [2] > https://code.google.com/p/chromium/codesearch#chromium/ > src/third_party/WebKit/Source/bindings/v8/PageScriptDebugServer.cpp&q= > PageScriptDebugServer.cpp&sq=package:chromium&type=cs&l=132 > [3] > https://code.google.com/p/chromium/codesearch#chromium/ > src/third_party/WebKit/Source/bindings/v8/DebuggerScript.js& > q=getScripts&sq=package:chromium&type=cs&l=132 > > https://codereview.chromium.org/296953005/ > -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
