Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Azure canvas should throw the correct exception types

RESOLVED FIXED in mozilla17

Status

()

Core
Canvas: 2D
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

(Blocks: 1 bug)

15 Branch
mozilla17
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

Updated

5 years ago
Blocks: 622842
(Assignee)

Comment 2

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

Comment 4

5 years ago
Created attachment 646453 [details] [diff] [review]
patch
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.

Comment 6

5 years ago
Also, you can nix the "workers" arguments, right?
(Assignee)

Comment 7

5 years ago
Created attachment 646479 [details] [diff] [review]
patch

addressed comments from roc and bz
Attachment #646453 - Attachment is obsolete: true
Attachment #646453 - Flags: review?(Ms2ger)
Attachment #646479 - Flags: review?(Ms2ger)

Comment 8

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

Comment 9

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

Comment 10

5 years ago
Created attachment 646480 [details] [diff] [review]
patch

re-instated quotes in MSG_INVALID_ENUM_VALUE
Attachment #646479 - Attachment is obsolete: true
Attachment #646479 - Flags: review?(Ms2ger)
Attachment #646480 - Flags: review?(Ms2ger)
(Assignee)

Comment 11

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

Comment 13

5 years ago
Created attachment 647052 [details] [diff] [review]
patch

Enumerated union options into the error message
Attachment #646480 - Attachment is obsolete: true
Attachment #646480 - Flags: review?(Ms2ger)
Attachment #647052 - Flags: review?(Ms2ger)

Updated

5 years ago
Attachment #647052 - Flags: review?(bzbarsky)

Comment 14

5 years ago
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+

Updated

5 years ago
Blocks: 779364
(Assignee)

Comment 15

5 years ago
Created attachment 647836 [details] [diff] [review]
patch

addressed comment; carrying r=bz
Attachment #647052 - Attachment is obsolete: true
Attachment #647836 - Flags: review+
(Assignee)

Comment 16

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7198b3ea1763
https://hg.mozilla.org/mozilla-central/rev/675d6bad2418
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.