Closed Bug 1173385 Opened 5 years ago Closed 5 years ago

Calling $$("") should throw an exception

Categories

(DevTools :: Console, defect)

40 Branch
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: bgrins, Assigned: fayolle-florent)

References

Details

Attachments

(3 files, 3 obsolete files)

Attached image $$-qSA.png
There is a $$ command line helper that's a wrapper for document.querySelectorAll.  It seems to swallow errors though, we should just let any errors propagate to the console.

> $$("")
 undefined
> document.querySelectorAll("")
 SyntaxError: An invalid or illegal string was specified
Attached patch 1173385.patch (obsolete) — Splinter Review
That trick should work.

Quoting :fitzgen about the use of |error.unsafeDereference()|:
> at a glance, I think it would be safe, since there would still be 
> cross-compartment/xray wrappers and it is clear we are OK with potential side 
> effects in the debuggee

Florent
Attachment #8620510 - Flags: review?(bgrinstead)
Comment on attachment 8620510 [details] [diff] [review]
1173385.patch

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

Forwarding this to fitzgen, he's better qualified to review this change.  The changes to test_jsterm_queryselector.html look fine
Attachment #8620510 - Flags: review?(bgrinstead) → review?(nfitzgerald)
Assignee: nobody → fayolle-florent
Status: NEW → ASSIGNED
Comment on attachment 8620510 [details] [diff] [review]
1173385.patch

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

::: toolkit/devtools/server/actors/webconsole.js
@@ +819,5 @@
>          let error = evalResult.throw;
>          errorGrip = this.createValueGrip(error);
> +        errorMessage = error && (typeof error === "object" ) ?
> +          error.unsafeDereference().toString() :
> +          '' + error;

Let's do `DevToolsUtils.safeErrorString(error.unsafeDereference())` instead of `error.unsafeDereference().toString()`.

DevToolsUtils.safeErrorString: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/DevToolsUtils.js#16

Nitpick: also, please format multiline `a : b ? c` expressions like this:

  obj.prop = foo
    ? bar
    : baz;

Which is consistent with the way we do it elsewhere.
Attachment #8620510 - Flags: review?(nfitzgerald) → review+
Attached patch 1173385.patch (obsolete) — Splinter Review
Attachment #8620510 - Attachment is obsolete: true
Attached image screenshot2.png
I updated the patch (though not the tests yet). I attached how the exception is rendered with the patch. 

The use of |DevToolsUtils.safeErrorString| appends the callstack to the error string (so the internal one). Nick, are you sure we want to use this function? 

Florent
Flags: needinfo?(nfitzgerald)
Ah, ok. My bad. Carry on with the last iteration.
Flags: needinfo?(nfitzgerald)
Attached patch 1173385.patch (obsolete) — Splinter Review
(forgot the updated patch)

Florent
Attachment #8622040 - Attachment is obsolete: true
Attached patch 1173385.patchSplinter Review
(Just an update to fix the summary)
Attachment #8622583 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4a5c802618fe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.