418426 - JavaScript Debugger (Venkman) ignores breakpoints in Firefox 3 [NS_NOINTERFACE [jsdIContext.scriptsEnabled] in venkman-debugger.js :: jsdExecutionHook :: line 359]

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[carglue a t ya hoo do tcom]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: 

Beginning with Firefox3 beta 3, Venkman fails to stop at any breakpoints.  Seems like something got seriously hosed in the latest FF beta build.

Another recent issue I have noticed, beginning with FF3beta2, is that I am unable to change the value of variables.  The menu option appears to be disabled and double-clicking on a variable (which normal prompts to change the value) does nothing.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[carglue a t ya hoo do tcom]

Clarification:  Breakpoint issue only occurs if set on a callback method that was invoked from an instance of an nsITimer object (via the nsITimerCallback interface) from within a javascript XPCOM component.

Comment 2

[Matthew Mastracci]

I can confirm this on FF3 RC1 as well.  Breakpoints are hit in normal code, but never through nsITimer callbacks.  I haven't tested to see if it's specifically because the timers are set from an XPCOM component. 

Comment 3

[:Gijs (he/him)]

So I missed this bug, apparently. Did it work in FF3B2? Do you have an easy testcase?

Comment 4

[Matthew Mastracci]

I'll try to reduce it to a simple testcase that can be installed as an extension.

Comment 5

[Matthew Mastracci]

Attachment: XPI that demonstrates the issue

PI that reproduces the bug. 

Steps:

1.  Install XPI
2.  Restart FF
3.  Restart FF again (let the component register)
4.  Set a breakpoint in the timer callback

The timer callback breakpoint is never hit, but breakpoints around it are.

Comment 6

[Matthew Mastracci]

I should add an additional note to clarify what happens:

The XPI contains an overlay that creates an instance of the object for each browser window.  You can trigger the timer to run again by opening a new window.  The breakpoints around the timer callback are hit once per window open, but the callback breakpoint is never hit.

Comment 7

[carglue a t ya hoo do tcom]

Verified that this is still an issue with the new FF 3 release.

We are unable to use any of the latest versions of Venkman with FF 3 because our addon uses heavy XPCOM with calls with nsITimerCallbacks throughout.  We have had to revert to debugging our addon using FF 2 with an older copy of venkman (0.9.87, because the .1, .2, .3, and .4 releases are all broken, including modifying interactively modifying variables).

Comment 8

[Caio Tiago Oliveira (asrail)]

Breakpoints are broken for chrome code.

Viewing source is broken for them, I'm trying to track down the issue.

Breakpoints are fine for content code.

I could verify this on gecko 1.9.1.

Comment 9

[Colin Blake]

I am seeing something similar for our product, which is based on the Mozilla 1.9 code base from around the time when Firefox 3 was released. Some (chrome) modules I can set a breakpoint in and the breakpoint works as expected, but in other modules the breakpoints are just ignored. I haven't figured out the logic behind which work which don't work yet.

Comment 10

[:Gijs (he/him)]

OK, this bug is a bit messy now. I was on holiday for all of July and August, hence the late comment. Trying to make sense here:

1) (ref. comment #8) are all breakpoints in chrome code not working? Or only ones from nsITimer stuff?

2) Did someone do a regression analysis? When did what break?

3) What exactly is "all broken" in comment #7 supposed to mean? And on any release, or on a specific Firefox release?

Comment 12

[Bruno Calvignac]

I did some investigation this morning and managed to get simple reproducable steps. Hope this helps.
1) Start Firefox with a debug console.
2) Start Venkman
3) Add a breakpoint in nsUpdateService.js in the notify function (line 2815)

-> the breakpoint never hits but in the console, you get error messages:

vnk: execution hook: function anonymous(timer=XPComponent:{10}) in <file:/C:/build/flock-trunk/objectdir/dist/bin/components/nsUpdateService.js> line 2815
** ASSERTION FAILED: no cx in execution hook **
<top>
anonymous@60
jsdExecutionHook@303
anonymous@2815
vnk: execution hook: function anonymous(timer=XPComponent:{10}) in <file:/C:/build/flock-trunk/objectdir/dist/bin/components/nsUpdateService.js> line 2815
** ASSERTION FAILED: no cx in execution hook **
<top>
anonymous@60
jsdExecutionHook@303
anonymous@2815

Comment 13

[Bruno Calvignac]

The problem does not seem limited to timers as I've seen it when trying to set breakpoints on callbacks from nsIXMLHttpRequest.

