On Tue, Jun 3, 2014 at 1:37 PM, Yang Guo <yang...@chromium.org> wrote:
> 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. > We don't need to tie this to the debugger entrance, we just need a global flag for disabling mirror cache. We would set that flag in DebuggerScript.js initialization code. > Since blink never enters the debugger, the mirror cache is not active. I > would then remove the ClearMirrorCache call when getting scripts. > Blink does enter the debugger in some cases, e.g. when doing evals on pause but we still don't need the cache in those cases. > > > 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.
