Bug 90976 – If value for responseType defined as type that not supported, it should not throw an exception in XHR 2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Oleg]

If responseType="bla" (or "json" for that matter) it will throw an exception "SYNTAX_ERR: DOM Exception 12".
This exception is not defined by spec and no other browser is throwing it on value it did not recognize.

[Kinuko Yasuda]

Created attachment 152753 [details]
Patch

[Jarred Nicholls]

Comment on attachment 152753 [details]
Patch

I recommend using the test harness code instead of manually doing dumpAsText() and log()'s.

[Kinuko Yasuda]

(In reply to comment #2)
> (From update of attachment 152753 [details])
> I recommend using the test harness code instead of manually doing dumpAsText() and log()'s.

Thanks for your comment.  I'll update the patch assuming you meant using the test utility methods like the ones defined in fast/js/resources/js-test-pre.js.

[Kinuko Yasuda]

Created attachment 152758 [details]
Patch

[Jarred Nicholls]

Comment on attachment 152758 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 152758 [details]
Patch

Attachment 152758 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13272177

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-arraybuffer.html

[WebKit Review Bot]

Created attachment 152773 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Kinuko Yasuda]

Created attachment 152971 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 152971 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        The spec doesn't say it should throw an exception when a non-supported type is set, or other browsers does not throw it either.

s/ or / and /
s/ does / do /

> Source/WebCore/xml/XMLHttpRequest.cpp:347
> +    // http://www.w3.org/TR/XMLHttpRequest2/#the-responsetype-attribute
> +    // An attempt to set an invalid type is silently discarded (does not change the value).

Please remove this comment. It just re-states what the code below does, and gives a link to a spec anyone working on this code knows about.

> Source/WebCore/xml/XMLHttpRequest.cpp:-357
> -    else
> -        ec = SYNTAX_ERR;

I think that we should log to Inspector when an unsupported type is used. It's something that is going to break sites, and would be incredibly hard to debug without an exception or any diagnostic logging.

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-set-types-expected.txt:5
> +PASS No exception.

This is not the best way to write a test. Ideally, the output would be self-explanatory - it's hard to correlate output lines with subtests here.

In this particular case, you can consider two approaches:
1. Use shouldNotThrow() function provided by js-test-pre.js;
2. Put preparation code inside shouldBe, like shouldBe("xhr.responseType = 'text'; xhr.responseType", "'text'");

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-set-types.html:9
> +function setResponseType(type, expected)

This function doesn't just set the response type, so the name is misleading.

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-set-types.html:27
> +setResponseType('arraybuffer', 'arraybuffer');
> +setResponseType('blob', 'blob');

Aren't these under ifdefs? We'll need platform specific results because of these sub-tests, and they don't really check anything changed in this patch.

[Kinuko Yasuda]

Created attachment 153236 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 152971 [details]
Patch

Thanks for reviewing,

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

>> Source/WebCore/ChangeLog:9
>> +        The spec doesn't say it should throw an exception when a non-supported type is set, or other browsers does not throw it either.
> 
> s/ or / and /
> s/ does / do /

Done.

>> Source/WebCore/xml/XMLHttpRequest.cpp:347
>> +    // An attempt to set an invalid type is silently discarded (does not change the value).
> 
> Please remove this comment. It just re-states what the code below does, and gives a link to a spec anyone working on this code knows about.

Done.

>> Source/WebCore/xml/XMLHttpRequest.cpp:-357
>> -        ec = SYNTAX_ERR;
> 
> I think that we should log to Inspector when an unsupported type is used. It's something that is going to break sites, and would be incredibly hard to debug without an exception or any diagnostic logging.

Good point... added logging.

>> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-set-types-expected.txt:5
>> +PASS No exception.
> 
> This is not the best way to write a test. Ideally, the output would be self-explanatory - it's hard to correlate output lines with subtests here.
> 
> In this particular case, you can consider two approaches:
> 1. Use shouldNotThrow() function provided by js-test-pre.js;
> 2. Put preparation code inside shouldBe, like shouldBe("xhr.responseType = 'text'; xhr.responseType", "'text'");

Makes sense, updated the code.  I added two subtests using each approach, to make it clearer what we are testing.

>> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-set-types.html:9
>> +function setResponseType(type, expected)
> 
> This function doesn't just set the response type, so the name is misleading.

I've removed this function.

>> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-set-types.html:27
>> +setResponseType('blob', 'blob');
> 
> Aren't these under ifdefs? We'll need platform specific results because of these sub-tests, and they don't really check anything changed in this patch.

They are no longer ifdef'ed. I thought it might be good to have a test for setting responseType but similar tests seem to be included in xmlhttprequest-responsetype-arraybuffer.html.

I removed them and renamed this test to '...-set-invalidtype.html'.

[WebKit Review Bot]

Comment on attachment 153236 [details]
Patch

Attachment 153236 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13281906

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-set-invalidtype.html

[WebKit Review Bot]

Created attachment 153248 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Kinuko Yasuda]

Created attachment 153253 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 153253 [details]
Patch

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

r=me, but please double check why cross-platform results don't need CONSOLE MESSAGE lines. I think that it's a mistake.

> Source/WebCore/xml/XMLHttpRequest.cpp:357
> +        logConsoleError(scriptExecutionContext(), "\"" + responseType + "\" is not a valid XMLHttpRequest.responseType.");

I would have said "supported", not "valid", because new types can be added to the spec in the future, and lack of support for them in a certain release of a WebKit based browser doesn't make such invalid. But it's not a big deal.

> LayoutTests/ChangeLog:14
> +        * fast/xmlhttprequest/xmlhttprequest-responsetype-arraybuffer.html: Removed a line which tries to set invalid type (since we now have a separate test)
> +        * platform/chromium/fast/xmlhttprequest/xmlhttprequest-responsetype-set-invalidtype-expected.txt: Added (since chromium expectation includes CONSOLE messages).

This surprises me - I think that every port does this. Looking at Mac DumpRenderTree source code, I do see console messages printed.

[Kinuko Yasuda]

(In reply to comment #15)
> (From update of attachment 153253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153253&action=review
> 
> r=me, but please double check why cross-platform results don't need CONSOLE MESSAGE lines. I think that it's a mistake.

You're right, resetting the results got the expected CONSOLE message on Mac too.

> > Source/WebCore/xml/XMLHttpRequest.cpp:357
> > +        logConsoleError(scriptExecutionContext(), "\"" + responseType + "\" is not a valid XMLHttpRequest.responseType.");
> 
> I would have said "supported", not "valid", because new types can be added to the spec in the future, and lack of support for them in a certain release of a WebKit based browser doesn't make such invalid. But it's not a big deal.

Will fix the line together with the expectation before submitting.

[Kinuko Yasuda]

Created attachment 153428 [details]
Patch for submit

[Kinuko Yasuda]

Committed r123194: <http://trac.webkit.org/changeset/123194>