Closed Bug 742191 Opened 12 years ago Closed 12 years ago

Fix Paris binding exceptions to be sane

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: bzbarsky, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

We need a non-XPConnect mechanism for creating exception objects.  These should, conceivably, also be new-binding objects.  That would certainly make it easier to use them on both workers and main-thread.

While we do this, we should somewhat audit the places we throw; I think we're inconsistent in terms of when we include the interface+propname and when we do not.  Ideally we would always include them somehow.
Blocks: 743887
Blocks: 744910
Attached patch wip (obsolete) — Splinter Review
I'm not really hwo to test any of this.  Partially since I've never figured out how to use the error console with a screen reader.
>-        return CGGeneric('return ThrowMethodFailedWithDetails<%s>(cx, rv, "%s", "%s");'
>-                         % (toStringBool(not self.descriptor.workers),
>-                            self.descriptor.interface.identifier.name,
>-                            self.idlNode.identifier.name))
>+        return CGGeneric('return ThrowErrorMessage(cx, MSG_METHOD_FAILED,  "%s", "%s");'
>+                         % (self.idlNode.identifier.name,
>+                            self.descriptor.interface.identifier.name))
This is wrong. The binding need to propagate the error from the method. That is, a new mechanism is required which create a WebIDL binding object representing a DOMException from the ErrorResult.
Comment on attachment 659075 [details] [diff] [review]
wip

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

