Last Comment Bug 765464 - Throw TypeError for binding exceptions
: Throw TypeError for binding exceptions
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
: 743887 (view as bug list)
Depends on:
Blocks: ParisBindings 774705
  Show dependency treegraph
 
Reported: 2012-06-16 01:17 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-09-07 04:33 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part a: Introduce JS_ReportErrorNumberVA (1.91 KB, patch)
2012-06-24 12:12 PDT, :Ms2ger (⌚ UTC+1/+2)
jwalden+bmo: review+
Details | Diff | Splinter Review
Part b: Throw some TypeErrors (8.89 KB, patch)
2012-06-24 12:13 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Part b: Add an exnType to JSErrorReport (5.39 KB, patch)
2012-07-03 05:26 PDT, :Ms2ger (⌚ UTC+1/+2)
jwalden+bmo: review+
Details | Diff | Splinter Review
Part c: Throw some better TypeErrors (12.99 KB, patch)
2012-07-03 05:27 PDT, :Ms2ger (⌚ UTC+1/+2)
khuey: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-06-16 01:17:56 PDT

    
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-06-18 12:19:41 PDT
Looking into this
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-06-24 12:12:18 PDT
Created attachment 636176 [details] [diff] [review]
Part a: Introduce JS_ReportErrorNumberVA

Yay, C++
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-06-24 12:13:01 PDT
Created attachment 636177 [details] [diff] [review]
Part b: Throw some TypeErrors
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-07-02 09:34:18 PDT
Comment on attachment 636177 [details] [diff] [review]
Part b: Throw some TypeErrors

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

This is good, but I think it can be better.

::: dom/bindings/Errors.msg
@@ +19,5 @@
> + * be replaced with a string value when the error is reported.
> + */
> +
> +MSG_DEF(MSG_INVALID_ENUM_VALUE, 0, "Value is not a valid value of the expected enumeration.")
> +MSG_DEF(MSG_MISSING_ARGUMENTS, 0, "Not enough arguments.")

I think that these should take some arguments.

For MSG_INVALID_ENUM_VALUE we should report the value passed, and the enumeration type name if possible.
For MSG_MISSING_ARGUMENTS we should report the (method, interface) pair invoked.

So these would say something like:

Value 'foo' is not a valid value for enumeration 'Bar'.
Not enough arguments to XMLHttpRequest.setRequestHeader.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-07-03 05:26:39 PDT
Created attachment 638663 [details] [diff] [review]
Part b: Add an exnType to JSErrorReport
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-07-03 05:27:34 PDT
Created attachment 638664 [details] [diff] [review]
Part c: Throw some better TypeErrors
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-07-03 09:25:10 PDT
Comment on attachment 638664 [details] [diff] [review]
Part c: Throw some better TypeErrors

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

::: dom/bindings/BindingUtils.h
@@ +36,5 @@
> +#undef MSG_DEF
> +  Err_Limit
> +};
> +
> +extern bool

Why extern?

@@ +437,5 @@
> +
> +  NS_LossyConvertUTF16toASCII deflated(static_cast<const PRUnichar*>(chars),
> +                                       length);
> +  return ThrowErrorMessage(cx, MSG_INVALID_ENUM_VALUE, deflated.get(), type);
> +}

Write this as two separate template specializations and avoid the if.
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-07-03 12:24:58 PDT
Comment on attachment 638663 [details] [diff] [review]
Part b: Add an exnType to JSErrorReport

This causes test failures in dom/tests/mochitest/bugs/test_onerror_message.html; see <https://tbpl.mozilla.org/?tree=Try&rev=48d631151de1>. I tried to investigate, but gdb hates me.
Comment 9 Masatoshi Kimura [:emk] 2012-07-03 18:33:16 PDT
https://mxr.mozilla.org/mozilla-central/source/js/src/jsexn.cpp?rev=13a8fa3afd28#1108
|report.exnType = JSEXN_NONE;| is needed here, I think.
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-07-04 04:26:52 PDT
(In reply to Masatoshi Kimura [:emk] from comment #9)
> https://mxr.mozilla.org/mozilla-central/source/js/src/jsexn.
> cpp?rev=13a8fa3afd28#1108
> |report.exnType = JSEXN_NONE;| is needed here, I think.

Yep, that fixed it. Thanks!
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-12 18:44:27 PDT
Comment on attachment 636176 [details] [diff] [review]
Part a: Introduce JS_ReportErrorNumberVA

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

(The r+ is intentional, notwithstanding my comment below.)

::: js/src/jsapi.h
@@ +5402,5 @@
> +#ifdef va_start
> +extern JS_PUBLIC_API(void)
> +JS_ReportErrorNumberVA(JSContext *cx, JSErrorCallback errorCallback,
> +                       void *userRef, const unsigned errorNumber, va_list ap);
> +#endif

Ugh I hate our error-reporting APIs so much.
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-12 18:48:34 PDT
Comment on attachment 638663 [details] [diff] [review]
Part b: Add an exnType to JSErrorReport

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

::: js/src/jsfriendapi.h
@@ +781,5 @@
>  /* Implemented in jsexn.cpp. */
>  
>  /*
> + * Get an error type name from a JSExnType constant.
> + * Retursn NULL for invalid arguments and JSEXN_INTERNALERR

ptyo
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-12 18:55:20 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> So these would say something like:
> 
> Value 'foo' is not a valid value for enumeration 'Bar'.
> Not enough arguments to XMLHttpRequest.setRequestHeader.

Not to bikeshed too much, but.  This should either be XMLHttpRequest.prototype.setRequestHeader, if we think developers understand the JS prototyping mechanism.  Or it should be something like "...to the setRequestHeader method on XMLHttpRequest objects, or somesuch.  The problem is XMLHttpRequest.setRequestHeader implies a "class-static" method, not a method "on" instances.  We should be careful not to muddy the waters here, in case a method exists both class-statically and on instances.
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2012-07-13 02:06:51 PDT
(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #13)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> > So these would say something like:
> > 
> > Value 'foo' is not a valid value for enumeration 'Bar'.
> > Not enough arguments to XMLHttpRequest.setRequestHeader.
> 
> Not to bikeshed too much, but.  This should either be
> XMLHttpRequest.prototype.setRequestHeader, if we think developers understand
> the JS prototyping mechanism.  Or it should be something like "...to the
> setRequestHeader method on XMLHttpRequest objects, or somesuch.  The problem
> is XMLHttpRequest.setRequestHeader implies a "class-static" method, not a
> method "on" instances.  We should be careful not to muddy the waters here,
> in case a method exists both class-statically and on instances.

FWIW, we already use strings like XMLHttpRequest.setRequestHeader in quickstub exceptions.
Comment 16 Masatoshi Kimura [:emk] 2012-09-07 04:33:36 PDT
*** Bug 743887 has been marked as a duplicate of this bug. ***

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