739659 - Uncaught DOMException is not clickable on Error/Web Console

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Masatoshi Kimura [:emk]]

js_ReportUncaughtException try to get filename and lineno from the exception only when the exception is Error object.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Try duck typing in js_ReportUncaughtException

DOMException duck quacks "filename", not "fileName". Weird...

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 609748 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException

The basic idea makes sense.  The patch is pretty gnarly-looking with its use of 'ok'; can you rewrite it to better structure the control flow?

Comment 3

[Luke Wagner [:luke]]

Blake: ignoring the stylistic nit in comment 2, is the patch safe from a DOM/security perspective?

Comment 4

[Masatoshi Kimura [:emk]]

I believe nothing will become less secure because:
1. We can already make any random strings clickable on Consoles.
   throw new Error("foo", "https://www.google.com/", 1)
2. We can already execute any JS codes in js_ReportUncaughtException.
   throw { toString: function(){alert("123");} };
   throw { toString: function(){throw "123";} };

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, I think Masatoshi is exactly right: this doesn't change any security properties.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Try duck typing in js_ReportUncaughtException, v2

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 610158 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, v2

Nuked |ok|.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Try duck typing in js_ReportUncaughtException, v2.1

The previous patch may not initialize |bytes| if the conversion failed.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 610162 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, v2.1

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

Great, much better.  Just some style nits:

::: js/src/jsexn.cpp
@@ +1188,5 @@
>  }
>  
>  JSBool
> +js_IsDuckTypedErrorObject(JSContext *cx, JSObject *exnObject,
> +                          const char **filename_strp)

This is file-local so you can make it 'static', strip the js_ prefix.  Also the return type can be 'bool'.

