Last Comment Bug 480765 - Scripts compiled before the debugger got activated cannot be debugged
: Scripts compiled before the debugger got activated cannot be debugged
Status: RESOLVED FIXED
[fixed1.9.1b4]
: fixed1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: mozilla1.9.2a1
Assigned To: Wladimir Palant
: jsd
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 374833 483685
  Show dependency treegraph
 
Reported: 2009-02-28 17:17 PST by Wladimir Palant
Modified: 2011-07-08 00:24 PDT (History)
19 users (show)
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch [Checkin: Comment 3 & 21] (4.08 KB, patch)
2009-02-28 17:47 PST, Wladimir Palant
timeless: review+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review

Description Wladimir Palant 2009-02-28 17:17:27 PST
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()).
Comment 1 Wladimir Palant 2009-02-28 17:47:41 PST
Created attachment 364730 [details] [diff] [review]
Proposed patch
[Checkin: Comment 3 & 21]

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.
Comment 2 Wladimir Palant 2009-03-01 06:35:19 PST
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 3 Serge Gautherie (:sgautherie) 2009-03-14 08:02:05 PDT
Comment on attachment 364730 [details] [diff] [review]
Proposed patch
[Checkin: Comment 3 & 21]


http://hg.mozilla.org/mozilla-central/rev/49de852e9837
Comment 4 Serge Gautherie (:sgautherie) 2009-03-14 11:38:57 PDT
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 ;->
Comment 5 Robert Kaiser 2009-03-15 08:07:18 PDT
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?
Comment 6 Wladimir Palant 2009-03-16 04:01:29 PDT
(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 7 Serge Gautherie (:sgautherie) 2009-03-16 04:12:50 PDT
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...
Comment 8 Serge Gautherie (:sgautherie) 2009-03-16 04:14:15 PDT
(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.
Comment 9 Robert Kaiser 2009-03-16 06:33:04 PDT
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 10 Henrik Skupin (:whimboo) 2009-03-16 09:29:28 PDT
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.
Comment 11 Henrik Skupin (:whimboo) 2009-03-16 09:30:19 PDT
Wladimir, can we mark it fixed on trunk now?
Comment 12 Serge Gautherie (:sgautherie) 2009-03-16 09:41:24 PDT
(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.
Comment 13 Wladimir Palant 2009-03-16 09:45:55 PDT
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.
Comment 14 :Gijs Kruitbosch 2009-03-16 09:56:14 PDT
(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 15 Samuel Sidler (old account; do not CC) 2009-03-16 15:08:27 PDT
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.
Comment 16 John J. Barton 2009-03-16 22:21:22 PDT
I don't see why this is really needed. Chromebug can get all of the jsdIScripts already using the current (3.0) jsd.
Comment 17 Wladimir Palant 2009-03-16 23:34:20 PDT
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.
Comment 18 John J. Barton 2009-03-17 08:48:14 PDT
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?
Comment 19 Wladimir Palant 2009-03-17 09:55:07 PDT
(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 20 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-07 10:49:38 PDT
Comment on attachment 364730 [details] [diff] [review]
Proposed patch
[Checkin: Comment 3 & 21]

a191=beltzner
Comment 21 Serge Gautherie (:sgautherie) 2009-04-07 20:40:35 PDT
Comment on attachment 364730 [details] [diff] [review]
Proposed patch
[Checkin: Comment 3 & 21]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/530c27c0321a
Comment 22 Serge Gautherie (:sgautherie) 2009-04-07 21:10:39 PDT
(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 23 Daniel Veditz [:dveditz] 2009-04-13 15:20:23 PDT
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?

Note You need to log in before you can comment on or make changes to this bug.