327349 - Add JavaScript NS_ASSERT function to globalOverlay.js

Status: RESOLVED FIXED

(Toolkit Graveyard :: Data Collection/Metrics, enhancement)

Description

[Joe Hughes]

Ben Goodger has written a handy JS ASSERT function that can print stack traces--you can see it here:
http://lxr.mozilla.org/mozilla/source/browser/components/places/content/controller.js#44

It'd be nice to put it (or something like it) in a more generally-accessible location.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Maybe make it not pass in null instead of a window, and then put it in a separate file, assert.js.

Then we can #include it all over the place. The advantage of doing it this way is that we can #include it from js components. 

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

(Extension Manager needs ASSERT badly)

Comment 3

[Joe Hughes]

Attachment: Patch containing rough outline of change (see discussion)

What do you think about the location and means of inclusion expressed in this patch?  Also, I added a global to determine whether the alert box will be shown on assertion failure or not.  My sense is that this should happen during betas, but not in release builds.  Would this be better as an #ifdef or about:config pref?

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214699 [details] [diff] [review]
Patch containing rough outline of change (see discussion)

>Index: toolkit/content/globalOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/globalOverlay.js,v
>retrieving revision 1.18.8.2
>diff -u -u -8 -p -r1.18.8.2 globalOverlay.js
>--- toolkit/content/globalOverlay.js	1 Feb 2006 02:29:57 -0000	1.18.8.2
>+++ toolkit/content/globalOverlay.js	10 Mar 2006 19:16:14 -0000
>@@ -1,8 +1,10 @@
>+#include assert.js
>+

I would be inclined to stick this at the end of the file, rather than at the start, to preserve line numbering.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

If this is be used with JS Components, ps.alert(window, ...) won't work (there is no window). Is that what gShowAssertionAlerts was meant to be used for (overridden after inclusion)?

Comment 6

[Alex Vincent [:WeirdAl]]

Hm, I wrote a nice little assert() function for jslib, but we haven't checked it in yet:

http://bugzilla.mozdev.org/show_bug.cgi?id=7367

You might want to take a look.

biesi points out catchable exceptions are probably not good, but if it's going to the console, I really would prefer to see filename, line number, source line, etc, a la nsIScriptError.

Comment 7

[Joe Hughes]

Attachment: Initial JS ASSERT implementation

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214737 [details] [diff] [review]
Initial JS ASSERT implementation

I also have many ideas for improving this but this is a good start. r=[e-mail]. Gavin, do you want to take another look at this too?

Comment 9

[Nickolay_Ponomarev]

Comment on attachment 214737 [details] [diff] [review]
Initial JS ASSERT implementation

>+    if (window)
>+      source = window;
This still doesn't work in XPCOM components. I would use "if (this.window)", although it may be considered a hack.

>+      Components.classes["@mozilla.org/embedcomp/prompt-service;1"].
>+      getService(Components.interfaces.nsIPromptService);
The common style is to write:
  Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
            .getService(Components.interfaces.nsIPromptService);

Comment 10

[Darin Fisher]

Comment on attachment 214737 [details] [diff] [review]
Initial JS ASSERT implementation

>Index: toolkit/content/assert.js
...
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

For vim users:

/* vim:set ts=2 sw=2 sts=2 ci et: */


