The debugger service keeps a collection of jsdIScript objects for each script that it sees compiled. It uses this collection to "translate" scripts for debugger hooks when its SpiderMonkey hooks are called. The problem is that some scripts might be compiled before the debugger is initialized with the consequence that the debugger service won't know them and won't call the hooks for them. This is mostly a problem when debugging XPCOM components, e.g. when I investigated NoScript with JavaScript deobfuscator extension I could see what NoScript's chrome part was doing but its XPCOM component stayed invisible. A solution of this problem would be creating new jsdIScript objects for unknown scripts independently of which hook is being called. In addition to the jsd_FindJSDScript() parameters, JSContext and JSFunction are required. The former is known to all jsd_FindJSDScript() callers and can be passed in, the latter can be obtained via JS_GetFrameFunction(JS_FrameIterator()).

This patch adds a function jsd_FindOrCreateJSDScript() that will call jsd_FindJSDScript() but fall back to creating a new JSDScript instance if that call returns NULL. Replaced calls to jsd_FindJSDScript() in all but three places: jsd_DestroyScriptHookProc: if we cannot find the script there is no point in doing anything _isActiveHook: this searches for breakpoints and unknown scripts cannot have breakpoints _addNewFrame: this is always called after a script was already found I thought about simply extending jsd_FindJSDScript() but the fallback isn't always necessary (as in the cases above) and having non-obvious side-effects in a function isn't really desirable.

Note that I don't think this will immediately fix Venkman - from what I remember, Venkman only expects to see new scripts in scriptHook calls. But that should be easy to fix.

Comment on attachment 364730 [details] [diff] [review] Proposed patch [Checkin: Comment 3 & 21] http://hg.mozilla.org/mozilla-central/rev/49de852e9837

