Scratchpad should display exceptions when throwing a string

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Developer Tools: Scratchpad
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Allison, Unassigned)

Tracking

unspecified
Firefox 17
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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 );
}
(Reporter)

Comment 1

5 years ago
Also doesn't work when trying to throw an object:

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

var bar = foo();

throw bar
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
I think SP_writeAsErrorComment() could become a little smarter:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#490
(Reporter)

Comment 4

5 years ago
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
*/
(Reporter)

Comment 5

5 years ago
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?
Attachment #625455 - Attachment is obsolete: true
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!
(Reporter)

Comment 7

5 years ago
Created attachment 626350 [details] [diff] [review]
Correcting exception comment, v3

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

Autotest works.
Attachment #625493 - Attachment is obsolete: true
Attachment #626350 - Flags: review?(rcampbell)
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!
Attachment #626350 - Flags: review?(rcampbell) → review+
Whiteboard: [has-patch][needs-update]
Allison, any updates for us? would you like us to finish this patch?

Filter on BLACKEAGLE?!
Priority: -- → P3
Rebased, removed the duplicate block and fixed a few recent test regressions:
https://hg.mozilla.org/integration/fx-team/rev/68a0ed01021d
Whiteboard: [has-patch][needs-update] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/68a0ed01021d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.