Closed Bug 921366 Opened 11 years ago Closed 11 years ago

Wrong output for throw false/0/null/undefined;

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: asaf, Assigned: denschub)

References

Details

(Whiteboard: [good first bug][lang=js][mentor=msucan][good first verify])

Attachments

(1 file, 2 obsolete files)

Scratchpad fails to identify the exceptions thrown by each of these statements:

throw 0;
throw null;
throw false;
throw undefined;

The run action for any of these produces no output (comparing to "Excpetion: the [primitive value thrown]" for other primitives).

Interestingly, The display action for each of these exceptions produces outputs "undefined".

This is reproducible in current trunk and in Firefox 21. I haven't tested earlier versions.
Looking at the code without knowing much about devtools in general, the problem seems to be that SP_evaluate (in scratchpad.js) uses the value of aResponse.exception both as an indication that an exception was thrown for accessing the exception data. Now, because |throw [some primitive]| does not result in an exception *object*, the aResponse.exception fails to indicate that an exception was thrown for all those values.

Given that even |undefined| is a legitimate exception, I think the fix must involve both WCA_onEvaluateJS (webconsole.js) not to set the "exception" property if no exceptions was thrown and SP_evaluate (in scratchpad.js) to strictly check for existence ("exception" in aRespone).

However that approach somewhat ssumes some code-style/design assumptions about devtools which may be wrong, so I'll leave this for someone who knows the code.
This actually looks (mostly) like a bug with WebConsoleActor#onEvaluateJS, where it's doing a truthy test on the thrown value to determine whether to create a grip for it or not. This fails whenever a falsey value is thrown. The Scratchpad does also need to be fixed as well (so it won't fail when the error is "" or 0) but with the server side fix it can just change the test to `aResult.exception !== null`.
Component: Developer Tools: Scratchpad → Developer Tools: Console
Priority: -- → P3
what about |throw null;|? That's completely valid.
(In reply to Mano from comment #3)
> what about |throw null;|? That's completely valid.

`null` will be converted to `{ type: "null" }` when transported over the protocol, so it can't be confused with an actual `null` (missing value).
Mano, thanks for the bug report and for the analysis. Also thanks to Brandon.

This is a good first bug.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Whiteboard: [good first bug][lang=js][mentor=msucan]
Comment on attachment 816765 [details] [diff] [review]
Proposed patch to show the correct output for throw false/0/null/undefined;

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

Thanks for your patch, Dennis!

To write mochitest-browser-chrome tests see:

https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

Scratchpad tests live in browser/devtools/scratchpad/test. Copy one of the existing tests, then add it to browser.ini. You will need to rebuild Firefox, then you can run the scratchpad tests with mach.

Edit the new tests and perform throw 0; throw null... and check for every throw if you get the expected output.

In toolkit/devtools/webconsole/test we have mochitest-chrome tests for the webconsole actors. Copy one of the existing tests, then add the new one in chrome.ini. These tests do not check UI, they only test the actors.

Also see https://developer.mozilla.org/en-US/docs/Chrome_tests

Similar to the scratchpad test, in this mochitest-chrome you can send eval requests to the console actor, like throw 0 and check if the response packet is the one expected.

Please let me know if you have more questions. Thank you!


(more comments below)

::: browser/devtools/scratchpad/scratchpad.js
@@ +616,5 @@
> +          type == "-0") {
> +        deferred.resolve(type);
> +      }
> +      else if (type == "longString") {
> +        deferred.resolve(aError.initial + "...");

This needs to be an actual unicode ellipsis.

::: toolkit/devtools/server/actors/webconsole.js
@@ +558,5 @@
>      let evalInfo = this.evalWithDebugger(input, evalOptions);
>      let evalResult = evalInfo.result;
>      let helperResult = evalInfo.helperResult;
>  
> +    let result, error, errorMessage, exception = null;

Since |error| is no longer used outside of the if (evalResult) block, you can remove |error| from this line.

@@ +567,5 @@
>        else if ("yield" in evalResult) {
>          result = evalResult.yield;
>        }
>        else if ("throw" in evalResult) {
>          error = evalResult.throw;

Add a |let| here.

@@ +568,5 @@
>          result = evalResult.yield;
>        }
>        else if ("throw" in evalResult) {
>          error = evalResult.throw;
> +        exception = this.createValueGrip(error);

s/exception/errorGrip/ to avoid naming confusion. (error vs. exception)
Attachment #816765 - Flags: feedback?(mihai.sucan) → feedback+
Thanks for your feedback and your assistance, Mihai!

I addressed your input in the patch attached. This new patch also includes a browser-test and a chrome-test.
Attachment #816765 - Attachment is obsolete: true
Attachment #822937 - Flags: feedback?(mihai.sucan)
Comment on attachment 822937 [details] [diff] [review]
Proposed patch to show the correct output for throw false/0/null/undefined;

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

(when you have fix + tests you can ask for review)

Patch looks good. Giving r+ with the comments below addressed. Once you update the test, we can push to try servers, and if it is green, we can land in fx-team.

Thanks for your work!

::: browser/devtools/scratchpad/test/browser_scratchpad_throw_output.js
@@ +14,5 @@
> +    gBrowser.selectedBrowser.removeEventListener("load", onLoad, true);
> +    openScratchpad(testThrowOutput);
> +  }, true);
> +
> +  content.location = "data:text/html,<p>Test throw outputs in Scratchpad</p>";

