Last Comment Bug 776685 - Azure canvas should throw the correct exception types
: Azure canvas should throw the correct exception types
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: 15 Branch
: x86_64 All
: -- normal (vote)
: mozilla17
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on: 764125
Blocks: 622842 779364
  Show dependency treegraph
 
Reported: 2012-07-23 13:25 PDT by Nick Cameron [:nrc]
Modified: 2012-08-01 19:40 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (14.32 KB, patch)
2012-07-26 19:45 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch (17.96 KB, patch)
2012-07-26 23:42 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch (17.96 KB, patch)
2012-07-26 23:49 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch (22.04 KB, patch)
2012-07-29 20:43 PDT, Nick Cameron [:nrc]
bzbarsky: review+
Details | Diff | Splinter Review
patch (22.88 KB, patch)
2012-07-31 20:59 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review

Description Nick Cameron [:nrc] 2012-07-23 13:25:38 PDT
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.
Comment 1 Nick Cameron [:nrc] 2012-07-23 13:27:00 PDT
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.)
Comment 2 Nick Cameron [:nrc] 2012-07-23 22:27:54 PDT
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)
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-07-24 02:46:22 PDT
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>.
Comment 4 Nick Cameron [:nrc] 2012-07-26 19:45:10 PDT
Created attachment 646453 [details] [diff] [review]
patch
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-26 21:48:04 PDT
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 Boris Zbarsky [:bz] 2012-07-26 21:53:27 PDT
Also, you can nix the "workers" arguments, right?
Comment 7 Nick Cameron [:nrc] 2012-07-26 23:42:21 PDT
Created attachment 646479 [details] [diff] [review]
patch

addressed comments from roc and bz
Comment 8 Boris Zbarsky [:bz] 2012-07-26 23:46:20 PDT
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.
Comment 9 Nick Cameron [:nrc] 2012-07-26 23:47:32 PDT
(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.
Comment 10 Nick Cameron [:nrc] 2012-07-26 23:49:59 PDT
Created attachment 646480 [details] [diff] [review]
patch

re-instated quotes in MSG_INVALID_ENUM_VALUE
Comment 11 Nick Cameron [:nrc] 2012-07-26 23:51:38 PDT
(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.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-27 14:25:58 PDT
(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?
Comment 13 Nick Cameron [:nrc] 2012-07-29 20:43:19 PDT
Created attachment 647052 [details] [diff] [review]
patch

Enumerated union options into the error message
Comment 14 Boris Zbarsky [:bz] 2012-07-31 08:52:32 PDT
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!
Comment 15 Nick Cameron [:nrc] 2012-07-31 20:59:18 PDT
Created attachment 647836 [details] [diff] [review]
patch

addressed comment; carrying r=bz
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-08-01 19:40:25 PDT
https://hg.mozilla.org/mozilla-central/rev/675d6bad2418

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