Closed Bug 739659 Opened 8 years ago Closed 8 years ago

Uncaught DOMException is not clickable on Error/Web Console

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: emk, Assigned: emk)

References

()

Details

Attachments

(1 file, 9 obsolete files)

7.73 KB, patch
emk
: review+
Details | Diff | Splinter Review
js_ReportUncaughtException try to get filename and lineno from the exception only when the exception is Error object.
DOMException duck quacks "filename", not "fileName". Weird...
Attachment #609748 - Flags: review?(luke)
Summary: DOMException is not clickable on Error/Web Console → Uncaught DOMException is not clickable on Error/Web Console
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?
Attachment #609748 - Flags: review?(luke)
Blake: ignoring the stylistic nit in comment 2, is the patch safe from a DOM/security perspective?
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";} };
Yeah, I think Masatoshi is exactly right: this doesn't change any security properties.
Attachment #609748 - Attachment is obsolete: true
Attachment #610158 - Flags: review?(luke)
Comment on attachment 610158 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, v2

Nuked |ok|.
The previous patch may not initialize |bytes| if the conversion failed.
Attachment #610158 - Attachment is obsolete: true
Attachment #610158 - Flags: review?(luke)
Attachment #610162 - Flags: review?(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
Attachment #610162 - Flags: review?(luke) → review+
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?
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.
Fixed style nits.
Attachment #610162 - Attachment is obsolete: true
Attachment #610196 - Flags: review+
(In reply to Masatoshi Kimura [:emk] from comment #11)
That looks way better :)
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;
(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 :)
Fixed nits again.
Attachment #610196 - Attachment is obsolete: true
Attachment #610214 - Flags: review+
Attachment #610214 - Attachment is patch: true
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?
Attachment #610214 - Flags: feedback?(mrbkap)
(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 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.
Attachment #610214 - Flags: feedback?(mrbkap) → feedback-
(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?
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 on attachment 610214 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, for checkin. r=luke

Yeah, that sounds fine then.
Attachment #610214 - Flags: feedback- → feedback+
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/6cb9adc02c12
Assignee: general → VYV03354
Keywords: checkin-needed
Target Milestone: --- → mozilla14
(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...
Fixed test failures.
https://tbpl.mozilla.org/?tree=Try&rev=dbb38dceac6d
Attachment #610214 - Attachment is obsolete: true
Attachment #611077 - Flags: review?(luke)
Ah, the previous patch will not initialize bytes if str is null.
Attachment #611077 - Attachment is obsolete: true
Attachment #611077 - Flags: review?(luke)
Attachment #611080 - Flags: review?(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))" ?
Attachment #611080 - Flags: review?(luke) → review+
Thanks. Resoved review comments.
Attachment #611080 - Attachment is obsolete: true
Attachment #611434 - Flags: review+
Whiteboard: [autoland-try:-b do -p all -u all -t none]
Whiteboard: [autoland-try:-b do -p all -u all -t none] → [autoland-in-queue]
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
Sometimes v4.2 still fails to initialize |bytes|.
Attachment #611434 - Attachment is obsolete: true
Attachment #611490 - Flags: review?(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)";
Attachment #611490 - Flags: review?(luke) → review+
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/autolanduser@mozilla.com-54432351263e
Whiteboard: [autoland-in-queue]
This patch looks OK (except known intermittent failures).
https://tbpl.mozilla.org/?tree=Try&rev=079d6ef32f1e
Attachment #611490 - Attachment is obsolete: true
Attachment #611633 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/725a2ff9d08c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 743049
You need to log in before you can comment on or make changes to this bug.