Closed Bug 614557 Opened 14 years ago Closed 13 years ago

NS_ERROR_NOT_IMPLEMENTED when opening venkman

Categories

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

defect
Not set
blocker

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Fallen, Assigned: mnyromyr)

References

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

Since bug 595243 landed, venkman is not usable anymore. While I understand that the focus is on firefox and therefore firebug/chromebug, this does not work for other Mozilla Apps.

Please at least fix up venkman so it works again.
More info, sorry:

When opening venkman, a dialog shows up that it could not start the debugger service, since jsdIDebuggerSerive::On() now throws NS_ERROR_NOT_IMPLEMENTED. It seems now that instead a debugMode needs to be enabled, see bug 595243.

After clicking ok, the debugger window opens, but everything is empty except the toolbar and menu.
you need to use .asyncOn(...)

http://mxr.mozilla.org/mozilla-central/ident?i=asyncOn
Attached patch Fix - v1 (obsolete) — Splinter Review
This fixes the initial startup, but still some files are not shown. I guess thats a different issues though.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #493000 - Flags: review?
Attachment #493000 - Flags: review? → review?(gijskruitbosch+bugs)
this isn't backwards compat.

What you should do is something like:

if ("asyncOn" in jsd) {
  jsd.asynOn(this);
} else {
  jsd.on();
  this.onDebuggerActivated();
}

and stick the rest of your code into this.onDebuggerActivated.
Attached patch Fix - v2 (obsolete) — Splinter Review
You're right, I guess I should keep it backwards compatible. This patch takes care and also aligns the style to whats in the files already. Note there is no explicit this object there, but I put the code into an extra function.
Attachment #493000 - Attachment is obsolete: true
Attachment #493111 - Flags: review?(gijskruitbosch+bugs)
Attachment #493000 - Flags: review?(gijskruitbosch+bugs)
+++ b/resources/content/tests/eval.xul	Wed Nov 24 22:47:02 2010 +0100

arguably this should also support the old way :)
Blocks: 595243
No longer depends on: 595243
(In reply to comment #7)
> +++ b/resources/content/tests/eval.xul    Wed Nov 24 22:47:02 2010 +0100
> 
> arguably this should also support the old way :)

Philipp, may I suggest fixing the test and requesting review from timeless? Gijs seems to be busy.
Attached patch Fix - v3 (obsolete) — Splinter Review
Ah yes, sorry. Totally forgot about this bug. Thanks for the note!
Attachment #493111 - Attachment is obsolete: true
Attachment #506340 - Flags: review?
Attachment #493111 - Flags: review?(gijskruitbosch+bugs)
Attachment #506340 - Flags: review? → review?(timeless)
Comment on attachment 506340 [details] [diff] [review]
Fix - v3

(In reply to comment #11)
> Created attachment 506340 [details] [diff] [review]
> Fix - v3
> 
> Ah yes, sorry. Totally forgot about this bug. Thanks for the note!

Two points. First, AFAICT there are two instances where you changed to an async pattern (not counting the test) and one has actual code in the callback listener (the dump statement is less important...).

What if the first bit of code turns on JSD (on startup), will the callback still fire once you call asyncOn a second time (from venkman-debugger.js) ? The code in that callback does need to be executed. Please test/verify this, and if not, make sure the code runs if jsds.isOn (it may be better to just run the code directly if jsds.isOn(), and only run .on()/.asyncOn() otherwise).


Second, initDebugger is expected to be synchronous. If you check the caller, there is some (but not all!) code there (in venkman-static.js) that expects the debugger is initialized (from the looks of it, at least the (delayed) initPost, the venkman-started hook, etc.). It would be best to use a callback structure (pass a function to initDebugger, have onDebuggerActivated call it) to make sure this code doesn't fire until the debugger is actually active.


With the callback and ensuring the hooks get added even if JSDS is turned on at startup, r=me.
Attachment #506340 - Flags: review?(timeless) → review+
Attached patch v4: sync asyncOn (obsolete) — Splinter Review
(Stealing this with Fallen's permission.)

(In reply to comment #12)
> What if the first bit of code turns on JSD (on startup), will the callback
> still fire once you call asyncOn a second time (from venkman-debugger.js)?

No, it won't. 
Fixed.

> Second, initDebugger is expected to be synchronous. 
...
> It would be best to use a callback structure

Fixed.

Venkman still dumps some JS errors on startup, but those don't seem to be related with this bug.
Assignee: philipp → mnyromyr
Attachment #506340 - Attachment is obsolete: true
Attachment #512915 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 512915 [details] [diff] [review]
v4: sync asyncOn

Drive-by review:

>+        // (initAtStartup was removed by changeset 46998/bug 568691/2010-06-10)
Changeset numbers are local and unportable.
Could try linking to a rev on hg.mozilla.org though.

>+                jsds.asyncOn({ onDebuggerActivated: onDebuggerActivated });
jsdIActivationCallback has recently been made a [function], so you can just pass onDebuggerActivated directly.

>+          jsds.asyncOn({ onDebuggerActivated: function() {} });
You can also pass null for no callback. (Maybe it should be optional?)

>-    console.executionHook = { onExecute: jsdExecutionHook };
>+        console.executionHook = { onExecute: jsdExecutionHook };
[Is a -w diff possible at all?]
Comment on attachment 512915 [details] [diff] [review]
v4: sync asyncOn

With the nits noted by Neil, r=me.
Attachment #512915 - Flags: review?(gijskruitbosch+bugs) → review+
This got pushed as <http://hg.mozilla.org/venkman/rev/65c7be10defd>, with Neil's nits fixed (I dropped the changeset part completely).
Attachment #512915 - Attachment is obsolete: true
Attachment #513265 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Karsten, thanks for taking care!
Checked out latest source, built XPI, and tested in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110217 Firefox/4.0b12pre ID:20110217123839. Works like a charm. Thanks Karsten for fixing that! Marking as verified fixed.
Status: RESOLVED → VERIFIED
could someone please update and release the extension on AMO..
Note that this patch does only work with Firefox trunk builds, but *not* with FF4 beta builds up to beta 11! 
(The function parameter change was only recently 2011-02-14 in bug 633833, well after beta 11 was released.)
s/this patch/Venkman with this patch/
So, the dupe count increases. Someone with appropriate rights should update the maxVersion on AMO as a minimum, or better still release a fixed version with maxVersion bumps to 4.0.* (Firefox) and 2.1.* (SeaMonkey).
I've set the max version for JavaScript Debugger 0.9.88.1 to Firefox 4.0b6 (i.e. beta 6) which I think is the last one that worked; if not, just say and I can adjust it, but it at least isn't claiming 4.0 final compatibility now.
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.