Closed Bug 1099071 Opened 5 years ago Closed 4 years ago

Throwed empty string are not displayed in console

Categories

(DevTools :: Console, defect)

33 Branch
x86
Linux
defect
Not set

Tracking

(firefox47 verified, firefox48 verified)

VERIFIED FIXED
Firefox 47
Tracking Status
firefox47 --- verified
firefox48 --- verified

People

(Reporter: michalwadas, Assigned: ajkerrigan)

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141013200110

Steps to reproduce:

I typed in console:

(function(){
throw '';
})();


Actual results:

Nothing was displayed.


Expected results:

Some information should be displayed.
Component: Untriaged → Developer Tools: Console
Similar problems with empty-string .toString

(function(){
throw Object.create(null)
})();


(function(){
throw {toString: null, valueOf: null};
})();
Confirmed.  The server is returning exceptionMessage = "" and exception = "".  For an evaluation that doesn't throw it returns exception = "" and no exceptionMessage, so maybe that's enough to go off of in the frontend.  The exceptionMessage is passed in here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#3166.  Then looks like a bad falsy check here causes it to not show: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/console-output.js#1352.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → ajkerrigan
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
The main change here is to the falsy check in console-output.js,
but that didn't feel like a complete fix. Throwing an empty string
would then correctly show an error line in the console, but the
line would be blank.

As a possible alternative, I made a minor tweak to webconsole.js
so that it constructs an Error object in the case of a thrown
string (the behavior for thrown -objects- is unchanged). Here
are the changes to error output:

Thrown String --> Console Error Output

'' --> Error
' ' --> Error: 
'test' --> Error: test

Before submitting this patch I peeked around at some other dev
tooling to see how these cases were handled out of curiosity.
The behavior is different everywhere, but this change seems to
match up with how Firebug handles thrown strings.
Attachment #8721160 - Flags: review?(bgrinstead)
Comment on attachment 8721160 [details] [diff] [review]
patch v1

Review of attachment 8721160 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  Can you add a few test cases for this to https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/test/browser_webconsole_jsterm.js?

::: devtools/client/webconsole/webconsole.js
@@ +3114,5 @@
>        Cu.reportError("Evaluation error " + response.error + ": " +
>                       response.message);
>        return;
>      }
> +    let errorMessage = typeof(response.exception) === "string" ? new Error(response.exceptionMessage).toString() : response.exceptionMessage;

This generally makes sense (although it took me a while to work through all the variations of RDP traffic to confirm exactly what cases this was covering).  I think it needs some explanation in a comment though.  Something like:

  let errorMessage = response.exceptionMessage;
  // Stringify thrown strings as Errors so `throw "foo"` outputs as "Error: foo"
  if (typeof(response.exception) === "string") {
    errorMessage = new Error(response.exceptionMessage).toString();
  }
Attachment #8721160 - Flags: review?(bgrinstead) → feedback+
Comment on attachment 8721160 [details] [diff] [review]
patch v1

Review of attachment 8721160 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the quick feedback! I'll explain the handling of thrown strings and add some test cases.

::: devtools/client/webconsole/webconsole.js
@@ +3114,5 @@
>        Cu.reportError("Evaluation error " + response.error + ": " +
>                       response.message);
>        return;
>      }
> +    let errorMessage = typeof(response.exception) === "string" ? new Error(response.exceptionMessage).toString() : response.exceptionMessage;

You make a good point. I was concerned it might be a bit too "magical" and I think the way you reworked it makes things a lot easier to grok at a glance.
Attached patch patch v2Splinter Review
Attachment #8721905 - Flags: review?(bgrinstead)
Attachment #8721160 - Attachment is obsolete: true
Comment on attachment 8721905 [details] [diff] [review]
patch v2

Review of attachment 8721905 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  This needed a rebase due to Bug 1249715, but I've done that and pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d9ed84b718d.  Let's land it if that turns up green.
Attachment #8721905 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/046e5f381e66
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
I've reproduced this bug on Nightly 36.0a1 (2014-11-14) on Ubuntu 14.04 LTS, 32-bit!

This bug fixed is verified on Latest Nightly 48.0a1!

Build ID: 20160308030418
User Agent: Mozilla/5.0 (X11; Linux i686; rv:48.0) Gecko/20100101 Firefox/48.0
QA Whiteboard: [bugday-20160309]
I have reproduced this bug according to (2014-11-14)

It's fixed on Latest Developer Edition -- Build ID (20160408004012), User Agent: Mozilla/5.0 (Windows NT 6.3; rv:47.0) Gecko/20100101 Firefox/47.0

Tested OS-- Windows8.1 32bit
[bugday-20160406]
I have reproduced this bug with Firefox nightly 36.0a1(2014-11-14)on
windows 7(64 bit)

I have verified this bug as fixed with Firefox aurora 47.0a2(build Id:20160415004038)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
As this bug is verified on both Linux (comment 10) and Windows (Comment 11), I am marking this as verified!
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.