756681 - Scratchpad should display exceptions when throwing a string

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

[Allison]

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]

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]]

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 (he/him) [:past] (please ni?)]

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]

Attachment: 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]

Attachment: 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)]

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]

Attachment: Correcting exception comment, v3

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

Autotest works.

Comment 8

[Rob Campbell [:rc] (:robcee)]

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)]

Allison, any updates for us? would you like us to finish this patch?

Filter on BLACKEAGLE?!

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

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)]

https://hg.mozilla.org/mozilla-central/rev/68a0ed01021d