Last Comment Bug 614557 - NS_ERROR_NOT_IMPLEMENTED when opening venkman
: NS_ERROR_NOT_IMPLEMENTED when opening venkman
Status: VERIFIED FIXED
: regression
Product: Other Applications
Classification: Client Software
Component: Venkman JS Debugger (show other bugs)
: Trunk
: All All
-- blocker (vote)
: ---
Assigned To: Karsten Düsterloh
:
:
Mentors:
: 608685 615106 622867 642182 645037 645053 645374 658492 (view as bug list)
Depends on:
Blocks: 595243
  Show dependency treegraph
 
Reported: 2010-11-24 07:08 PST by Philipp Kewisch [:Fallen]
Modified: 2011-05-20 03:34 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix - v1 (5.37 KB, patch)
2010-11-24 07:38 PST, Philipp Kewisch [:Fallen]
no flags Details | Diff | Splinter Review
Fix - v2 (6.10 KB, patch)
2010-11-24 13:50 PST, Philipp Kewisch [:Fallen]
no flags Details | Diff | Splinter Review
Fix - v3 (6.28 KB, patch)
2011-01-24 01:41 PST, Philipp Kewisch [:Fallen]
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review
v4: sync asyncOn (10.34 KB, patch)
2011-02-16 14:12 PST, Karsten Düsterloh
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review
v5: final version (10.29 KB, patch)
2011-02-17 14:17 PST, Karsten Düsterloh
mnyromyr: review+
Details | Diff | Splinter Review

Description User image Philipp Kewisch [:Fallen] 2010-11-24 07:08:55 PST
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.
Comment 1 User image Philipp Kewisch [:Fallen] 2010-11-24 07:12:26 PST
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.
Comment 2 User image timeless 2010-11-24 07:13:07 PST
you need to use .asyncOn(...)

http://mxr.mozilla.org/mozilla-central/ident?i=asyncOn
Comment 3 User image Philipp Kewisch [:Fallen] 2010-11-24 07:38:57 PST
Created attachment 493000 [details] [diff] [review]
Fix - v1

This fixes the initial startup, but still some files are not shown. I guess thats a different issues though.
Comment 4 User image timeless 2010-11-24 07:59:22 PST
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.
Comment 5 User image Philipp Kewisch [:Fallen] 2010-11-24 13:50:22 PST
Created attachment 493111 [details] [diff] [review]
Fix - v2

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.
Comment 6 User image timeless 2010-11-25 00:36:50 PST
*** Bug 608685 has been marked as a duplicate of this bug. ***
Comment 7 User image timeless 2010-11-25 00:39:02 PST
+++ b/resources/content/tests/eval.xul	Wed Nov 24 22:47:02 2010 +0100

arguably this should also support the old way :)
Comment 8 User image Jens Hatlak (:InvisibleSmiley) 2010-11-29 14:12:13 PST
*** Bug 615106 has been marked as a duplicate of this bug. ***
Comment 9 User image Alex Vincent [:WeirdAl] 2011-01-04 10:32:15 PST
*** Bug 622867 has been marked as a duplicate of this bug. ***
Comment 10 User image Jens Hatlak (:InvisibleSmiley) 2011-01-22 10:41:29 PST
(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.
Comment 11 User image Philipp Kewisch [:Fallen] 2011-01-24 01:41:45 PST
Created attachment 506340 [details] [diff] [review]
Fix - v3

Ah yes, sorry. Totally forgot about this bug. Thanks for the note!
Comment 12 User image :Gijs 2011-01-24 02:28:19 PST
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.
Comment 13 User image Karsten Düsterloh 2011-02-16 14:12:01 PST
Created attachment 512915 [details] [diff] [review]
v4: sync asyncOn

(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.
Comment 14 User image neil@parkwaycc.co.uk 2011-02-16 16:23:33 PST
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 15 User image :Gijs 2011-02-16 23:10:56 PST
Comment on attachment 512915 [details] [diff] [review]
v4: sync asyncOn

With the nits noted by Neil, r=me.
Comment 16 User image Karsten Düsterloh 2011-02-17 14:17:11 PST
Created attachment 513265 [details] [diff] [review]
v5: final version

This got pushed as <http://hg.mozilla.org/venkman/rev/65c7be10defd>, with Neil's nits fixed (I dropped the changeset part completely).
Comment 17 User image Philipp Kewisch [:Fallen] 2011-02-19 03:17:01 PST
Karsten, thanks for taking care!
Comment 18 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2011-02-19 04:23:13 PST
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.
Comment 19 User image alta88 2011-02-20 08:10:43 PST
could someone please update and release the extension on AMO..
Comment 20 User image Karsten Düsterloh 2011-02-20 13:00:15 PST
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.)
Comment 21 User image Karsten Düsterloh 2011-02-20 13:01:00 PST
s/this patch/Venkman with this patch/
Comment 22 User image Jens Hatlak (:InvisibleSmiley) 2011-03-16 11:41:29 PDT
*** Bug 642182 has been marked as a duplicate of this bug. ***
Comment 23 User image Jens Hatlak (:InvisibleSmiley) 2011-03-25 10:05:48 PDT
*** Bug 645037 has been marked as a duplicate of this bug. ***
Comment 24 User image Jens Hatlak (:InvisibleSmiley) 2011-03-26 17:42:45 PDT
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).
Comment 25 User image James Ross 2011-03-26 17:58:50 PDT
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.
Comment 26 User image Philip Chee 2011-03-26 18:01:44 PDT
*** Bug 645053 has been marked as a duplicate of this bug. ***
Comment 27 User image Philip Chee 2011-03-26 18:02:00 PDT
*** Bug 645374 has been marked as a duplicate of this bug. ***
Comment 28 User image James Ross 2011-05-20 03:34:22 PDT
*** Bug 658492 has been marked as a duplicate of this bug. ***

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