452288 - Allow ignoring NS_ERROR_NO_INTERFACE errors

Status: RESOLVED INCOMPLETE

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

Description

[Alex Vincent [:WeirdAl]]

Components are asked very frequently about interfaces they don't support, usually resulting in "throw NS_ERROR_NO_INTERFACE".  These are static to Venkman users, most of the time.

Comment 1

[Alex Vincent [:WeirdAl]]

As a starting point, this is the code for debugTrap, under case jsdIExecutionHook.TYPE_THROW:

if (rv.value.isNumber &&
    rv.value.jsType == Components.interfaces.jsdIValue.TYPE_DOUBLE &&
    rv.value.doubleValue == Components.results.NS_ERROR_NO_INTERFACE) {
  return jsdIExecutionHook.RETURN_CONTINUE_THROW;
}

Comment 2

[Serge Gautherie (:sgautherie)]

See also bug 243621
https://bugzilla.mozilla.org/attachment.cgi?id=148758&action=edit

Comment 3

[:Gijs (he/him)]

(In reply to comment #1)
> As a starting point, this is the code for debugTrap, under case
> jsdIExecutionHook.TYPE_THROW:
> 
> if (rv.value.isNumber &&
>     rv.value.jsType == Components.interfaces.jsdIValue.TYPE_DOUBLE &&
>     rv.value.doubleValue == Components.results.NS_ERROR_NO_INTERFACE) {
>   return jsdIExecutionHook.RETURN_CONTINUE_THROW;
> }

I will happily r+ a patch for this, with a pref (in case someone is writing xpcom components and wonders wtf is going on if their interface doesn't work because, say, they forgot to say it implements nsISupports (happens particularly often for JS components, at least I've seen bemused folks in newsgroups and IRC who ran into that)). Should be trivial to adapt the original from bug 243621 taking comment #1 into consideration.

Comment 4

[Alex Vincent [:WeirdAl]]

Attachment: patch from comment 1, tested locally and fixed

Here you go, Gijs.

Comment 5

[Alex Vincent [:WeirdAl]]

Comment on attachment 474764 [details] [diff] [review]
patch from comment 1, tested locally and fixed

oops, botched it

Comment 6

[Alex Vincent [:WeirdAl]]

Attachment: patch from comment 1, tested locally and fixed, take 2

Fixed the botch.  If we tried to catch nsIException objects, it wouldn't work... this attempt takes care of that.

Comment 7

[:Gijs (he/him)]

