Bug 95728 – [XMLHttpRequest] overrideMimeType(mime) should throw if state is LOADING or DONE

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

According to the latest XMLHttpRequest specification (http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#dom-xmlhttprequest-overridemimetype):

The overrideMimeType(mime) throws an "InvalidStateError" exception if the state is LOADING or DONE.

The current implementation never throws.

[Chris Dumez]

Created attachment 162015 [details]
Patch

[Kentaro Hara]

Comment on attachment 162015 [details]
Patch

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

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-invalid-state.html:15
> +                finishJSTest();

Is it OK to finish the test here? (I'm not sure why this test proceeds to xhr.readyState == xhr.Done.)

> Source/WebCore/ChangeLog:11
> +        This behavior is according to the latest XMLHttpRequest specification:

Would you check the behavior of other browsers (Firefox, IE, Opera)? You need to take care of compatibility when you try to throw an exception for the code that has not thrown an exception.

[Kenneth Rohde Christiansen]

Comment on attachment 162015 [details]
Patch

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

>> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-invalid-state.html:15
>> +                finishJSTest();
> 
> Is it OK to finish the test here? (I'm not sure why this test proceeds to xhr.readyState == xhr.Done.)

Yeah it could just return, or the next if could be an else if

[Kentaro Hara]

(In reply to comment #3)
> >> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-invalid-state.html:15
> >> +                finishJSTest();
> > 
> > Is it OK to finish the test here? (I'm not sure why this test proceeds to xhr.readyState == xhr.Done.)
> 
> Yeah it could just return, or the next if could be an else if

I'm sorry if I'm wrong:

(1) xhr.onreadystatechange is invoked.
(2) if (xhr.readyState == xhr.LOADING) calls finishJSTest().
(3) finishJSTest() calls testRunner.notifyDone(), which tells the testRunner that the asynchronous test has finished. 
(4) xhr.onreadystatechange is invoked.
(5) if (xhr.readyState == xhr.DONE) calls finishJSTest().

My question is why (4) and (5) run, even though the test has already finished at the point of (3).

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> (In reply to comment #3)
> > >> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-invalid-state.html:15
> > >> +                finishJSTest();
> > > 
> > > Is it OK to finish the test here? (I'm not sure why this test proceeds to xhr.readyState == xhr.Done.)
> > 
> > Yeah it could just return, or the next if could be an else if
> 
> I'm sorry if I'm wrong:
> 
> (1) xhr.onreadystatechange is invoked.
> (2) if (xhr.readyState == xhr.LOADING) calls finishJSTest().
> (3) finishJSTest() calls testRunner.notifyDone(), which tells the testRunner that the asynchronous test has finished. 
> (4) xhr.onreadystatechange is invoked.
> (5) if (xhr.readyState == xhr.DONE) calls finishJSTest().
> 
> My question is why (4) and (5) run, even though the test has already finished at the point of (3).

You are right, of course it should not finish before everything has been tested.

[Chris Dumez]

(In reply to comment #4)
> (In reply to comment #3)
> > >> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-invalid-state.html:15
> > >> +                finishJSTest();
> > > 
> > > Is it OK to finish the test here? (I'm not sure why this test proceeds to xhr.readyState == xhr.Done.)
> > 
> > Yeah it could just return, or the next if could be an else if
> 
> I'm sorry if I'm wrong:
> 
> (1) xhr.onreadystatechange is invoked.
> (2) if (xhr.readyState == xhr.LOADING) calls finishJSTest().
> (3) finishJSTest() calls testRunner.notifyDone(), which tells the testRunner that the asynchronous test has finished. 
> (4) xhr.onreadystatechange is invoked.
> (5) if (xhr.readyState == xhr.DONE) calls finishJSTest().
> 
> My question is why (4) and (5) run, even though the test has already finished at the point of (3).

Yes, my bad, the finishJSTest() for LOADING case should not be there. I'll remove it.

I have checked with Firefox and Opera. None of them throw for this case.
I don't have access to IE to test it.

[Kentaro Hara]

(In reply to comment #6)
> I have checked with Firefox and Opera. None of them throw for this case.
> I don't have access to IE to test it.

Then you might want to fix the spec instead. Throwing an exception for the code that has not thrown an exception sounds dangerous. Let's hear opinions of XHR experts.

[Chris Dumez]

Created attachment 162027 [details]
Patch

Fixed the LayoutTest as reported by Haraken.

[Chris Dumez]

For your information:

The old spec (http://www.w3.org/TR/2010/WD-XMLHttpRequest2-20100907/#the-overridemimetype-method) did not indicate that we should throw for this case.

This was corrected later in:
http://www.w3.org/TR/2011/WD-XMLHttpRequest2-20110816/#the-overridemimetype-method

And the latest working draft still indicates we should throw:
http://www.w3.org/TR/2012/WD-XMLHttpRequest-20120117/#the-overridemimetype-method

[Alexey Proskuryakov]

Can you find working group discussion that led to this change? We need to somehow verify that this change would be good for WebKit users.

[Chris Dumez]

I added James Robinson in CC since it seems he proposed the throwing behavior in:
http://lists.w3.org/Archives/Public/public-webapps/2010OctDec/0022.html

This thread seems related as well:
http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/0261.html

[Chris Dumez]

Third related thread:
http://lists.w3.org/Archives/Public/public-webapps/2011OctDec/0883.html

This relates to the following change in the latest spec:

client . overrideMimeType(mime)
Sets the Content-Type header for the response to mime.
- Throws an INVALID_STATE_ERR exception if the state is not OPENED or HEADERS_RECEIVED.
+ Throws an "InvalidStateError" exception if the state is LOADING or DONE.

[Alexey Proskuryakov]

It's curious that the discussion seems to have revolved around charset only. That case is undoubtedly unfortunate, but what about changing MIME type in order to use responseXML? That should be fine to do later.

[Anders Carlsson]

Comment on attachment 162027 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Rob Buis]

This was fixed in https://bugs.webkit.org/show_bug.cgi?id=136699.

[Rob Buis]

As stated in Comment 15, I think this is fixed.