Comment 14

[Colin Blake]

I wish I knew where to look to try and track down this one. Not being able to
set breakpoints in our code is killing us!

Comment 15

[Lo]

Is anyone currently working on this bug (or planning to at any point in the future)?

Comment 16

[Lo]

(In reply to comment #0)
> Another recent issue I have noticed, beginning with FF3beta2, is that I am
> unable to change the value of variables.  The menu option appears to be
> disabled and double-clicking on a variable (which normal prompts to change the
> value) does nothing.

I opened Bug 503975 for this other issue.

Comment 17

[Lo]

I tracked down the source of this bug to the patch for bug 352791. 

Problem seems to be that GetContextForObject() does not return the correct
JSContext. Venkman's breakpoint handler is called but since it doesn't get the
correct execution context object to deal with the breakpoint, it just ignores
it.

Several other bugs also appeared as side-effects of that patch (bug 412598, bug
413200, bug 414658), but were quickly resolved soon after. We're gonna need
expert help with this one too.

Comment 18

[Lo]

Attachment: Workaround XPI

I've found a temporary workaround:

Comment out lines 289-316 and 332-338 in venkman-debugger.js. So from the beginning of the 1st try-catch up to just before the 2nd try-catch and then another 7 lines from just after the 2nd try-catch up till just before the line "delete console.frames;" in function jsdExecutionHook (frame, type, rv).

I attached an xpi that includes this workaround.

Comment 19

[:Gijs (he/him)]

(In reply to comment #17)
> I tracked down the source of this bug to the patch for bug 352791. 
> 
> Problem seems to be that GetContextForObject() does not return the correct
> JSContext. Venkman's breakpoint handler is called but since it doesn't get the
> correct execution context object to deal with the breakpoint, it just ignores
> it.
> 
> Several other bugs also appeared as side-effects of that patch (bug 412598, bug
> 413200, bug 414658), but were quickly resolved soon after. We're gonna need
> expert help with this one too.

Blake, could you take a look at this? I don't think commenting out the lines mentioned in comment #18 is a good fix . As far as I know, we *should* be checking contexts and windows, or we'll not be looking for the right local variables etc...

Comment 20

[Lo]

(In reply to comment #19)
> Blake, could you take a look at this? I don't think commenting out the lines
> mentioned in comment #18 is a good fix . As far as I know, we *should* be
> checking contexts and windows, or we'll not be looking for the right local
> variables etc...

I agree.

I should have elaborated more on what the workaround does:

It makes Venkman skip a bunch of checks that are normally done before handling a breakpoint. Since this bug was causing these checks to fail for certain breakpoints (such as those in nsITimer callbacks), skipping the checks allows Venkman to try to handle the breakpoint anyway.

Of course, skipping the checks means that, while the debugger will now stop at the breakpoint, the information it gets about the executing script could very well be incorrect, so I wouldn't be surprised if the local variables etc are wrong in some way or the other. Your Mileage May Vary.

All I can say is that the workaround is definitely NOT A FIX, but hopefully will be at least somewhat useful in the meantime for anyone who hasn't been able to use the debugger with FF 3.0+ due to this bug.

Comment 21

[Lo]

Attachment: Workaround XPI

corrected the XPI structure, so it uses the updated file now.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #19)
> Blake, could you take a look at this? I don't think commenting out the lines
> mentioned in comment #18 is a good fix . As far as I know, we *should* be
> checking contexts and windows, or we'll not be looking for the right local
> variables etc...

It's hard for me to really comment on the internals of jsd. In the JS engine, the context doesn't matter for local variables and stuff like that. The information to look up a local variable is entirely contained in the JSStackFrame (possibly using the function or other debugger APIs). The context represents more of a thread of execution. I don't know if the same thing holds true for JSDContexts as well.

There is definitely a bug in jsdExecutionHook, though: it assumes that context.globalObject is always a window. In some cases (notably JS components) this is not the case. So the code that Lo removed, instead of being removed, should just check for a null return value from getBaseWindowFromWindow and deal with it.

The reason this appears to be a regression is because before the patch for bug 352791 the callback for timers would run on the "safe" JSContext. That context has a ChromeWindow as its globalObject, so the code ran fine. Now, we're running the timeout on the component's context, which has a BackstagePass (which has no relation to windows) as its globalObject and failure happens.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix?

I don't know where Venkman's source lives these days. I generated this patch (but haven't even tested it) against "1.9.0" CVS HEAD.

Comment 24

[Alex Vincent [:WeirdAl]]

mrkbap: http://hg.mozilla.org/venkman/

Comment 25

[:Gijs (he/him)]

Ah, and suddenly there was enlightenment. :-)

This is a dupe, and it has been fixed in HG, AFAICT.

Comment 26

[:Gijs (he/him)]

(In reply to bug 494060 comment #8)
> Bug 418426 is still reproducible with this patch. The call to cx.scriptsEnabled
> = false throws an exception:
> 
> [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERF
> ACE) [jsdIContext.scriptsEnabled]"  nsresult: "0x80004002 (NS_NOINTERFACE)" 
> loc
> ation: "JS frame :: chrome://venkman/content/venkman-debugger.js ::
> jsdExecution
> Hook :: line 323"  data: no]

OK. So now I am back to being (at least slightly) confused. Can we somehow not set cx.scriptsEnabled on non-window contexts?

Comment 27

[Lo]

Also though fixing bug 494060 will help fix this problem for FF 3.5, it will still continue to fail in FF 3.0. 

In FF 3.0, the failure occurs at a different point, an earlier line in
jsdExecutionHook():

290:        cx = frame.executionContext;

which returns null which makes the assertion at line 300 fail (as seen in comment #12):

300:    if (!ASSERT(cx, "no cx in execution hook"))
301:        return hookReturn;

Comment 28

[:Gijs (he/him)]

(In reply to comment #27)
> Also though fixing bug 494060 will help fix this problem for FF 3.5, it will
> still continue to fail in FF 3.0. 
> 
> In FF 3.0, the failure occurs at a different point, an earlier line in
> jsdExecutionHook():
> 
> 290:        cx = frame.executionContext;
> 
> which returns null which makes the assertion at line 300 fail (as seen in
> comment #12):
> 
> 300:    if (!ASSERT(cx, "no cx in execution hook"))
> 301:        return hookReturn;

I'm pretty sure that if we don't have a cx at all, we'll be pretty helpless no matter what else we do.

Comment 29

[Lo]

It certainly seems like that would be the case, but it isn't. As you can see from the workaround I gave in comment #18 above, commenting out this entire section of code doesn't prevent the debugger from hitting all breakpoints as expected and still evaluating local variables to their correct values (at least in my limited testing).

In fact, this cx object doesn't appear to ever be used again in the code beyond this function.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

I tried looking into this the other day, but I keep running into assertions due to leaks on shutdown and random corruption afterward. It might be possible to bisect to figure out which changeset fixed this bug on the 3.5 branch and then backport it to 1.9.0 (the 3.0 branch). I don't know about scriptsEnabled.

Comment 31

[Lo]

(In reply to comment #26)
> (In reply to bug 494060 comment #8)
> > Bug 418426 is still reproducible with this patch. The call to cx.scriptsEnabled
> > = false throws an exception:
> > 
> > [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERF
> > ACE) [jsdIContext.scriptsEnabled]"  nsresult: "0x80004002 (NS_NOINTERFACE)" 
> > loc
> > ation: "JS frame :: chrome://venkman/content/venkman-debugger.js ::
> > jsdExecution
> > Hook :: line 323"  data: no]
> 
> OK. So now I am back to being (at least slightly) confused. Can we somehow not
> set cx.scriptsEnabled on non-window contexts?

found this in /js/jsd/idl/jsdIDebuggerService.idl (1.9.1 branch):

706:    /**
707:     * Context object.  Only context's which are also nsISupports objects can be
708:     * reflected by this interface.
709:     */
710:    [scriptable, uuid(a2dd25a4-1dd1-11b2-bda6-ed525acd4c35)]
711:    interface jsdIContext : jsdIEphemeral
712:    {
.
.
.
770:        /**
771:         * |true| if this context should be allowed to run scripts, |false|
772:         * otherwise.  This attribute is only valid for contexts which implement
773:         * nsIScriptContext.  Setting or getting this attribute on any other
774:         * context will throw a NS_ERROR_NO_INTERFACE exception.
775:         */
776:        attribute boolean                scriptsEnabled;
777:    };

Comment 32

[Lo]

(In reply to comment #30)
> I tried looking into this the other day, but I keep running into assertions due
> to leaks on shutdown and random corruption afterward. It might be possible to
> bisect to figure out which changeset fixed this bug on the 3.5 branch and then
> backport it to 1.9.0 (the 3.0 branch). I don't know about scriptsEnabled.

Here is the changeset that fixed the 3.5 branch:
http://hg.mozilla.org/mozilla-central/rev/60846c749814

Looks to me like it's all related to the context no longer being an nsISupports...

Comment 33

[James Ross]

What is this bug actually about now? I can see two different issues:
 - Firefox 3.0 only dies on if (!ASSERT(cx, "no cx in execution hook")).
 - Firefox 3.0+ have jsdIContext.scriptsEnabled issues with certain callbacks.

Bug 459996 also reports the first issue. I fixed the jsdIContext.scriptsEnabled issue locally using basically Blake's patch in this bug, i.e. moving cx.scriptsEnabled assignments inside if (console.targetWindow).

Comment 34

[Igor Velkov]

Attachment: fix for venkman 0.9.87.4 by silver. Obtained in IRC.

Make breakpoints work again

Comment 35

[Karsten Düsterloh]

Comment on attachment 432064 [details] [diff] [review]
fix for venkman 0.9.87.4 by silver. Obtained in IRC.

That's not a fix, that's just wallpapering. :-|

Comment 36

[James Ross]

(In reply to comment #35)
> That's not a fix, that's just wallpapering. :-|

Indeed. It's a hack I handed out on IRC so someone could actually use Venkman for what they wanted. I'd suggest we go with the patch I mentioned in comment 33 but I don't remember what it fixed exactly, whether it's the right approach and I currently have no sensible way of testing Venkman code anyway. :(

Comment 37

[Stefan Fleiter (:sfleiter)]

I had this problem today with venkman 0.9.88.2 and Thunderbird 11.0a2/20120102.
The workaround from comment 34 could be ported to current venkman and helped
to work with the debugger keyword again.
Thanks a lot for finding that out!

It's a shame, though there is no real fix and no released version of venkman for extension authors...

Comment 38

[Jens Hatlak (:InvisibleSmiley)]

Attachment: updated XPI that demonstrates the issue

Test case, updated for the "new" Gecko 2 JS components requirements

Comment 39

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 432064 [details] [diff] [review]
fix for venkman 0.9.87.4 by silver. Obtained in IRC.

Removing obsolete review request (it's not an acceptable fix anyway, see comment 36).

Comment 40

[Jens Hatlak (:InvisibleSmiley)]

Attachment: adapted version of "Fix?" [Checkin: comment 42]

(In reply to James Ross from comment #36)
> I'd suggest we go with the patch I mentioned
> in comment 33 but I don't remember what it fixed exactly, whether it's the
> right approach

For the attached patch I took this idea, consolidating the checks for consistency. With that and the updated test case, Venkman stops at testcase-component.js line 12. Without the patch, I get the error message.

I cannot answer the questions I quoted, though. As I said, I just took the existing patch approach and improved it.

Not taking until I get either f+ or r+. Feel free to take if you have something better. :-)

Comment 41

[:Gijs (he/him)]

Comment on attachment 585407 [details] [diff] [review]
adapted version of "Fix?" [Checkin: comment 42]

Review of attachment 585407 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sensible enough to me (but it's been a while since I did a Vnk review!). AFAICT the whole thing is dying because it's being run on JS global objects which aren't windows (callback envs, worker threads, js modules, yada yada), and so there's no cx.scriptsEnabled property on them. The patch seems to address exactly that, so, r=me.

Comment 42

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 585407 [details] [diff] [review]
adapted version of "Fix?" [Checkin: comment 42]

http://hg.mozilla.org/venkman/rev/148fe7bca62e
(moved up console.targetWindow.enabled = false; one line for consistency)

Comment 43

[Stefan Fleiter (:sfleiter)]

Thanks a lot Jens and Gijs for your work, this fix helps a lot
and I could make some people happy by telling them Venkman from trunk
is stopping on breakpoints again. :-)

Many extension developers avoid using using trunk/nightly/daily
versions, so would this bug and bug 715719 fixed make a new Venkman release
here
https://addons.mozilla.org/en-US/firefox/addon/javascript-debugger/
be worth it?

If so who could do that? Thanks!

Comment 44

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Stefan Fleiter (:sfleiter) from comment #43)
> If so who could do that?

Good question, I have no idea. Technically it could be as easy as updating the changed files in the XPI, bumping the version number and releasing it to AMO, but I don't know the details of the Venkman release process, who is involved in that, did it in the past, or would be allowed to do it. There could also be mandatory extra steps, like performing certain tests.