(In reply to comment #6)
> Created attachment 474785 [details] [diff] [review]
> patch from comment 1, tested locally and fixed, take 2
> 
> Fixed the botch.  If we tried to catch nsIException objects, it wouldn't
> work... this attempt takes care of that.

Cool, looks pretty good (though I'd like to test it, and possibly for it to be more modular...). Anyway, do you know who we can ask about that getWrappedValue + instanceof for the exception? It might lead to a security issue. Is mrbkap still around?

Comment 8

[Alex Vincent [:WeirdAl]]

(In reply to comment #7)
> Anyway, do you know who we can ask about that getWrappedValue
> + instanceof for the exception? It might lead to a security issue. Is mrbkap
> still around?

No, I don't know.  I'm not qualified to evaluate security issues.  I also don't know about mrbkap's availability.

I'm considering filing a bug for ignoring "throw StopIteration" as well.  In recent development, that proved to be pretty annoying...

Would you want this exposed through the user-interface?  In practice, I've found going to about:config is more steps than I want to jump through.

Comment 9

[Alex Vincent [:WeirdAl]]

From timeless:

>the basic question is what sort of thing is value
>WeirdAl: no, i meant ... what type was value, i.e. i wanted to know that it's "jsdIValue"
>WeirdAl: offhand, i think it might be better to add another QI for jsdValue

Comment 10

[Alex Vincent [:WeirdAl]]

I'll fix this patch in a few moments to explicitly check rv.value, but in the meantime, let me point this out:

debugTrap is called from jsdExecutionHook (http://hg.mozilla.org/venkman/file/19048ef465ad/resources/content/venkman-debugger.js#l327 ).  This appears to be called through console.executionHook.onExecute (line 135 of venkman-debugger.js).  The onExecute method is called from http://hg.mozilla.org/mozilla-central/annotate/812710794ca1/js/jsd/jsd_xpc.cpp#l685, in jsds_ExecutionHookProc.  This last method guarantees rv is a jsdIValue (or null).

Comment 11

[Alex Vincent [:WeirdAl]]

:(  I need some help figuring out how to modify the Venkman user-interface and command structure.  (The code in venkman-commands and venkman-views begs for JavaDoc.)

Comment 12

[Karsten Düsterloh]

Basically, venkman-menus.js has code like 
    console.menuSpecs["mainmenu:debug"] = {
        label: MSG_MNU_DEBUG,
        accesskey: getAccessKeyForMenu('MSG_MNU_DEBUG'),
        items:
        [
         …
         ["-"],
         [">popup:emode"],
         [">popup:tmode"],
         …
for every menu entry, with items containing definitions or redirections: ">popup:emode" means there is also console.menuSpecs["popup:emode"], etc.

The first element of each such item is the name of an entry in the command map in venkman-commands.js's initCommands() method.

venkman-menus.js:
         ["em-ignore",
                 {type: "radio", name: "em",
                  checkedif: "console.errorMode == EMODE_IGNORE"}],
venkman-commands.js:
         ["em-ignore",                "emode ignore",                        0],
being just a redirect to
         ["emode",          cmdEMode,                              CMD_CONSOLE],
which defines the function cmdEMode as the actual handler (which in turn will get passed the second string "ignore" as its parameter).


That said, on IRC you proposed this:
[2011-02-19 00:20:44] <WeirdAl> UI
[2011-02-19 00:22:18] <WeirdAl> Debug 
[2011-02-19 00:22:26] <WeirdAl>   > Exceptions To Ignore
[2011-02-19 00:22:26] <WeirdAl>     > NO_INTERFACE
[2011-02-19 00:22:30] <WeirdAl>     > StopIteration (future)
[2011-02-19 00:22:31] <WeirdAl> that is, under the Debug menu, we'd want a new submenu
[2011-02-19 00:23:06] <WeirdAl> for the most common, most irritating exceptions


I'd propose a different, more general approch.
Instead of having a pref ignoreNoInterface just for one special case, consider a kind of "lastThrowMode" for each possible exception:

what_to_do = lastThrowMode
if .throwMode.<ExceptionName> exists
  what_to_do = .throwMode.<ExceptionName>

Then, we could extend the menus:
  debug
  +- Throw Trigger
     +- ( ) Ignore      // these would be taken
     +- ( ) Trace       // for exceptions without
     +- ( ) Stop        // their own settings
     +- -----------
     +- <ExceptionName>
        +- ( ) Ignore   // settings specific
        +- ( ) Trace    // for just this type
        +- ( ) Stop     // of exception

As long as you don't have the pref(s), your UI stays as is; or we could preadd certain prefs like NS_ERROR_NO_INTERFACE - we'd have a very flexible way to handle this.

(This could as well be imposed upon the errorMode menu.)

Comment 13

[Karsten Düsterloh]

Attachment: more generic attempt

So, here's a prove of concept patch for my proposal in comment 12, for both Error Triggers and Throw Triggers.
Basically, the global setting is still stored as it is now, i.e. extensions.venkman.lastErrorMode and .lastThrowMode. But now you can override the global setting for each error code and exception code individually!
Once you do this, the overrides will appear in the menu as described in comment 12. To specify an override, use Venkman's shell commands /emode and /tmode, which have been extended to cope with overrides as well. 
For example, 
  /tmode trace
  /tmode NS_ERROR_NO_INTERFACE ignore
  /tmode 0x8000ffff ignore
will trace all exceptions except for NS_ERROR_NO_INTERFACE and 0x8000ffff. 

Same goes for error codes (see http://mxr.mozilla.org/comm-central/source/mozilla/js/src/js.msg), although you can't use symbolic names there.

I had to roll in three drive-by fixes for Venkman:
- the possibility to add a callback for manual menu construction
- new preferences were not saved by the interactive shell
- subpopups' onpopshowing handler got called multiple times

Al, what do you think?

Comment 14

[Alex Vincent [:WeirdAl]]

Comment on attachment 516445 [details] [diff] [review]
more generic attempt

The first question I'm going to ask is, "How have you tested this?"  There's a lot to go through in this patch, so I'd like some reassurance that it works as intended, especially since it's so much larger than the original patch I submitted - by more than an order of magnitude.

We really should talk about some kind of automated regression testing for Venkman, but that's for another bug.

>diff --git a/locales/en-US/chrome/venkman.properties b/locales/en-US/chrome/venkman.properties
>+cmd.emode.help   = Sets what action the debugger should take when an error occurs in the debug target.  |emode ignore| ignores all errors, |emode trace| shows a log of the error in the console, and |emode break| stops execution when an error is thrown.  |emode| without any parameter will display the current error mode.  By specifying an errorcode, you can override the behaviour for specific errors; errorcode can be a number like 0x9c or 156.  Note that |emode| controls what happens when an exception goes uncaught, to control what happens when an exception is *thrown*, use |tmode|.

"errorcode can be a number like 0x9c or 156."

Can it be 3.14159, or Infinity, or NaN?  Please be more specific.

What'd really be nice is if we could use names of exceptions from Components.results, or other common errors (like StopIteration).  This would save people from looking up the exact error code in some jsError table.  (I see you did that for tmode.)

"Note that |emode| controls what happens when an exception goes uncaught, to control what happens when an exception is *thrown*, use |tmode|."

Nit:  I'd suggest replacing the comma with a semicolon, or split it into two sentences there.

>diff --git a/resources/content/menu-manager.js b/resources/content/menu-manager.js
>-    var cx;
>+
>     var popup = event.originalTarget;
>-    var menuitem = popup.firstChild;
> 
>-    
>     /* If the host provided a |contextFunction|, use it now.  Remember the
>      * return result as this.cx for use if something from this menu is actually
>      * dispatched.  this.cx is deleted in |hidePopup|. */
>+    var cx;

Why move the cx declaration?  It doesn't really gain us anything.  (Also, for "use strict", the preference is to move variable declarations to the top, I believe... but that's a nit.  I don't think we'll ever convert wholesale to use strict JS.)

>+    // Dynamic popups might want to alter their content.
>+    if (typeof popup.popupshowing == "function")
>+        popup.popupshowing(cx);

Can popupshowing throw an exception?  How should we handle that?

>diff --git a/resources/content/venkman-commands.js b/resources/content/venkman-commands.js

> function cmdEMode (e)

I see you're practically rewriting this function.  Although it's not necessarily the local code style, I think you should JavaDoc on this function.  That would clarify what this function is for.  (No, the name's not enough - people reading the code will wonder "what's an emode?")  Especially, you should clarify what e is, what it looks like.

>+    // There are several ways to get here:
>+    // · menu: Debug → Error Trigger → ignore/trace/break
>+    // · menu: Debug → Error Trigger → error code → ignore/trace/break
>+    // · console: /emode command
>+    // The menu commands only provide actions ignore/trace/break
>+    // and cannot pass the errorcode parameter properly.
>+    // The console command provides ignore/trace/break/cycle/delete
>+    // and may omit parameters.
>+    // The command parameter signature is

Several non-standard character here - clean-up?  Maybe convert into /* */?

>+        // originalEvent only exists for menu calls
>+        var errorcode = e.originalEvent.originalTarget.getAttribute("menuName");

Camelcase attributes in XUL?  That's unusual.  Not wrong, but unusual.

>+        if (errorcode != "mainmenu:debug")
>+        {
>+            // read error code from dynamically created submenu
>+            e.action    = e.errorcode;
>+            e.errorcode = errorcode;
>+        }

I do a double-take every time I see this.  I need some clarification on the difference between action and errorcode.

>+    // If action is empty, then errorcode is either a true error code (where we
>+    // just show its override emode) or a request to change the global emode.
>+    if (!e.action)
>+    {
>+        if (e.errorcode == EMODE_IGNORE ||
>+            e.errorcode == EMODE_TRACE  ||
>+            e.errorcode == EMODE_BREAK  ||
>+            e.errorcode == EMODE_CYCLE  ||
>+            e.errorcode == EMODE_DELETE)
>         {

Would a switch (e.errorcode) help here?

>+        action = String(e.action).toLowerCase();
>+        if (action != EMODE_IGNORE &&
>+            action != EMODE_TRACE  &&
>+            action != EMODE_BREAK  &&
>+            action != EMODE_CYCLE  &&
>+            (action != EMODE_DELETE || !e.errorcode))

Would a switch (action) help here?

>+        // errorcode must be a number, because the JS error names in
>+        // http://mxr.mozilla.org/comm-central/source/mozilla/js/src/js.msg
>+        // are not acessible from JS currently

Spelling nit:  "accessible"

>+        errorcode = Number(e.errorcode);

parseInt?  parseFloat?

Also, we're converting from a string earlier (the getAttribute call earlier) to a number now.  I know it's okay, but I've heard it causes some churn for JIT JS execution.  Not sure if that matters here.

>+    // exec action
>+    if (e.action)
>+    {
>+        switch (e.action)
>+        {

Something about this strikes me as wrong.  There's only six values for which e.action would evaluate to false:  false, 0, NaN, "", undefined and null.  You can check for all of those inside the switch statement, though you probably only need to check for one or two of them.  (I hope!)

>+                switch (action)
>+                {
>+                    case EMODE_IGNORE:
>+                        e.action = EMODE_TRACE;
>+                        break;
>+                    case EMODE_TRACE:
>+                        e.action = EMODE_BREAK;
>+                        break;
>+                    case EMODE_BREAK:
>+                        e.action = EMODE_IGNORE;
>+                        break;
>+                    default:
>+                        // cycle without set value => create as ignore
>+                        e.action = EMODE_IGNORE;
>+                }

Why do I get the feeling we're playing Rock, Paper, Scissors here? :)  Whatever the action is, choose the next action?  Must be that funny EMODE_CYCLE.  Do we even use that?

cmdEMode (e) is a very long function by the time we reach the end of it... is it worth breaking up into smaller functions?

I think most of the comments I made about cmdEMode apply to cmdTMode as well.

>diff --git a/resources/content/venkman-debugger.js b/resources/content/venkman-debugger.js

>+// errorNumber is the JS error number as defined in
>+// http://mxr.mozilla.org/comm-central/source/mozilla/js/src/js.msg
>+function jsdErrorHook (message, fileName, line, pos, flags, errorNumber)

Another function heavily rewritten.  JavaDoc while you're at it?

>@@ -1615,23 +1634,35 @@ function testBreakpoint(currentFrame, rv
>+// Error Mode
>+const EMODE_PREF       = "lastErrorMode";
>+const EMODE_BRANCH     = EMODE_PREF + ".";
>+const EMODE_BRANCH_LEN = EMODE_BRANCH.length;
>+const EMODE_IGNORE = "ignore";
>+const EMODE_TRACE  = "trace";
>+const EMODE_BREAK  = "break";
>+const EMODE_CYCLE  = "cycle";   // not saved
>+const EMODE_DELETE = "delete";  // not saved
> 
>-const TMODE_IGNORE = 0;
>-const TMODE_TRACE  = 1;
>-const TMODE_BREAK  = 2;
>+// Throw Mode
>+const TMODE_PREF       = "lastThrowMode";
>+const TMODE_BRANCH     = TMODE_PREF + ".";
>+const TMODE_BRANCH_LEN = TMODE_BRANCH.length;
>+const TMODE_IGNORE = "ignore";
>+const TMODE_TRACE  = "trace";
>+const TMODE_BREAK  = "break";
>+const TMODE_CYCLE  = "cycle";   // not saved
>+const TMODE_DELETE = "delete";  // not saved

Just a thought - consider something like this:

const ERROR_MODES = {
  // Ignore errors.
  IGNORE: "ignore",

  // Trace through errors
  TRACE: "trace",

  // ...
};

const THROW_MODES = {
  // ...
};


>diff --git a/resources/content/venkman-menus.js b/resources/content/venkman-menus.js
>+        popupshowing: function(cx)
>+        {
>+            // remove all dynamic menuitems
>+            while (this.lastChild.nodeName != "menuseparator")
>+                this.removeChild(this.lastChild);
>+            // do we have new ones?
>+            var overrides = console.prefManager.listPrefs(EMODE_BRANCH);
>+            this.lastChild.setAttribute("visibleif", overrides.length > 0);
>+            // add new items
>+            for (var i = 0; i < overrides.length; ++i)
>+            {

Is it possible that the dynamic menuitems you just removed are identical to the ones you're about to insert?  If so, could they be reused?  Could we detect for that by checking the pre-existing menu has an attribute matching the one we want to inject?

>+                // errorName is always a number, because the JS error names in
>+                // http://mxr.mozilla.org/comm-central/source/mozilla/js/src/js.msg
>+                // are not acessible from JS currently
>+                var errorName = overrides[i].substr(EMODE_BRANCH_LEN);
>+                var errorNumber = Number(errorName);
>+                if (!isNaN(errorNumber))
>+                {

Maybe |if (isNaN(errorNumber)) continue;| ?  Save a little on indentation.

>+                    // create the new dynamic submenu
>+                    var menuid = this.id + ":" + errorNumber;
>+                    var subMenu = console.menuManager.appendSubMenu(this, null, errorNumber, menuid, errorName, null, null);
>+                    console.menuManager.createMenuItems(subMenu, null,
>+                        [
>+                         ["em-ignore",
>+                                 {type: "radio", name: "tm",
>+                                  checkedif: "console.prefs['" + overrides[i] + "'] == EMODE_IGNORE"}],
>+                         ["em-trace",
>+                                 {type: "radio", name: "tm",
>+                                  checkedif: "console.prefs['" + overrides[i] + "'] == EMODE_TRACE"}],
>+                         ["em-break",
>+                                 {type: "radio", name: "tm",
>+                                  checkedif: "console.prefs['" + overrides[i] + "'] == EMODE_BREAK"}]
>+                        ]
>+                    );

I wonder if we're overoptimizing on the checkedif setting... but oh well.

>+                }
>+            }
>+        },

This function begs for empty lines to help with readability.

My comments about emode popupshowing appear to apply to tmode popupshowing as well.

>diff --git a/resources/content/venkman-prefs.js b/resources/content/venkman-prefs.js
>+         [EMODE_PREF, EMODE_IGNORE],
>+         [TMODE_PREF, TMODE_IGNORE],

I trust you can prove beyond all doubt that these variables are defined before venkman-prefs.js executes this code.  Since it's chrome, all you need to show me is that the code that defines them executes in an earlier file, first.

feedback+ from me, if you answer all the nits above - especially the "How did you test this" part.

Comment 15

[Alex Vincent [:WeirdAl]]

By "answer" I mean look into, not necessarily implement. :)

Comment 16

[:Gijs (he/him)]

Just as a note, the 'e' parameter is always there for commands. It's a single object holding any parameters to the command, "and maybe other stuff". I don't think it makes sense to document it just for these functions, in javadoc. The old venkman convention is that the 'right' way to document the particular parameters a command takes is by using the cmd.whatever.help text in the localization files. If there are other magical properties on 'e' that the function expects/uses which are somehow unclear, I would say the (block of!) code using those properties should have a comment.

Comment 17

[Karsten Düsterloh]

Yay for resizable text areas! ;-)

(In reply to comment #14)
> The first question I'm going to ask is, "How have you tested this?"

The code was written with "although I change the two commands' signature, they should still work as they did before when called with correct parameters". I tested the functionality from both the menus and the interactive shell. I think I tested all combinations of "good" parameters, and quite some bad ones to check for invalid conditions. I faked invalid pref entries to see whether that'd break something. Of course, I checked if ignore/trace/break with a few selected errors and exceptions.
Of course, I can't guarantee that I didn't miss something, but if you don't use any of the new features, I'm rather certain that nothing bad can happen.

> We really should talk about some kind of automated regression testing for
> Venkman, but that's for another bug.

Yes, although I'm not quite sure how do that for the actual hook code. Autotesting commands should work, though, I guess.

> >diff --git a/locales/en-US/chrome/venkman.properties b/locales/en-US/chrome/venkman.properties
> >+cmd.emode.help   = Sets what action the debugger should take when an error occurs in the debug target.  |emode ignore| ignores all errors, |emode trace| shows a log of the error in the console, and |emode break| stops execution when an error is thrown.  |emode| without any parameter will display the current error mode.  By specifying an errorcode, you can override the behaviour for specific errors; errorcode can be a number like 0x9c or 156.  Note that |emode| controls what happens when an exception goes uncaught, to control what happens when an exception is *thrown*, use |tmode|.

Note that I only added the sentence "By specifying …" here and for /tmode, the rest of the text is already there as is.

> "errorcode can be a number like 0x9c or 156."
> 
> Can it be 3.14159, or Infinity, or NaN?  Please be more specific.

It has to be an error code from js.msg, i.e. an integer currently between 0 and 269. Other values don't harm, but won't work, of course. 

> What'd really be nice is if we could use names of exceptions from Components.results,

I do for exceptions, I can't for errors, because the names from js.msg aren't reflected into JS, AFAICT.
We could (should we? would it be used/helpful?) add the names to Venkman, but I left that out in this poc. ;-)

> or other common errors (like StopIteration).

Can you point me to an example/testcase I can test against?

> " "Note that |emode| controls what happens when an exception goes uncaught, to
> control what happens when an exception is *thrown*, use |tmode|."
> 
> Nit:  I'd suggest replacing the comma with a semicolon, or split it into two
> sentences there.

Can do (although not my text).

> >     /* If the host provided a |contextFunction|, use it now.  Remember the
> >      * return result as this.cx for use if something from this menu is actually
> >      * dispatched.  this.cx is deleted in |hidePopup|. */
> >+    var cx;
> 
> Why move the cx declaration?  It doesn't really gain us anything.

Oh, but it does, it's closer to where it's used.

> (Also, for "use strict", the preference is to move variable declarations to the
> top, I believe... 

Source? 
Personally, I really hate the Pascal way of variables. ;-)

> >+    // Dynamic popups might want to alter their content.
> >+    if (typeof popup.popupshowing == "function")
> >+        popup.popupshowing(cx);
> 
> Can popupshowing throw an exception?  How should we handle that?

Good point. popupshowing in general might throw, because it can be arbitrarily complex. My popupshowing functions call back into other Venkman code which might throw. 
We probably should just safeguard the call above, although it might not work in certain circumstances (and if it throws, something is in bad shape anyway!).

> > function cmdEMode (e)
> 
> I see you're practically rewriting this function.  Although it's not
> necessarily the local code style, I think you should JavaDoc on this function. 

Yes, good point. "Local code style" would be "no doc"? ;-)

> >+    // · menu: Debug → Error Trigger → ignore/trace/break
> >+    // · menu: Debug → Error Trigger → error code → ignore/trace/break
...
> Several non-standard character here - clean-up?

Huh? That's just plain UTF-8, like all Mozilla files. No need to cleanup anything here.

> >+        // originalEvent only exists for menu calls
> >+        var errorcode = e.originalEvent.originalTarget.getAttribute("menuName");
> 
> Camelcase attributes in XUL?  That's unusual.  Not wrong, but unusual.

Yeah, I noticed that, too. Not my choice, though.

> >+        if (errorcode != "mainmenu:debug")
> >+        {
> >+            // read error code from dynamically created submenu
> >+            e.action    = e.errorcode;
> >+            e.errorcode = errorcode;
> >+        }
> 
> I do a double-take every time I see this.  I need some clarification on the
> difference between action and errorcode.

The commands have two parameters, both of which are optional. So, if you just have one argument, it could belong to the first or second parameter. If the command is evoked from the menu, the Venkman system does not allow arbitrary arguments and the existing stuff it uses does not know about the new errorcode parameter.
Venkmans cmd.*.param settings provide machine-parsable parameter definitions. This means
  cmd.tmode.params = [<errorcode>] [<action>]
will make the e parameter of the cmdTMode look like this:
  e.errorcode = first parameter given to /tmode call
  e.action    = second parameter given to /tmode call
The menu item call can't pass the errorcode parameter, hence the automatic parameter parsing drops the action value into the errorcode, which the above code corrects.

I should probably rename errorcode to errorNumber in cmdEMode and to excNumber in cmdTMode.
 
> >+    if (!e.action)
> >+    {
> >+        if (e.errorcode == EMODE_IGNORE ||
> >+            e.errorcode == EMODE_TRACE  ||
> >+            e.errorcode == EMODE_BREAK  ||
> >+            e.errorcode == EMODE_CYCLE  ||
> >+            e.errorcode == EMODE_DELETE)
> >         {
> 
> Would a switch (e.errorcode) help here?

No, why should it? It'd do the same, just longer.

> >+        errorcode = Number(e.errorcode);
> 
> parseInt?  parseFloat?

Why? What would be the advantage of using parseInt?
(And parseFloat doesn't make much sense anyway.)

> Also, we're converting from a string earlier (the getAttribute call earlier) to
> a number now.  I know it's okay, but I've heard it causes some churn for JIT JS
> execution.  Not sure if that matters here.

Source?
But also, well, I can't help it anyway - the shell command parameters are string by default, as are XUL attributes; but the error/exception numbers are integers …

> >+    if (e.action)
> >+    {
> >+        switch (e.action)
> >+        {
> 
> Something about this strikes me as wrong.  There's only six values for which
> e.action would evaluate to false:  false, 0, NaN, "", undefined and null.  You
> can check for all of those inside the switch statement, though you probably
> only need to check for one or two of them.  (I hope!)

That is just a perf optimization - it doesn't make much sense to test a known invalid action against several values which we know will not match, before doing nothing in the end …

> >+                switch (action)
> >+                {
> >+                    case EMODE_IGNORE:
> >+                        e.action = EMODE_TRACE;
> >+                        break;
...
> Why do I get the feeling we're playing Rock, Paper, Scissors here? :)  Whatever
> the action is, choose the next action?  Must be that funny EMODE_CYCLE.

That's why that code is part of the "case EMODE_CYCLE:" branch. :-P

> Do we even use that?

Well, there's even a shortcut bound to both cycle commands (ctrl-shift-e/ctrl-t), but it's not available in the menus.
(My first thought was "WTF is this?!". ;-) )

> cmdEMode (e) is a very long function by the time we reach the end of it... is
> it worth breaking up into smaller functions?

Not really. I considered joining it with cmdTMode, which is almost identical by structure, but that would mean too much passing around of parameters to be useful.

> I think most of the comments I made about cmdEMode apply to cmdTMode as well.

The only real difference is that cmdTMode tries to cope with exception names.

> >-const TMODE_IGNORE = 0;
> >-const TMODE_TRACE  = 1;
> >-const TMODE_BREAK  = 2;

Just as a side note: these integer values were complete crap - each time something important was happening, they were converted back to the command texts …

> Just a thought - consider something like this:
> 
> const ERROR_MODES = {
>   // Ignore errors.
>   IGNORE: "ignore",

Mmh, yes, it's appealing.

> >diff --git a/resources/content/venkman-menus.js b/resources/content/venkman-menus.js
...
> Is it possible that the dynamic menuitems you just removed are identical to the
> ones you're about to insert?

Yes, in fact, I assume that they most probably are, as you you won't change your pet exceptions very often. ;-)

> If so, could they be reused? Could we detect for that by checking the pre-existing
> menu has an attribute matching the one we want to inject?

Yes, good point. 

> Maybe |if (isNaN(errorNumber)) continue;| ?  Save a little on indentation.

Yeah, agreed.

> >+                         ["em-break",
> >+                                 {type: "radio", name: "tm",
> >+                                  checkedif: "console.prefs['" + overrides[i] + "'] == EMODE_BREAK"}]
> >+                        ]
> >+                    );
> 
> I wonder if we're overoptimizing on the checkedif setting... but oh well.

I don't understand?

> >+                }
> >+            }
> >+        },
> 
> This function begs for empty lines to help with readability.

Huh? Especially here, empty lines would make things worse, IMO …

> >diff --git a/resources/content/venkman-prefs.js b/resources/content/venkman-prefs.js
> >+         [EMODE_PREF, EMODE_IGNORE],
> >+         [TMODE_PREF, TMODE_IGNORE],
> 
> I trust you can prove beyond all doubt that these variables are defined before
> venkman-prefs.js executes this code.  Since it's chrome, all you need to show
> me is that the code that defines them executes in an earlier file, first.

Erm, I may be missing something, but these constants are defined in the global scope of their file, and we only get into Venkman's onload handler, which will call it, after all XUL incl. scripts is loaded and parsed?!
So how would that not work?
(JFTR: venkman-debugger.js is included before venkman-prefs.js venkman-scripts.xul.)


BTW1: I made a typo in cmdTMode: TEMODE_IGNORE should be TMODE_IGNORE, of course.
BTW2: I'll appreciate comments/testing from others as well. ;-)

Comment 18

[Alex Vincent [:WeirdAl]]

(In reply to comment #17)
> > What'd really be nice is if we could use names of exceptions from Components.results,
> 
> I do for exceptions, I can't for errors, because the names from js.msg aren't
> reflected into JS, AFAICT.
> We could (should we? would it be used/helpful?) add the names to Venkman, but I
> left that out in this poc. ;-)

Maybe a build step TODO:  add a python script to read js.msg and translate it into a JSON file, say, at resource://gre/metadata/js_msg.json.

> > or other common errors (like StopIteration).
> 
> Can you point me to an example/testcase I can test against?

Not immediately, though I can probably write one without much difficulty.  Reimplementing TreeWalker in JS is a potential use case for StopIteration.

https://developer.mozilla.org/en/Core_JavaScript_1.5_Guide/Iterators_and_Generators talks about this error (exception?  I always get those confused) in the context of an iterator.  (To my knowledge, Venkman hasn't supported iterators all that well, so it's something we need to test anyway.)

http://mxr.mozilla.org/mozilla-central/search?string=throw+StopIteration&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central provides some hints, but I'd skip the domplate.jsm one (they define their own StopIteration value, which I find nonsensical).

> > (Also, for "use strict", the preference is to move variable declarations to the
> > top, I believe... 
> 
> Source? 
> Personally, I really hate the Pascal way of variables. ;-)

I was wrong:  it's functions inside functions that have to be moved to the top...

http://whereswalden.com/2011/01/24/new-es5-strict-mode-requirement-function-statements-not-at-top-level-of-a-program-or-function-are-prohibited/


> > Also, we're converting from a string earlier (the getAttribute call earlier) to
> > a number now.  I know it's okay, but I've heard it causes some churn for JIT JS
> > execution.  Not sure if that matters here.
> 
> Source?

I can't find it on a quick search, so I'm probably wrong here.

Comment 19

[Alex Vincent [:WeirdAl]]

This is your baby at this point, Gijs.  :)

Comment 20

[Alex Vincent [:WeirdAl]]

Whoops, it's actually Karsten's.  Sorry about that!

Comment 21

[BMO Automation]

Component is obsolete so resolving bugs as INCOMPLETE