@@ +1199,5 @@
> +    if (!JS_HasProperty(cx, exnObject, filename_str, &found) || !found) {
> +        // DOMException duck quacks "filename" (all lowercase)
> +        filename_str = "filename";
> +        if (!JS_HasProperty(cx, exnObject, filename_str, &found) || !found) {
> +            return false;

No { } around 'return false'

@@ +1263,5 @@
> +         js_IsDuckTypedErrorObject(cx, exnObject, &filename_str))) {
> +
> +        JSString *tmp = NULL;
> +        if (JS_GetProperty(cx, exnObject, js_name_str, &roots[2]) &&
> +            JSVAL_IS_STRING(roots[2])) {

{ on new line to separate condition from body.

@@ +1277,5 @@
> +            str = JSVAL_TO_STRING(roots[3]);
> +            if (tmp)
> +                str = JS_ConcatStrings(cx, tmp, str);
> +            else
> +            if (bytesStorage.encode(cx, str))

else if (...) on single line

@@ +1291,3 @@
>          uint32_t lineno;
> +        if (!JS_GetProperty(cx, exnObject, js_lineNumber_str, &roots[5]) ||
> +            !ToUint32(cx, roots[5], &lineno)) {

{ on newline

Comment 10

[Luke Wagner [:luke]]

Also, just to be clear, this patch also changes the reported error string to be prefixed with the 'name' property of the exception object.  I assume this is fine but, since I have no idea about these things, I'll again ask Blake: is this ok?

Comment 11

[Masatoshi Kimura [:emk]]

Before the patch, the message was somthing like:
> uncaught exception: [Exception... "An attempt was made to use an object that is not, or is no longer, usable"  code: "11" nsresult: "0x8053000b (NS_ERROR_DOM_INVALID_STATE_ERR)"  location: "file:///J:/temp/domerror.html Line: 19"]

Now it will become:
> InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable

Rationale is as follows:
|code| is deprecated per DOM4 and is redundant to identify the error type.
|nsresult| is Gecko proprietary and is also redundant.
|filename| and |lineNumber| are no longer required in the message.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Try duck typing in js_ReportUncaughtException, for checkin. r=luke

Fixed style nits.

Comment 13

[Luke Wagner [:luke]]

(In reply to Masatoshi Kimura [:emk] from comment #11)
That looks way better :)

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 610196 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, for checkin. r=luke

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

Looks great, but I noticed two more tiny style nits.

::: js/src/jsexn.cpp
@@ +1188,5 @@
>  }
>  
> +static bool
> +IsDuckTypedErrorObject(JSContext *cx, JSObject *exnObject,
> +                       const char **filename_strp)

SM column width is 99, so all this can go on a single line.

@@ +1259,5 @@
>      JSAutoByteString filename;
> +    if (!reportp && exnObject &&
> +        (exnObject->isError() ||
> +         IsDuckTypedErrorObject(cx, exnObject, &filename_str))) {
> +

Sorry if there was ambiguity, I meant:

  if (!reportp && ...
      ...
      IsDuckTypedErrorObject(...))
  {
      JSString *tmp = NULL;

Comment 15

[Luke Wagner [:luke]]

(In reply to Luke Wagner [:luke] from comment #14)
> SM column width is 99, so all this can go on a single line.

Err, to be very clear: "static bool" is still on its own line, but the rest of it is on a single line :)

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: Try duck typing in js_ReportUncaughtException, for checkin. r=luke

Fixed nits again.

Comment 17

[Masatoshi Kimura [:emk]]

Comment on attachment 610214 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, for checkin. r=luke

Blake, what do you think about comment #10?

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Masatoshi Kimura [:emk] from comment #11)
> |nsresult| is Gecko proprietary and is also redundant.

My understanding is that the NS_ERROR_* error codes for DOM errors actually mirror what's in the spec, and the text of the error is "Gecko proprietary." Because of that, I'd prefer to keep the error code in the message.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 610214 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, for checkin. r=luke

f- for losing the error name. I'm not going to be in tomorrow, so assume an f+ for any patch with that added back it.

Comment 20

[Masatoshi Kimura [:emk]]

(In reply to Blake Kaplan (:mrbkap) from comment #19)
> f- for losing the error name. I'm not going to be in tomorrow, so assume an
> f+ for any patch with that added back it.
For DOMExceptions, "NS_ERROR_DOM_FOO_BAR_ERR" was renamed to "FooBarError" by bug 720208 (not this bug). In comment #11 example, "InvalidStateError" (formerly "NS_ERROR_DOM_INVALID_STATE_ERROR") is the error name. So the error name is not lost.

> uncaught exception: [Exception... "An attempt was made to use an object
> that is not, or is no longer, usable"  code: "11" nsresult: "0x8053000b
> (NS_ERROR_DOM_INVALID_STATE_ERR)"  location: "file:///J:/temp/domerror.html
> Line: 19"]
Sorry, this example was taken from Firefox 11. Actually, The current Nightly will display such as:
> uncaught exception: [Exception... "An attempt was made to use an object
> that is not, or is no longer, usable"  code: "11" nsresult: "0x8053000b
> (InvalidStateError)"  location: "file:///J:/temp/domerror.html Line: 19"]

Coud you reconsider based on these facts?

Comment 21

[Masatoshi Kimura [:emk]]

FYI, the string "InvalidStateError" is the normative part of DOM4 spec (it's the reason renaming NS_ERROR_DOM_XXX_ERR to XxxError).
http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#error-types-0

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 610214 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, for checkin. r=luke

Yeah, that sounds fine then.

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6cb9adc02c12

Comment 24

[Dão Gottwald [:dao]]

Backed out since this was causing crashes:

https://tbpl.mozilla.org/php/getParsedLog.php?id=10478271&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=10478280&tree=Mozilla-Inbound

and probably this as well:

https://tbpl.mozilla.org/php/getParsedLog.php?id=10478672&tree=Mozilla-Inbound

Comment 25

[Masatoshi Kimura [:emk]]

(In reply to Dão Gottwald [:dao] from comment #24)
> Backed out since this was causing crashes:
Ugh, bytesStorage.clear(); was accidentally removed.
> and probably this as well:
Hm, the error message change was observable from contents via onerror handlers...

Comment 26

[Masatoshi Kimura [:emk]]

Attachment: Bug 739659 - Try duck typing in js_ReportUncaughtException

Fixed test failures.
https://tbpl.mozilla.org/?tree=Try&rev=dbb38dceac6d

Comment 27

[Masatoshi Kimura [:emk]]

Attachment: Try duck typing in js_ReportUncaughtException, v4.1

Ah, the previous patch will not initialize bytes if str is null.

Comment 28

[Luke Wagner [:luke]]

Comment on attachment 611080 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, v4.1

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

::: js/src/jsexn.cpp
@@ +1197,5 @@
> +        return false;
> +
> +    const char *filename_str = *filename_strp;
> +    if (!JS_HasProperty(cx, exnObject, filename_str, &found) || !found) {
> +        // DOMException duck quacks "filename" (all lowercase)

/* */ for single-line comments

@@ +1274,5 @@
> +        if (name && msg) {
> +            JSString *tmp = JS_NewStringCopyZ(cx, ": ");
> +            if (tmp)
> +                name = JS_ConcatStrings(cx, name, tmp);
> +            str = name ? JS_ConcatStrings(cx, name, msg) : msg;

This control flow seems unnecessarily complex.  Can you just have a simple linear sequence like:

  JSString *colon = JS_NewStringCopyZ(cx, ": ");
  if (!colon)
      return false;
  JSString *nameColon = JS_ConcatStrings(cx, name, colon);
  if (!nameColon)
      return false;
  str = JS_ConcatStrings(cx, nameColon, msg);
  if (!str)
      return false;

@@ +1300,5 @@
>          report.filename = filename.ptr();
>          report.lineno = (unsigned) lineno;
> +        if (str) {
> +            JSFixedString *fixed = str->ensureFixed(cx);
> +            if (fixed)

Can you write "if (JSFixedString *fixed = str->ensureFixed(cx))" ?

Comment 29

[Masatoshi Kimura [:emk]]

Attachment: Try duck typing in js_ReportUncaughtException, v4.2; r=luke

Thanks. Resoved review comments.

Comment 30

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 611434
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=54432351263e
Try run started, revision 54432351263e. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=54432351263e

Comment 31

[Masatoshi Kimura [:emk]]

Cipc failed...
https://tbpl.mozilla.org/php/getParsedLog.php?id=10564868&tree=Try

Comment 32

[Masatoshi Kimura [:emk]]

Attachment: Try duck typing in js_ReportUncaughtException, v4.3

Sometimes v4.2 still fails to initialize |bytes|.

Comment 33

[Masatoshi Kimura [:emk]]

Interdiff:
https://bugzilla.mozilla.org/attachment.cgi?oldid=611434&action=interdiff&newid=611490&headers=1

Comment 34

[Luke Wagner [:luke]]

Comment on attachment 611490 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, v4.3

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

::: js/src/jsexn.cpp
@@ +1313,5 @@
> +    JSAutoByteString bytesStorage;
> +    if (str)
> +        bytesStorage.encode(cx, str);
> +    if (!(bytes = bytesStorage.ptr()))
> +        bytes = "unknown (can't convert to string)";

Ugh, sorry for not reviewing this more carefully.  The hoisted-decls-to-the-top style is a remnant from old C SpiderMonkey and we've been trying to push decls down to uses ever since switching to C++.  In that line, could you push the decl of 'bytes' down to here?  Also, we try to avoid treating assignments as boolean expressions, so could this be written:

  const char *bytes = NULL;
  if (str)
      bytes = bytesStorage.encode(cx, str);
  if (!bytes)
      bytes = "unknown (can't convert to string)";

Comment 35

[Mozilla RelEng Bot]

Try run for 54432351263e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=54432351263e
Results (out of 248 total builds):
    success: 207
    warnings: 39
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-54432351263e

Comment 36

[Masatoshi Kimura [:emk]]

Attachment: Try duck typing in js_ReportUncaughtException, v4.4

This patch looks OK (except known intermittent failures).
https://tbpl.mozilla.org/?tree=Try&rev=079d6ef32f1e

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/725a2ff9d08c

Comment 38

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/725a2ff9d08c