::: dom/bindings/Codegen.py
@@ +715,5 @@
>      nsresult rv;
>      JS::Value val = OBJECT_TO_JSVAL(obj);
>      rv = xpc_qsUnwrapArg<nsISupports>(cx, val, &global, &globalRef.ptr, &val);
>      if (NS_FAILED(rv)) {
> +      return ThrowErrorMessage(cx, MSG_NOT_OBJECT);

The value *is* an object here; not sure in which case it would be hit exactly, but something like "global is not a native object" sounds closer to the truth.

@@ +1508,5 @@
>      As CastableObjectUnwrapper, but defaulting to throwing if unwrapping fails
>      """
>      def __init__(self, descriptor, source, target):
>          CastableObjectUnwrapper.__init__(self, descriptor, source, target,
> +            "return ThrowErrorMessage(cx, MSG_NOT_OBJECT);")

I think DOES_NOT_IMPLEMENT_INTERFACE is more correct here.

@@ +1519,5 @@
>      |target| is an nsCOMPtr of the appropriate type in which we store the result.
>      """
>      def __init__(self, descriptor, source, target, codeOnFailure=None):
>          if codeOnFailure is None:
> +            codeOnFailure = ("return ThrowErrorMessage(cx, MSG_NOT_OBJECT);")

And here

@@ +1738,5 @@
>    return false;
>  }
>  Sequence< %s > &arr = const_cast< Sequence< %s >& >(%s);
>  if (!arr.SetCapacity(length)) {
> +  return ThrowErrorMessage(cx, MSG_OUT_OF_MEMORY);

JS_ReportOutOfMemory(cx);
return false;

I think; bz probably knows better.

@@ +2986,5 @@
>  
>      def getErrorReport(self):
> +        return CGGeneric('return ThrowErrorMessage(cx, MSG_METHOD_FAILED,  "%s", "%s");'
> +                         % (self.idlNode.identifier.name,
> +                            self.descriptor.interface.identifier.name))

This change is wrong; exceptions from the native implementation should not turn into TypeErrors.

@@ +3247,5 @@
>                  pass
>              else:
>                  # Just throw; we have no idea what we're supposed to
>                  # do with this.
> +                caseBody.append(CGGeneric("return ThrowErrorMessage(cx, MSG_NOT_OBJECT);"))

Ask bz if this is right, please.
> something like "global is not a native object" sounds closer to the truth.

Yes, agreed.  Note that you can add new message strings to dom/bindings/Errors.msg.

I'm not sure how to write a test for this one offhand...

> I think DOES_NOT_IMPLEMENT_INTERFACE is more correct here.

Yes.

This can be tested by doing something like this:

  var xhr = new XMLHttpRequest();
  try {
    xhr.open.call({});
    ok(false, "Calling open() with bogus this object should throw");
  } catch (e) {
    // Interrogate the exception 
  }

In fact, we have some code that looks like that in dom/imptests/idlharness.js, but it's only in IdlException.prototype.test_members.  We should add it to IDLInterface.prototype.test_members.  And ideally get that change pushed upstream.  Aryeh, how do we do that last?

> And here

Indeed.  But I don't actually know how to exercise this codepath yet, so not sure how to write a test for it.

> +  return ThrowErrorMessage(cx, MSG_DOES_NOT_IMPLEMENT_INTERFACE, "array");

Hmm.  It might be better to add a new message for "Object cannot be converted to a sequence" or something, because this condition isn't quite like checking for an "array" interface.

> JS_ReportOutOfMemory(cx);

Hmm.  The question is whether we want a catchable exception here or not.  I'm not quite sure.  :(  Peter, thoughts?

>+                caseBody.append(CGGeneric("return ThrowErrorMessage(cx, MSG_NOT_OBJECT);"))

That's not quite the right error message.  It could be an object, but not one that does anything useful in our overload set.

What's probably ideal here is a new message along the lines of "Argument {0} not valid for any of the {1}-argument overloads" and passing a string version of distinguishingIndex for {0} and a string version of argCount for {1}.

This should be testable with something like this:

  var ctx = document.createElement("canvas").getContext("2d");
  try {
    ctx.drawImage({}, 0, 0);
  } catch(e) {
    // interrogate the exception
  }

which should hit this case.
Attached patch A test for this-unwrapping (obsolete) — Splinter Review
This pulls in <http://dvcs.w3.org/hg/resources/rev/6a164969de8d>; you can just remove the "XMLHttpRequest interface: attribute *" parts of test_interfaces.html.json again in your patch.

Please don't touch idlharness.js or the imported tests without sending me (or someone else with push access, like Aryeh) an upstream patch, though; that would hurt me when I next pull in an update.
(In reply to Boris Zbarsky (:bz) from comment #4)
> Yes.
> 
> This can be tested by doing something like this:
> 
>   var xhr = new XMLHttpRequest();
>   try {
>     xhr.open.call({});
>     ok(false, "Calling open() with bogus this object should throw");
>   } catch (e) {
>     // Interrogate the exception 
>   }
> 
> In fact, we have some code that looks like that in
> dom/imptests/idlharness.js, but it's only in
> IdlException.prototype.test_members.  We should add it to
> IDLInterface.prototype.test_members.  And ideally get that change pushed
> upstream.  Aryeh, how do we do that last?

IdlException.prototype.test_members only tests the getters for fields, not operations, right?  Also, test_members doesn't test anything on actual objects that implement the interface, so if the test is there it would have to be

  XMLHttpRequest.prototype.open.call({});

Which probably makes at least as much sense, right?  We'd want to add some dummy arguments, too, so that it doesn't throw due to too few arguments.  We already have logic in the harness to do this, because we already test that the right exception is thrown for too few arguments.

This is in a W3C repository, so anyone with W3C DVCS access should be able to change it.  I wrote it originally, and Ms2ger has made changes to it before, so both of us are probably good candidates for adding new tests to it.  Ms2ger, do you want to do this, or should I?
> IdlException.prototype.test_members only tests the getters for fields, not operations,
> right? 

Yes.  We want both for XHR.  Ms2ger's thing adds attributes, but not operations.

> Which probably makes at least as much sense, right?

Yes, indeed.

> We'd want to add some dummy arguments, too, so that it doesn't throw due to too few
> arguments.

The check for correct this is done before the argument count check for operations.  See http://dev.w3.org/2006/webapi/WebIDL/#es-operations step 2.  The argcount check is done in step 4.  So you can not add dummy arguments, and if the impl throws about the argcount being wrong the impl is buggy.
(In reply to Boris Zbarsky (:bz) from comment #7)
> The check for correct this is done before the argument count check for
> operations.  See http://dev.w3.org/2006/webapi/WebIDL/#es-operations step 2.
> The argcount check is done in step 4.  So you can not add dummy arguments,
> and if the impl throws about the argcount being wrong the impl is buggy.

But both throw a TypeError.  I don't want an implementation to spuriously pass because it throws a TypeError for too few arguments, rather than for not implementing the interface.  In practice, this probably won't be a problem as long as we're testing at least one method that takes no arguments, but it's easy enough to come up with dummy arguments in most cases, so we may as well do so.

It would be nice if WebIDL threw different types of exceptions instead of just a TypeError for everything.
> But both throw a TypeError.

Oh, hmm.  And you can't rely on the info the error carries with it.  Ok, yes.

Ideally, in fact, you would pick a method that doesn't throw when called on an object but does throw when called the same way on {}.  Not sure how you'd do that....
Not sure what you mean.  I'd make it so that for every operation in every tested IDL, it would try calling

  InterfaceName.prototype.methodName.call({});

with some suitable-looking arguments, and make sure it threw a TypeError.  It would never try calling the method with suitable-looking arguments on a suitable object, because then the method call should succeed, which might have unknown side-effects.  It will only check that it fails in all the cases it should fail in.
> with some suitable-looking arguments

That's the hard part.  How would you do the argument selection?
It doesn't have to be perfect.  In the worst case, the argument isn't actually of the right type and so we get a spurious pass due to a TypeError being thrown for the wrong reason.  That's not ideal, but also no big deal -- it just means that specific method doesn't actually have coverage for this.  The current code for this is:

http://hg.mozilla.org/mozilla-central/file/5faccd0b7618/dom/imptests/idlharness.js#l1471

which doesn't even handle interface types other than Node.  Obviously, it's only suitable for testing cases where a TypeError is expected anyway.
Updated upstream: http://dvcs.w3.org/hg/resources/rev/953e1f74ed0d

It should be pulled the next time we sync.  Tell me if you want any other idlharness.js changes.
Blocks: 791537
> >+                caseBody.append(CGGeneric("return ThrowErrorMessage(cx, MSG_NOT_OBJECT);"))
> 
> That's not quite the right error message.  It could be an object, but not
> one that does anything useful in our overload set.
> 
> What's probably ideal here is a new message along the lines of "Argument {0}
> not valid for any of the {1}-argument overloads" and passing a string
> version of distinguishingIndex for {0} and a string version of argCount for
> {1}.
> 
> This should be testable with something like this:
> 
>   var ctx = document.createElement("canvas").getContext("2d");
>   try {
>     ctx.drawImage({}, 0, 0);
>   } catch(e) {
>     // interrogate the exception
>   }
> 
> which should hit this case.

do we want to add something to idlharness.js for this, or where should that test go?
idlharness doesn't and shouldn't test the *message* btw, just that the exception is a TypeError.
A test in dom/bindings/test should be fine.
Attached patch patch v2 (obsolete) — Splinter Review
ok, this is the interesting bit of the patch dom/bindings/

I ran the mochitests in dom/imptests/ and ran parseFailures.py on the result which produces a diff to large to attach uncompressed :( I'm not sure why the changes are so huge but atleast a lot of it appears to just be reformating. I will note though that the only tests the XMLHttpRequest test_interfaces.html to fail is the constructor test and a bunch of FormData stuff.
Attachment #659075 - Attachment is obsolete: true
Attachment #666252 - Flags: review?(bzbarsky)
Hmm.  So there's a second patch to change our known-fails list?  What's the review plan for that one?
(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> I ran the mochitests in dom/imptests/ and ran parseFailures.py on the result
> which produces a diff to large to attach uncompressed :( I'm not sure why
> the changes are so huge but atleast a lot of it appears to just be
> reformating.

Because of bug 794891. Just revert that change locally for now, I'll deal with it at some point.
Sorry, I was wrong. Try updating to 12453e5547a8, I hope that should fix most unnecessary changes.
(In reply to :Ms2ger from comment #21)
> Sorry, I was wrong. Try updating to 12453e5547a8, I hope that should fix
> most unnecessary changes.

doesn't look like that worked :(  My diff to update the know failures is ~3400 lines and about 5 megs, and it does look like files are just getting rewritten wholesale.

(In reply to Boris Zbarsky (:bz) from comment #19)
> Hmm.  So there's a second patch to change our known-fails list?  What's the
> review plan for that one?

unclear, probably figure out why parseFailures.py is rewriting whole files and how to get around that then hopefully we will have something its possible to review.
> (In reply to Boris Zbarsky (:bz) from comment #19)
> > Hmm.  So there's a second patch to change our known-fails list?  What's the
> > review plan for that one?
> 
> unclear, probably figure out why parseFailures.py is rewriting whole files
> and how to get around that then hopefully we will have something its
> possible to review.

if it seems like that's hard I gues I can just hand you the list of new failures to review.

actually, most of the known failure changes outside of XMLHttpRequest test_interfaces.html should just because of the changes to idlharness.js which shouldn't' land in this patch anyway.  So I think the thing to do here actually is get Ms2ger or AryehGregor to pull from upstream and then the problem should just go away.
I'll update.
Comment on attachment 666252 [details] [diff] [review]
patch v2

>+MSG_DEF(MSG_METHOD_FAILED, 2, "method '{0}' on interface '{1}' failed!")

This seems to be unused. Please take it out.

r=me with that.  Thank you for doing this!
Attachment #666252 - Flags: review?(bzbarsky) → review+
This updates the lists of expected failures for recent changes to parseFailures.py.
Attachment #667502 - Flags: review?(ayg)
This updates the test harness from upsteam and adds the new expected failures.
Attachment #659232 - Attachment is obsolete: true
Attachment #667504 - Flags: review?(ayg)
Trevor, with those three patches applied, you should get a sane result from parseFailures.py
Blocks: 793151
Comment on attachment 667502 [details] [diff] [review]
Part a: Regenerate expected failures

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

::: dom/imptests/parseFailures.py
@@ +32,5 @@
>          if objstr == '{}\n':
>              continue
>  
>          # Avoid overly large diffs.
> +        if 'editing/' in url:

Did you mean to include this in your patch?  It doesn't seem like a good idea to mix other changes in with large automated things like this -- they'll get lost.  r+ on the rest provided it's just the result of rerunning the appropriate scripts, but please put this change in a separate patch.

(This check seems fragile, too -- what if some subdirectory called "editing/" is added somewhere else sometime in the future?  The pattern should really be anchored at the start somehow, although I'm guessing that's not completely trivial or else you'd have done it.)
Attachment #667502 - Flags: review?(ayg) → review+
Comment on attachment 667504 [details] [diff] [review]
Part b: Update testharness.js and friends

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

Thanks for doing this!

::: dom/imptests/failures/editing/selecttest/test_interfaces.html.json
@@ +6,5 @@
> +  "Selection interface: attribute anchorOffset":true,
> +  "Selection interface: attribute focusNode":true,
> +  "Selection interface: attribute focusOffset":true,
> +  "Selection interface: attribute isCollapsed":true,
> +  "Selection interface: attribute rangeCount":true,

Looks like these are already filed as bug 791537, along with the other "attribute X" ones.  (I didn't look at every single one.)

::: dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json
@@ +60,5 @@
>    "Stringification of new CustomEvent(\"foo\")": "debug",
>    "Event interface: new CustomEvent(\"foo\") must inherit property \"timeStamp\" with the proper type (14)": true,
>    "Event interface: calling initEvent(DOMString,boolean,boolean) on new CustomEvent(\"foo\") with too few arguments must throw TypeError": true,
>    "EventTarget interface: operation addEventListener(DOMString,EventListener,boolean)": true,
> +  "EventTarget interface: operation removeEventListener(DOMString,EventListener,boolean)": true,

Looks like bug 793151, along with all the other operation failures.  (I looked at all of them, since there aren't very many.)

::: dom/imptests/failures/webapps/XMLHttpRequest/tests/submissions/Ms2ger/test_interfaces.html.json
@@ +1,2 @@
>  {
> +  "XMLHttpRequestUpload interface: existence and properties of interface prototype object": true,

I see this:

assert_equals: prototype of XMLHttpRequestUpload.prototype is not Object.prototype expected object "[object Object]" but got object "[object XMLHttpRequestEventTargetPrototype]"

Why?  XMLHttpRequestUpload seems to inherit from XMLHttpRequestEventTarget in the interface, so why is it testing for Object.prototype here?

It looks like it's due to this change:

http://dvcs.w3.org/hg/resources/rev/5bc1e3580cdf

As far as I'm reading the spec, this is incorrect.  [NoInterfaceObject] means there's no interface object, but there's still an interface *prototype* object, which is still accessible using .getPrototypeOf().  I don't think this should land until this issue is fixed.  Please back out this commit from the resources repo before syncing, then re-request review.  Or else point out why I'm wrong!
Attachment #667504 - Flags: review?(ayg)
Attachment #667504 - Flags: review-
Attachment #667504 - Flags: feedback+
Comment on attachment 667504 [details] [diff] [review]
Part b: Update testharness.js and friends

On second thought, backing out 5bc1e3580cdf shouldn't block this bug, since it only causes a bogus new test failure.  Just file a followup bug to fix that and CC me, that's sufficient.  (Although you have a patch ready, so you could also just apply that if you like.)
Attachment #667504 - Flags: review-
Attachment #667504 - Flags: review+
Attachment #667504 - Flags: feedback+
Attached patch Part c (2): Fix up the XHR test (obsolete) — Splinter Review
Attachment #670804 - Flags: review?(ayg)
Qref? Eh.
Attachment #670804 - Attachment is obsolete: true
Attachment #670804 - Flags: review?(ayg)
Attachment #670806 - Flags: review?(ayg)
Attachment #670803 - Flags: review?(ayg) → review+
Comment on attachment 670806 [details] [diff] [review]
Part c (2): Fix up the XHR test (bis)

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

Thanks!
Attachment #670806 - Flags: review?(ayg) → review+
Blocks: 801712
Should this be marked fixed?
It looks like only tests are updated.
Attachment #666252 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ed39040f53ad
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.