Last Comment Bug 756681 - Scratchpad should display exceptions when throwing a string
: Scratchpad should display exceptions when throwing a string
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Scratchpad (show other bugs)
: unspecified
: x86 Mac OS X
: P3 normal (vote)
: Firefox 17
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-18 15:47 PDT by Allison
Modified: 2012-08-03 14:23 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Correcting exception comment, v1 (1.36 KB, patch)
2012-05-19 19:39 PDT, Allison
no flags Details | Diff | Review
Correcting exception comment, v2 (6.09 KB, patch)
2012-05-20 08:53 PDT, Allison
no flags Details | Diff | Review
Correcting exception comment, v3 (5.41 KB, patch)
2012-05-23 00:24 PDT, Allison
rcampbell: review+
Details | Diff | Review

Description Allison 2012-05-18 15:47:16 PDT
When an exception is thrown, Scratchpad displays the exception as implemented by bug 690552.

When a string is thrown, no exception is displayed but execution will halt:

throw "a string error";

Handling a thrown string behaves as expected.  The following will log the original string to the console:

try {
  throw "a string inside a try";
}
catch (error) {
  console.log( error );
}
Comment 1 Allison 2012-05-18 15:55:10 PDT
Also doesn't work when trying to throw an object:

function foo() {
  console.log( "foo constructor" );
}

var bar = foo();

throw bar
Comment 2 Jason Orendorff [:jorendorff] 2012-05-18 16:19:03 PDT
In Nightly, if I run this code in Scratchpad:
  throw "FAIL";
I get this:

/*
Exception: undefined
undefined:undefined
*/


Same thing if I run this code:
  throw {};


Similarly if I run this:
  document.body.appendChild(document.body);
I get this:

/*
Exception: Node cannot be inserted at the specified point in the hierarchy
undefined:1
*/


So it looks like Scratchpad is just kind of assuming the exception has a .fileName, .lineNumber, and .message even if it's not a plain JS Error but something else.
Comment 3 Panos Astithas [:past] 2012-05-19 13:11:34 PDT
I think SP_writeAsErrorComment() could become a little smarter:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#490
Comment 4 Allison 2012-05-19 19:39:04 PDT
Created attachment 625455 [details] [diff] [review]
Correcting exception comment, v1

I've modified SP_writeAsErrorComment() but it isn't pretty.

This patch fixes two problems with the original code:

(1) In some cases, aError does not have .fileName, .lineNumber, or .message properties.

(2) Concatenation of undefined with a string becomes a string so || was not behaving as expected

A few test cases with their output:

throw "a thrown string";
/*
Exception: a thrown string
*/

throw {}
/*
Exception: [object Object]
*/

document.body.appendChild(document.body)
/*
Exception: Node cannot be inserted at the specified point in the hierarchy
@Scratchpad:10
*/
Comment 5 Allison 2012-05-20 08:53:09 PDT
Created attachment 625493 [details] [diff] [review]
Correcting exception comment, v2

Same patch as before but with tests added.

The tests all pass except for the fourth error case, document.body.appendChild(document.body)

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug756681_display_non_error_exceptions.js | Alternative format error display output - Got document.body.appendChild(document.body)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug756681_display_non_error_exceptions.js | Alternative format error display output - Got document.body.appendChild(document.body)

For this error case, no Exception is thrown during the automated test but there is an exception is thrown when tested manually.

Any ideas why the discrepancy?
Comment 6 Rob Campbell [:rc] (:robcee) 2012-05-22 06:37:08 PDT
Comment on attachment 625493 [details] [diff] [review]
Correcting exception comment, v2

hi Allison. Thanks for the patch!

+    let stack = ( aError.stack ? aError.stack :
+                 ( aError.fileName ?
+                  ( aError.lineNumber ? "@" + aError.fileName + ":" + aError.lineNumber : aError.fileName) :
+                  (aError.filename ?
+                   ( aError.lineNumber ? "@" + aError.filename + ":" + aError.lineNumber : aError.filename ) :
+                   ( aError.lineNumber ? "@" + aError.lineNumber : "" ))));    
+    let newComment = "Exception: " + ( aError.message || aError) + ( stack == "" ? stack : "\n" + stack.replace(/\n$/, "") );

Could you break this up into separate lines? having a bunch of nested conditional expressions like that is a bit hard to parse (for humans).

Also, for that kind of logic, you're probably better off using actual if() else if() else... style conditionals.

The mentioned failing test is probably just differing by some slight output. Check the test file and what your modified code is producing and make sure they match up. Adding some dump() statements around the failures might be helpful for debugging that.

Let us know if we can help any further!
Comment 7 Allison 2012-05-23 00:24:41 PDT
Created attachment 626350 [details] [diff] [review]
Correcting exception comment, v3

Replaced chained ternary operator with if, if else, else statement.

Autotest works.
Comment 8 Rob Campbell [:rc] (:robcee) 2012-06-01 07:06:26 PDT
Comment on attachment 626350 [details] [diff] [review]
Correcting exception comment, v3

+    else if (aError.fileName) {
+      if (aError.lineNumber) {
+        stack = "@" + aError.fileName + ":" + aError.lineNumber;
+      }
+      else {
+        stack = "@" + aError.fileName;
+      }
+    }
+    else if (aError.filename) {
+      if (aError.lineNumber) {
+        stack = "@" + aError.filename + ":" + aError.lineNumber;
+      }
+      else {
+        stack = "@" + aError.filename;
+      }
+    }

these two blocks are the same. You can remove the second else if condition.

Otherwise, looks good!
Comment 9 Rob Campbell [:rc] (:robcee) 2012-08-02 09:40:53 PDT
Allison, any updates for us? would you like us to finish this patch?

Filter on BLACKEAGLE?!
Comment 10 Panos Astithas [:past] 2012-08-03 01:57:39 PDT
Rebased, removed the duplicate block and fixed a few recent test regressions:
https://hg.mozilla.org/integration/fx-team/rev/68a0ed01021d
Comment 11 Rob Campbell [:rc] (:robcee) 2012-08-03 14:23:56 PDT
https://hg.mozilla.org/mozilla-central/rev/68a0ed01021d

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