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
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Chris Dumez
http://dvcs.w3.org/hg/xhr/raw-file/ti...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-04 03:59 PDT by Chris Dumez
Modified: 2014-08-11 15:34 PDT (History)
13 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-09-04 03:59:43 PDT
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 Chris Dumez 2012-09-04 05:07:09 PDT
Created attachment 162015 [details]
Patch
Comment 2 Kentaro Hara 2012-09-04 05:19:07 PDT
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.
Comment 3 Kenneth Rohde Christiansen 2012-09-04 05:25:17 PDT
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
Comment 4 Kentaro Hara 2012-09-04 05:38:32 PDT
(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 Kenneth Rohde Christiansen 2012-09-04 05:45:39 PDT
(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 Chris Dumez 2012-09-04 05:50:59 PDT
(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 Kentaro Hara 2012-09-04 05:53:29 PDT
(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 Chris Dumez 2012-09-04 06:05:01 PDT
Created attachment 162027 [details]
Patch

Fixed the LayoutTest as reported by Haraken.
Comment 9 Chris Dumez 2012-09-04 06:09:45 PDT
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 Alexey Proskuryakov 2012-09-04 09:20:04 PDT
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 Chris Dumez 2012-09-04 09:50:41 PDT
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 Chris Dumez 2012-09-04 09:56:19 PDT
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 Alexey Proskuryakov 2012-09-04 10:24:46 PDT
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 Anders Carlsson 2014-02-05 11:13:16 PST
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.