Bug 95728 - [XMLHttpRequest] overrideMimeType(mime) should throw if state is LOADING or DONE
: [XMLHttpRequest] overrideMimeType(mime) should throw if state is LOADING or DONE
Status: ASSIGNED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://dvcs.w3.org/hg/xhr/raw-file/ti...
:
:
:
  Show dependency treegraph
 
Reported: 2012-09-04 03:59 PST by
Modified: 2014-02-05 12:39 PST (History)


Attachments
Patch (6.43 KB, patch)
2012-09-04 05:07 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.28 KB, patch)
2012-09-04 06:05 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-09-04 03:59:43 PST
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.
------- Comment #1 From 2012-09-04 05:07:09 PST -------
Created an attachment (id=162015) [details]
Patch
------- Comment #2 From 2012-09-04 05:19:07 PST -------
(From update of attachment 162015 [details])
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.
------- Comment #3 From 2012-09-04 05:25:17 PST -------
(From update of attachment 162015 [details])
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
------- Comment #4 From 2012-09-04 05:38:32 PST -------
(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).
------- Comment #5 From 2012-09-04 05:45:39 PST -------
(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.
------- Comment #6 From 2012-09-04 05:50:59 PST -------
(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.
------- Comment #7 From 2012-09-04 05:53:29 PST -------
(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.
------- Comment #8 From 2012-09-04 06:05:01 PST -------
Created an attachment (id=162027) [details]
Patch

Fixed the LayoutTest as reported by Haraken.
------- Comment #9 From 2012-09-04 06:09:45 PST -------
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
------- Comment #10 From 2012-09-04 09:20:04 PST -------
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.
------- Comment #11 From 2012-09-04 09:50:41 PST -------
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
------- Comment #12 From 2012-09-04 09:56:19 PST -------
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.
------- Comment #13 From 2012-09-04 10:24:46 PST -------
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.
------- Comment #14 From 2014-02-05 11:13:16 PST -------
(From update of attachment 162027 [details])
Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.