Tests for exceptions of type DOMException.TYPE_ERR in encrypted-media-syntax.html are broken

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

These tests were added in http://trac.webkit.org/changeset/113736 and check that the error's code attribute is set to DOMException.TYPE_ERR. However, I don't think they're doing what was intended.

In both JSC and V8, DOMException.TYPE_ERR is undefined. I think the correct name of the error is DOMException.TYPE_MISMATCH_ERR. 

The tests pass because neither JSC nor V8 set the error attribute when creating errors of this type. They do however set the attribute for other types of errors. I'm not sure whether or not this is a bug in the bindings, though I suspect not.

We should fix the test and perhaps the bindings too.

[Steve Block]

Created attachment 194953 [details]
Patch

[Steve Block]

Comment on attachment 194953 [details]
Patch

David, as author of the tests, do you know whether it's expected that the exception error code is not set in this case?

[David Dorwin]

(In reply to comment #2)
> (From update of attachment 194953 [details])
> David, as author of the tests, do you know whether it's expected that the exception error code is not set in this case?

I'm not sure what you mean. I expected two few parameters to throw an exception. That's really all that matters.


View in context: https://bugs.webkit.org/attachment.cgi?id=194953&action=review

> LayoutTests/media/video-test.js:211
> +        logResult(code === DOMException.TYPE_MISMATCH_ERR || code === ex.code,

What does the first comparison do to prevent the problem?

[Steve Block]

> I'm not sure what you mean. I expected two few parameters to throw an exception. That's really all that matters.
In addition to checking that an exception is thrown, the tests check the value of the exception's code attribute. In some of these tests, the expected value is DOMException.TYPE_ERR, but this constant is undefined. These tests only pass because the exception's code attribute is also undefined. Presumably, this isn't intentional?

I think the expected value should be DOMException.TYPE_MISMATCH_ERR. My question is whether the exception's code attribute should be undefined in this case or whether it should be set to this value.

[David Dorwin]

(In reply to comment #4)
> > I'm not sure what you mean. I expected two few parameters to throw an exception. That's really all that matters.
> In addition to checking that an exception is thrown, the tests check the value of the exception's code attribute. In some of these tests, the expected value is DOMException.TYPE_ERR, but this constant is undefined. These tests only pass because the exception's code attribute is also undefined. Presumably, this isn't intentional?
> 
> I think the expected value should be DOMException.TYPE_MISMATCH_ERR. My question is whether the exception's code attribute should be undefined in this case or whether it should be set to this value.

I'm not sure what the correct behavior is or if the current behavior (below) is expected or a bug in the implementation. I'd defer to someone with more experience in this area. It appears that this is not actually a DOMException that gets thrown.

Chrome (and maybe other ports) does not throw TYPE_MISMATCH_ERR for these values. It reports "TypeError: Not enough arguments", which does not have a value like the other tests.

This is one of several other test that expects the same behavior:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/LayoutTests/storage/storageinfo-missing-arguments.html&l=9

Below are a few commands and results run in Chrome's Developer Tools Console.

video.webkitGenerateKeyRequest()
TypeError: Not enough arguments

video.webkitAddKey('webkit-org.w3.clearkey', '')
Error: SyntaxError: DOM Exception 12

video.webkitAddKey('webkit-org.w3.clearkey', new Uint8Array())
Error: TypeMismatchError: DOM Exception 17

[Steve Block]

> It appears that this is not actually a DOMException that gets thrown.
I think you're right. In the case where insufficient arguments are supplied, a v8::Exception::TypeError is thrown, which doesn't have a code attribute. The code attribute is present only on DOMExceptions.
 
I think the right fix is to rename the existing testException() function to testDomException(), and add a new testException() function (similar to shouldThrow() from js-test-pre.js) for the tests with insufficient arguments.

[Steve Block]

Created attachment 195689 [details]
Patch

[Steve Block]

Created attachment 195690 [details]
Patch

[David Dorwin]

Comment on attachment 195690 [details]
Patch

Seems ok. You mentioned V8's behavior. Does JSC also throw a TypeError with the same string? These tests also run on Mac.

View in context: https://bugs.webkit.org/attachment.cgi?id=195690&action=review

> LayoutTests/media/video-test.js:208
>          eval(testString);

To help avoid and detect accidental bad expected files, should we logResult("Failed to receive exception for....")?
Same at 217.

[Steve Block]

Created attachment 196270 [details]
Patch

[Steve Block]

> Seems ok. You mentioned V8's behavior. Does JSC also throw a TypeError with the
> same string? These tests also run on Mac.
Yes, it does.

> To help avoid and detect accidental bad expected files, should we logResult("Failed to receive exception for....")?
Done

[Steve Block]

+abarth for review, who originally reviewed these tests

[David Dorwin]

Comment on attachment 196270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196270&action=review

> LayoutTests/media/video-test.js:212
> +    logResult(exception instanceof DOMException && exception.code === eval(exceptionString),

|exception| could be undefined here if the |testString| did not throw. It seems that would lead to a confusing failure message.
Would it be simpler to just logResult("FAILED: " + testString + " did not throw " + exceptionString) after 208 and move this back into the catch?

[Steve Block]

Comment on attachment 196270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196270&action=review

>> LayoutTests/media/video-test.js:212
>> +    logResult(exception instanceof DOMException && exception.code === eval(exceptionString),
> 
> |exception| could be undefined here if the |testString| did not throw. It seems that would lead to a confusing failure message.
> Would it be simpler to just logResult("FAILED: " + testString + " did not throw " + exceptionString) after 208 and move this back into the catch?

I don't think the failure message would be confusing: logResult will append 'FAIL' to the text below. I prefer this approach as it means we only need a single call to logResult() and hence a single description of the test.

[David Dorwin]

Comment on attachment 196270 [details]
Patch

Ok. Thanks.

[WebKit Commit Bot]

Comment on attachment 196270 [details]
Patch

Clearing flags on attachment: 196270

Committed r147969: <http://trac.webkit.org/changeset/147969>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.