Is this bug trunk only, or wanted in 1.9.1/1.9.0/1.8.1/1.8.0 too ? (In reply to comment #2) Looking forward for next patches ;->

From what I heard, at least for 1.9.1 this should be interesting as well. Wladimir, can you please request approval for 1.9.1 if you agree?

(In reply to comment #5) > From what I heard, at least for 1.9.1 this should be interesting as well. > Wladimir, can you please request approval for 1.9.1 if you agree? I would love to - but there is no flag for that. So, did somebody forget to configure the flag for this product?

Comment on attachment 364730 [details] [diff] [review] Proposed patch [Checkin: Comment 3 & 21] Asking for (missing flag) "approval1.9.1=?" first; but may want this patch on the other branches too...

(In reply to comment #6) > there is no flag for that. So, did somebody forget to > configure the flag for this product? You're right, this very flag is missing! Could you search/file a bug? Thanks.

Actually, this bug might even be in the wrong component, as JSD is a JavaScript Engine feature, while this component here is IIRC for venkman code.

Comment on attachment 364730 [details] [diff] [review] Proposed patch [Checkin: Comment 3 & 21] As said by previous comments that would result in much better performance when running a debugger like Venkman. Would be great to have it in 1.9.1.

Wladimir, can we mark it fixed on trunk now?

(In reply to comment #10) Iiuc, this by itself won't give any speedup. (In reply to comment #11) See (ToDo) comment 2. After that, the Venkman speed issue could be fixed in one the already filed bugs about that.

Yes, I tested with Minefield 3.2a1pre (build 20090316) and I can debug NoScript with JavaScript Deobfuscator despite jsd not being initialized on startup. And - yes, this bug won't give any speedup by itself. But it is a pre-requisite to get Venkman fixed.

(In reply to comment #9) > Actually, this bug might even be in the wrong component, as JSD is a JavaScript > Engine feature, while this component here is IIRC for venkman code. Huh? No. The JSD component is both for the JSD part of JS, as well as for Venkman. Yes, there ought to be separate components, but somehow that has never happened. Just check for the other bugs filed in the original component, and it'll be obvious that JSD bugs have always been dealt with there (and still are). If you feel strongly about the matter, please file a bug on it and CC me. You're not the first to be confused, so it's probably best we get that over with. :-) (In reply to comment #10) > (From update of attachment 364730 [details] [diff] [review]) > As said by previous comments that would result in much better performance when > running a debugger like Venkman. Would be great to have it in 1.9.1. As the others have indicated, the assertion of "better performance when running a debugger" is wrong. None of the previous comments here mention it. As the summary would imply, the patch fixes a bug in JSD, and because JSD is not used intensively, it is reasonably low-risk. Because the bug is a very serious impediment to a fully-functional debugger or JS analysis tool, it is high-value. The high-value/low-risk tradeoff is why this could hopefully land on branch, assuming we don't run into trouble on trunk. That's why people are asking for approval. The only time performance comes into play is when we are in fact *not* debugging, yet have a debugger installed. And even so, this bug doesn't actually fix that issue, but it allows fixing it with more followup patches in bugs that are yet to be filed (see also comment #2). Please don't just move bugs or attempt to explain the effect of patches if you don't understand the problem and patch. (I would have moved this back but I presume the flag would then be erased, and for now I care more that this gets into 1.9.1 than about where the bug lives)

Comment on attachment 364730 [details] [diff] [review] Proposed patch [Checkin: Comment 3 & 21] Please nominate this for approval on the applicable branches after it lands on 1.9.1.

I don't see why this is really needed. Chromebug can get all of the jsdIScripts already using the current (3.0) jsd.

John, that's because ChromeBug sets jsdIDebuggerService.initAtStartup flag when the debugger is used for the first time (at least that's the way I read the code). After that it imposes the same constant penalty on the performance as Venkman (see http://adblockplus.org/blog/hidden-cost-of-not-using-venkman). And even then some XPCOM components won't be visible to it because they initialize before jsdIDebuggerService. Which ones those are is pretty random and depends on the profile, for me it was NoScript's component for example.

I don't think initAtStartup is needed; I don't know what it does. I re-wrote the initialization of Chromebug for version 0.5 (unfortunately still called 0.4 in svn). It gets the component scripts two ways, 1) by setting jsd hooks very early in start-up using a command line process registered under const clh_category = "b-chromebug"; (see http://code.google.com/p/fbug/source/browse/chromebug/branches/chromebug0.4/components/chromebug_command_line.js) 2) using jsd.enumerateScripts() to look for scripts stored from the time jsd comes on until the function call. (Here is where initAtStartup may help? That is in the absence of the command line processor). If you tell me which file of what extension you want to see I will try it to confirm whether Chromebug 0.5 can see it. The performance penalty bit I don't get. If you want the jsdIScripts from components then you need to run jsd during start up. Or maybe I misunderstand. Does this patch create jsdIScripts for js scripts that are compiled when jsd is not active?

(In reply to comment #18) > I don't think initAtStartup is needed; I don't know what it does. It does exactly what you are doing in Chromebug explicitly now - starts jsdIDebuggerService during XPCOM initialization. > Or maybe I misunderstand. Does this patch create jsdIScripts for js scripts > that are compiled when jsd is not active? Yes. With this patch you won't need to activate jsd early, you can just activate it when you need it. Additional scripts will be created as they are executed, so they will be debuggable even though these scripts were compiled before jsd was turned on.

Comment on attachment 364730 [details] [diff] [review] Proposed patch [Checkin: Comment 3 & 21] a191=beltzner

Comment on attachment 364730 [details] [diff] [review] Proposed patch [Checkin: Comment 3 & 21] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/530c27c0321a

(In reply to comment #14) > If you feel strongly about the matter, please file a bug on it and CC me. Bug 483686 was filed. > it allows fixing it with more followup patches in > bugs that are yet to be filed (see also comment #2). Bug 483685 was filed.

Comment on attachment 364730 [details] [diff] [review] Proposed patch [Checkin: Comment 3 & 21] We're nervous mucking with old debug stuff on the 1.9.0 branch without a pressing need. Why can't developers use a 1.9.1 build with the fix to debug their sites?