On Tue, Jun 3, 2014 at 3:30 PM, Yang Guo <yang...@chromium.org> wrote:
> The thing is, if the debugger is not active, entering the debugger loads a > new debug context every time. > We assume that the same debug context is reused as long as we have debug event listener set in v8 and this matches with what I see in debug.cc [1]. Also we compile DebuggerScript.js once when devtools window opens and keep a persistent handle on a global object from the debug context. [1] https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/debug.cc&q=GetDebugContext&sq=package:chromium&type=cs&l=3134 > You would have to re-disable the mirror cache every time. Is that what you > want? > Since we reuse one debug context during give debug session we will have to set that flag only once when opening devtools. Of cause if the user closes/reopens devtools a new debug context will be created and the flag will be reset. > > Yang > > > On Tue, Jun 3, 2014 at 1:22 PM, Yury Semikhatsky <yu...@chromium.org> > wrote: > >> >> >> >> 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.
