1173385 - Calling $$("") should throw an exception

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Brian Grinstead [:bgrins]]

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

Comment 1

[Florent Fayolle]

Attachment: 1173385.patch

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

Comment 2

[Brian Grinstead [:bgrins]]

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

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 4

[Florent Fayolle]

Attachment: 1173385.patch

Comment 5

[Florent Fayolle]

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

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ah, ok. My bad. Carry on with the last iteration.

Comment 7

[Florent Fayolle]

No problem, thanks for the answer!

Try push: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d398984dbc85

Florent

Comment 8

[Florent Fayolle]

Attachment: 1173385.patch

(forgot the updated patch)

Florent

Comment 9

[Florent Fayolle]

(real try-push link):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d398984dbc85

Florent

Comment 10

[Florent Fayolle]

Attachment: 1173385.patch

(Just an update to fix the summary)

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4a5c802618fe

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a5c802618fe