>+var gTraceOnAssert = true;
>+
>+/**
>+ * This function provides a simple assertion function for JavaScript.
>+ * If the condition is true, this function will do nothing.  If the
>+ * condition is false, then the message will be printed to the console
>+ * and an alert will appear showing a stack trace, so that the (alpha
>+ * or nightly) can file a bug.  For future enhancements, see 
>+ * bugs 330077 and 330078.

I'd recommend fixing bug 330077 now rather than later.  It isn't too 
hard to do.  At least, we could get all of the basic glue for it in
place, and then leave it to bug 330077 to introduce a MOZCONFIG option.
You'd just need to preprocess this file, and use #ifdef SOME_FLAG.


>+function ASSERT(condition, message) {

Hmm... what do you think about prefixing this function with "NS_"?
Since this script is going to be included into every XUL document,
namespacing our globals might be nice.


>+  if (!condition) {

nit: return early to reduce overall indentation in this function:

    if (condition)
      return;


>+    var caller = arguments.callee.caller;
>+    var str = "ASSERT: ";
>+    str += message;

      var str = "ASSERT: " + message;

Not that performance matters here...


>+    var ps =
>+      Components.classes["@mozilla.org/embedcomp/prompt-service;1"].
>+      getService(Components.interfaces.nsIPromptService);
>+    ps.alert(source, "Assertion Failed", assertionText + stackText);

Oh, these are going to be annoying.  I think you should provide a
hidden preference to turn off these assertion alerts.  A couple reasons:

1) Some developers will want to use a build even if some UI is asserting
badly.  Suppose for example that you are working to fix the assert, you
wouldn't want the whole development community to be breathing down your
back waiting for the fix.  Give them a way to turn off the alerts ;-)

2) If I'm running a performance test, I don't want modal alert dialogs
screwing up my performance test.  I want to be able to run the test w/o
having to keep an eye on the browser.

It might even make sense to use an environment variable instead of a
pref to control whether or not the alert is shown.  That way, developers
can suppress the alerts temporarily for a particular browser session:

$ XUL_ASSERT_PROMPT=0 ./firefox -profile test

See nsIEnvironment to get access to environment variables from script.


>Index: toolkit/content/jar.mn

>+   content/global/assert.js                   (assert.js)

Add a "*" prefix here to enable preprocessing if you want to conditionalize
the ASSERT function for different build configurations.


>Index: browser/components/places/content/bookmarkProperties.js

> #include controller.js
>+#include ../../../../toolkit/content/assert.js

Ideally, there should be a search path for preprocessor-based includes.
I'd recommend talking to bsmedberg about this.  He'll have a good
suggestion I bet.  If you don't want to do that before checking in this
fix, then please file a bug (maybe against "Core: Build Config") and CC
him ([e-mail]).

Comment 11

[Joe Hughes]

Attachment: New JS NS_ASSERT() patch which incorporates all current feedback

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 214952 [details] [diff] [review]
New JS NS_ASSERT() patch which incorporates all current feedback

>Index: toolkit/content/debug.js

+ var NS_ASSERT_ENVIRONMENT_VARIABLE_NAME = "XUL_ASSERT_PROMPT";

Nit: should be const. Maybe worth adding a comment about setting XUL_ASSERT_PROMPT=0 being the way to turn off the dialog, and that it defaults to on?

>+function NS_ASSERT(condition, message) {

>+  var stackText = "Stack Trace: \n";
>+  if (gTraceOnAssert) {

Shouldn't stackText be initialized inside if (gTraceOnAssert)? Looks like you'll get "Stack trace: " followed by nothing if 
gTraceOnAssert is false.

>+    var count = 0;
>+    while (caller) {
>+      stackText += count++ + ":" + caller.name + "(";
>+      for (var i = 0; i < caller.arguments.length; ++i) {
>+        var arg = caller.arguments[i];
>+        stackText += arg;
>+        if (i < caller.arguments.length - 1)
>+          stackText += ",";
>+      }
>+      stackText += ")\n";
>+      caller = caller.arguments.callee.caller;
>+      }

nit: fix "}" indent

>+  var environment =
>+    Components.classes["@mozilla.org/process/environment;1"]
>+              .getService(Components.interfaces.nsIEnvironment);

I prefer the form:
  var environment = Components.classes["@mozilla.org/process/environment;1"].
                    getService(Components.interfaces.nsIEnvironment);
which removes the need for the wrappping, but I guess there really isn't any standard here, and I'm not sure whether other places code has a different convention.

>+  var ps = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                     .getService(Components.interfaces.nsIPromptService);

same comment as with "environment" above.

Comment 13

[Joe Hughes]

Attachment: NS_ASSERT patch as checked in to branch & trunk

Comment 14

[Joe Hughes]

Submitted on branch and trunk after incorporating Gavin's final suggestions and updating a couple stray ASSERT references that I had missed in browser.js

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 214957 [details] [diff] [review]
NS_ASSERT patch as checked in to branch & trunk

>Index: toolkit/content/debug.js

>+  if (gTraceOnAssert) {
>+    var stackText = "Stack Trace: \n";

The declaration and initialization to "" needs to be outside the if, otherwise stackTex will be "undefined" at:
>+  ps.alert(source, "Assertion Failed", assertionText + stackText);

>+  var environment = Components.classes["@mozilla.org/process/environment;1"].
>+                               getService(Components.interfaces.nsIEnvironment);

Seems as though getService is indented too much here, pushing this over 80 chars. Same with ps below. I meant to say that  getService should be aligned with the C in "Components".

>+  var ps = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].
>+                      getService(Components.interfaces.nsIPromptService);

Comment 16

[Joe Hughes]

Attachment: Fix to scoping error in the submitted patch (thanks Gavin)

Comment 17

[Joe Hughes]

Landed scoping error fix on branch & trunk.

Comment 18

[Asaf Romano (gone)]

Shall i ask why NS_ASSERT instead of just ASSERT?

Comment 19

[Nickolay_Ponomarev]

Mano: see comment 10. Not that I like the NS_ prefix very much myself...

Comment 20

[Nickolay_Ponomarev]

(Fixing the summary to reflect what has been done in this bug and noting that the #include of debug.js in globalOverlay.js was later removed in bug 330130.)