Add ;charset=utf8 to avoid warning about missing charset.

@@ +32,5 @@
> +      label: "Correct exception message for '" + value + "' is shown"
> +    });
> +  });
> +
> +  let longString = Array(temp.DebuggerServer.LONG_STRING_LENGTH + 1).join('a'),

s/'/"/

(use double quotes for consistency)

::: toolkit/devtools/webconsole/test/test_throw.html
@@ +12,5 @@
> +<p>Web Console throw tests</p>
> +
> +<script class="testbody" type="text/javascript;version=1.8">
> +let temp = {};
> +Cu.import("resource://gre/modules/devtools/dbg-server.jsm", temp);

DebuggerServer should already be available from common.js.

@@ +29,5 @@
> +
> +  let falsyValues = ["-0", "null", "undefined", "Infinity", "-Infinity", "NaN"];
> +  falsyValues.forEach(function(value) {
> +    tests.push((function(value) {
> +      return function() {

This is slightly confusing. Why not just push the function? tests.push(function() { aState.client.evaluateJS.... }) ?

@@ +32,5 @@
> +    tests.push((function(value) {
> +      return function() {
> +        aState.client.evaluateJS("throw " + value + ";", function(aResponse) {
> +          let type = aResponse.exception.type;
> +          ok(type == value, "throw " + type);

is(type, value, "exception.type for throw " + value);

@@ +42,5 @@
> +
> +  let identityTestValues = [false, 0];
> +  identityTestValues.forEach(function(value) {
> +    tests.push((function(value) {
> +      return function() {

Same as above.

@@ +44,5 @@
> +  identityTestValues.forEach(function(value) {
> +    tests.push((function(value) {
> +      return function() {
> +        aState.client.evaluateJS(
> +          "throw " + value.toString() + ";",

toString() is not needed. toString() happens automatically

@@ +47,5 @@
> +        aState.client.evaluateJS(
> +          "throw " + value.toString() + ";",
> +          function(aResponse) {
> +            let exception = aResponse.exception;
> +            ok(exception === value, "throw " + exception);

ise(exception, value, "response.exception for throw " + value)

@@ +55,5 @@
> +      }
> +    })(value));
> +  });
> +
> +  let longString = Array(temp.DebuggerServer.LONG_STRING_LENGTH + 1).join('a'),

s/'/"/

@@ +61,5 @@
> +                        temp.DebuggerServer.LONG_STRING_INITIAL_LENGTH
> +                      );
> +  tests.push(function() {
> +    aState.client.evaluateJS("throw '" + longString + "';", function(aResponse) {
> +      ok(aResponse.exception.initial == shortedString, "throw longString");

is()
Attachment #822937 - Flags: feedback?(mihai.sucan) → review+
Thanks for your comments!

I addressed all your annotations in the patch attached.
Attachment #822937 - Attachment is obsolete: true
Dennis, thanks for the quick update.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=d580cd824445
Landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/ca8b56c6a6c1
Whiteboard: [good first bug][lang=js][mentor=msucan] → [good first bug][lang=js][mentor=msucan][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ca8b56c6a6c1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][mentor=msucan][fixed-in-fx-team] → [good first bug][lang=js][mentor=msucan]
Target Milestone: --- → Firefox 28
Depends on: 934852
Whiteboard: [good first bug][lang=js][mentor=msucan] → [good first bug][lang=js][mentor=msucan][good first verify]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: