Last Comment Bug 739659 - Uncaught DOMException is not clickable on Error/Web Console
: Uncaught DOMException is not clickable on Error/Web Console
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
javascript:(function(x){x=XMLHttpRequ...
Depends on: 743049 812747 878543
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-27 09:22 PDT by Masatoshi Kimura [:emk]
Modified: 2013-07-11 14:00 PDT (History)
4 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Try duck typing in js_ReportUncaughtException (2.93 KB, patch)
2012-03-27 09:27 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Try duck typing in js_ReportUncaughtException, v2 (4.82 KB, patch)
2012-03-28 09:04 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Try duck typing in js_ReportUncaughtException, v2.1 (4.98 KB, patch)
2012-03-28 09:13 PDT, Masatoshi Kimura [:emk]
luke: review+
Details | Diff | Splinter Review
Try duck typing in js_ReportUncaughtException, for checkin. r=luke (4.97 KB, patch)
2012-03-28 10:39 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Try duck typing in js_ReportUncaughtException, for checkin. r=luke (4.95 KB, patch)
2012-03-28 11:20 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
mrbkap: feedback+
Details | Diff | Splinter Review
Bug 739659 - Try duck typing in js_ReportUncaughtException (7.53 KB, patch)
2012-03-30 16:29 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Try duck typing in js_ReportUncaughtException, v4.1 (7.52 KB, patch)
2012-03-30 16:35 PDT, Masatoshi Kimura [:emk]
luke: review+
Details | Diff | Splinter Review
Try duck typing in js_ReportUncaughtException, v4.2; r=luke (7.65 KB, patch)
2012-04-02 06:24 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Try duck typing in js_ReportUncaughtException, v4.3 (7.67 KB, patch)
2012-04-02 09:45 PDT, Masatoshi Kimura [:emk]
luke: review+
Details | Diff | Splinter Review
Try duck typing in js_ReportUncaughtException, v4.4 (7.73 KB, patch)
2012-04-02 15:55 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2012-03-27 09:22:18 PDT
js_ReportUncaughtException try to get filename and lineno from the exception only when the exception is Error object.
Comment 1 Masatoshi Kimura [:emk] 2012-03-27 09:27:15 PDT
Created attachment 609748 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException

DOMException duck quacks "filename", not "fileName". Weird...
Comment 2 Luke Wagner [:luke] 2012-03-27 10:00:32 PDT
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] 2012-03-27 10:01:37 PDT
Blake: ignoring the stylistic nit in comment 2, is the patch safe from a DOM/security perspective?
Comment 4 Masatoshi Kimura [:emk] 2012-03-27 17:35:28 PDT
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) 2012-03-28 08:02:06 PDT
Yeah, I think Masatoshi is exactly right: this doesn't change any security properties.
Comment 6 Masatoshi Kimura [:emk] 2012-03-28 09:04:53 PDT
Created attachment 610158 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, v2
Comment 7 Masatoshi Kimura [:emk] 2012-03-28 09:05:27 PDT
Comment on attachment 610158 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, v2

Nuked |ok|.
Comment 8 Masatoshi Kimura [:emk] 2012-03-28 09:13:08 PDT
Created attachment 610162 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, v2.1

The previous patch may not initialize |bytes| if the conversion failed.
Comment 9 Luke Wagner [:luke] 2012-03-28 10:05:30 PDT
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] 2012-03-28 10:07:45 PDT
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] 2012-03-28 10:38:02 PDT
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] 2012-03-28 10:39:48 PDT
Created attachment 610196 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, for checkin. r=luke

Fixed style nits.
Comment 13 Luke Wagner [:luke] 2012-03-28 11:04:19 PDT
(In reply to Masatoshi Kimura [:emk] from comment #11)
That looks way better :)
Comment 14 Luke Wagner [:luke] 2012-03-28 11:07:03 PDT
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] 2012-03-28 11:07:54 PDT
(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] 2012-03-28 11:20:53 PDT
Created attachment 610214 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, for checkin. r=luke

Fixed nits again.
Comment 17 Masatoshi Kimura [:emk] 2012-03-29 00:32:28 PDT
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) 2012-03-29 08:49:56 PDT
(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) 2012-03-29 08:56:45 PDT
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] 2012-03-29 09:11:13 PDT
(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] 2012-03-29 09:17:02 PDT
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) 2012-03-29 09:17:37 PDT
Comment on attachment 610214 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, for checkin. r=luke

Yeah, that sounds fine then.
Comment 25 Masatoshi Kimura [:emk] 2012-03-29 18:37:02 PDT
(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] 2012-03-30 16:29:41 PDT
Created attachment 611077 [details] [diff] [review]
Bug 739659 - Try duck typing in js_ReportUncaughtException

Fixed test failures.
https://tbpl.mozilla.org/?tree=Try&rev=dbb38dceac6d
Comment 27 Masatoshi Kimura [:emk] 2012-03-30 16:35:05 PDT
Created attachment 611080 [details] [diff] [review]
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] 2012-04-01 22:21:49 PDT
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] 2012-04-02 06:24:53 PDT
Created attachment 611434 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, v4.2; r=luke

Thanks. Resoved review comments.
Comment 30 Mozilla RelEng Bot 2012-04-02 06:30:01 PDT
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] 2012-04-02 09:03:46 PDT
Cipc failed...
https://tbpl.mozilla.org/php/getParsedLog.php?id=10564868&tree=Try
Comment 32 Masatoshi Kimura [:emk] 2012-04-02 09:45:32 PDT
Created attachment 611490 [details] [diff] [review]
Try duck typing in js_ReportUncaughtException, v4.3

Sometimes v4.2 still fails to initialize |bytes|.
Comment 34 Luke Wagner [:luke] 2012-04-02 09:53:49 PDT
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 2012-04-02 11:31:47 PDT
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
Comment 36 Masatoshi Kimura [:emk] 2012-04-02 15:55:51 PDT
Created attachment 611633 [details] [diff] [review]
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] 2012-04-03 17:13:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/725a2ff9d08c
Comment 38 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-04 05:06:47 PDT
https://hg.mozilla.org/mozilla-central/rev/725a2ff9d08c

Note You need to log in before you can comment on or make changes to this bug.