The default bug view has changed. See this FAQ.

Throw TypeError for binding exceptions

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Looking into this
Assignee: nobody → Ms2ger
(Assignee)

Comment 2

5 years ago
Created attachment 636176 [details] [diff] [review]
Part a: Introduce JS_ReportErrorNumberVA

Yay, C++
(Assignee)

Comment 3

5 years ago
Created attachment 636177 [details] [diff] [review]
Part b: Throw some TypeErrors
Attachment #636177 - Flags: review?(khuey)
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.
Attachment #636177 - Flags: review?(khuey)
(Assignee)

Updated

5 years ago
Attachment #636176 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

5 years ago
Created attachment 638663 [details] [diff] [review]
Part b: Add an exnType to JSErrorReport
Attachment #638663 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

5 years ago
Created attachment 638664 [details] [diff] [review]
Part c: Throw some better TypeErrors
Attachment #636177 - Attachment is obsolete: true
Attachment #638664 - Flags: review?(khuey)
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.
Attachment #638664 - Flags: review?(khuey) → review+
(Assignee)

Comment 8

5 years ago
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.
https://mxr.mozilla.org/mozilla-central/source/js/src/jsexn.cpp?rev=13a8fa3afd28#1108
|report.exnType = JSEXN_NONE;| is needed here, I think.
(Assignee)

Comment 10

5 years ago
(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 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.
Attachment #636176 - Flags: review?(jwalden+bmo) → review+
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
Attachment #638663 - Flags: review?(jwalden+bmo) → review+
(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.
(Assignee)

Comment 14

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 774705
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/mozilla-central/rev/ba8f96c49185
https://hg.mozilla.org/mozilla-central/rev/811a6217b1b9
https://hg.mozilla.org/mozilla-central/rev/1ed8454e7090
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 776861
No longer depends on: 776861
Duplicate of this bug: 743887
You need to log in before you can comment on or make changes to this bug.