Closed Bug 776685 Opened 8 years ago Closed 8 years ago

Azure canvas should throw the correct exception types

Categories

(Core :: Canvas: 2D, defect)

15 Branch
x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: nrc, Assigned: nrc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Splitting this off from bug 764125, because we can land without fixing this (Azure canvas would only be as broken as Thebes canvas was).

A whole bunch of canvas tests, when passed null, return an exception with message "Error: Exception thrown (nsresult = 0x80570009).", they should throw TypeError.
relevant comment from Ms2ger on bug 764125 (comment 54):

So, for exceptions, I think we want different messages for the two places where we call onFailure. The one in wrapObjectTemplate should say something about the value not being an object, and the one next to the xpc_qsUnwrapArg call something about it not implementing descriptor.interface.identifier. (The latter never deals with non-object values.)
Blocks: 622842
Ms2ger: you mentioned on irc that the code to set the name for an exception was well hidden, I'm afraid I still haven't found it, could you reveal the location please?

(I can set the message at the moment, but the message ends up as e.name and e.message gets the empty string)
I'm surprised to hear that; it seems to work fine at <http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1679>.

The TypeError name comes from the JSEXN_TYPEERR at <http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.cpp#17>.
Attached patch patch (obsolete) — Splinter Review
Attachment #646453 - Flags: review?(Ms2ger)
Comment on attachment 646453 [details] [diff] [review]
patch

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

::: dom/bindings/Codegen.py
@@ +1743,5 @@
>          throw = CGGeneric("if (failed) {\n"
>                            "  return false;\n"
>                            "}\n"
>                            "if (!done) {\n"
> +                          "  return ThrowErrorMessage(cx, MSG_BAD_ELEMENT);\n"

This surely isn't right for union types outside of canvas.

Maybe you could have MSG_NOT_MEMBER_OF_UNION_2 and MSG_NOT_MEMBER_OF_UNION_3 with messages "Value could not be converted to '{0}' or '{1}'." and "Value could not be converted to '{0}', '{1}' or '{2}'."

::: dom/bindings/Errors.msg
@@ +21,5 @@
>  
>  MSG_DEF(MSG_INVALID_ENUM_VALUE, 2, "Value '{0}' is not a valid value for enumeration '{1}'.")
>  MSG_DEF(MSG_MISSING_ARGUMENTS, 1, "Not enough arguments to {0}.")
> +MSG_DEF(MSG_NOT_OBJECT, 0, "value not an object")
> +MSG_DEF(MSG_TYPE_ERR, 1, "object does not implement interface '{0}'")

Presumably these should start with a capital letter and end with a full stop?

Maybe MSG_TYPE_ERR should be MSG_DOES_NOT_IMPLEMENT_INTERFACE?

Also need to decide on whether identifiers should be surrounded by single quotes. I would suggest not.
Also, you can nix the "workers" arguments, right?
Attached patch patch (obsolete) — Splinter Review
addressed comments from roc and bz
Attachment #646453 - Attachment is obsolete: true
Attachment #646453 - Flags: review?(Ms2ger)
Attachment #646479 - Flags: review?(Ms2ger)
Comment on attachment 646479 [details] [diff] [review]
patch

In MSG_INVALID_ENUM_VALUE, {0} is actually a string, and probably _should_ be quoted....

Ms2ger might be out for a few days; if he doesn't get to this before I do, I'll try to just review this.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 646453 [details] [diff] [review]
> patch
> 
> Review of attachment 646453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/Codegen.py
> @@ +1743,5 @@
> >          throw = CGGeneric("if (failed) {\n"
> >                            "  return false;\n"
> >                            "}\n"
> >                            "if (!done) {\n"
> > +                          "  return ThrowErrorMessage(cx, MSG_BAD_ELEMENT);\n"
> 
> This surely isn't right for union types outside of canvas.
> 

No, it is much more generic than I thought.

> Maybe you could have MSG_NOT_MEMBER_OF_UNION_2 and MSG_NOT_MEMBER_OF_UNION_3
> with messages "Value could not be converted to '{0}' or '{1}'." and "Value
> could not be converted to '{0}', '{1}' or '{2}'."

This is hard because there can be any number of possible members and they can come from lots of places. It's possible, but I figure not worth the effort for an error message. I changed the message to something generic, rather than mentioning the element types.
Attached patch patch (obsolete) — Splinter Review
re-instated quotes in MSG_INVALID_ENUM_VALUE
Attachment #646479 - Attachment is obsolete: true
Attachment #646479 - Flags: review?(Ms2ger)
Attachment #646480 - Flags: review?(Ms2ger)
(In reply to Boris Zbarsky (:bz) from comment #8)
> 
> Ms2ger might be out for a few days; if he doesn't get to this before I do,
> I'll try to just review this.

Cool, thanks bz.
(In reply to Nick Cameron [:nrc] from comment #9)
> This is hard because there can be any number of possible members and they
> can come from lots of places. It's possible, but I figure not worth the
> effort for an error message. I changed the message to something generic,
> rather than mentioning the element types.

In that case maybe you can just print the serialization of the union type?
Attached patch patch (obsolete) — Splinter Review
Enumerated union options into the error message
Attachment #646480 - Attachment is obsolete: true
Attachment #646480 - Flags: review?(Ms2ger)
Attachment #647052 - Flags: review?(Ms2ger)
Attachment #647052 - Flags: review?(bzbarsky)
Comment on attachment 647052 [details] [diff] [review]
patch

>+++ b/content/canvas/test/test_canvas.html
>@@ -1932,35 +1940,34 @@ for (uint32_t i = 0; i < length; ++i) {
>             "if (!JS_Is%s(&${val}.toObject(), cx)) {\n"
>-            "%s" # No newline here because onFailure() handles that
>+            "%s" # No newline here because onFailureNotAnObject() handles that
>             "}\n"
>             "%s.%s(cx, &${val}.toObject());\n" %
>-            (jsname, CGIndenter(onFailure(failureCode, descriptorProvider.workers)).define(),
>+            (jsname, CGIndenter(onFailureNotAnObject(failureCode)).define(),

This should use onFailureBadType(failureCode, type.name), because this is failing out cases when the object is not the right kind of typed array, not cases when the value is not an object at all.

r=me with that change.  Thanks for the patch!
Attachment #647052 - Flags: review?(bzbarsky)
Attachment #647052 - Flags: review?(Ms2ger)
Attachment #647052 - Flags: review+
Attached patch patchSplinter Review
addressed comment; carrying r=bz
Attachment #647052 - Attachment is obsolete: true
Attachment #647836 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/675d6bad2418
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.