918733 - [XHR2] overrideMimeType() in DONE state doesn't throw

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Hallvord R. M. Steen [:hallvors]]

Test case:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/overridemimetype-done-state.htm

Implementations are somewhat messy, for example I think Chrome/WebKit/Blink allow calling the method but it will only work for overriding MIME type and not for overriding encoding (?!?). In certain other browsers it's a noop even though it doesn't throw.

Noting also that spec is slightly controversial:
https://bugs.webkit.org/show_bug.cgi?id=95728#c10

Comment 1

[Thomas Wisniewski]

Attachment: 918733-throw-invalid-state-err-on-overridemimetype-during-xhr-loading-or-done-states.diff

Note that Chrome now throws whether also specifying a charset or not.

Here's a patch that makes these two WPTs pass to match Chrome:
- http://w3c-test.org/XMLHttpRequest/overridemimetype-loading-state.htm
- http://w3c-test.org/XMLHttpRequest/overridemimetype-done-state.htm

I'm not sure if these changes warrant updating nsIXMLHttpRequest's uuid or not?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8756932 [details] [diff] [review]
918733-throw-invalid-state-err-on-overridemimetype-during-xhr-loading-or-done-states.diff

Review of attachment 8756932 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but please fix the below and I'll review the updated patch.

::: dom/base/nsIXMLHttpRequest.idl
@@ +252,5 @@
>     * @param mimetype The type used to override that returned by the server
>     *                 (if any).
>     */
> +  [binaryname(SlowOverrideMimeType)] void overrideMimeType(in DOMString mimetype)
> +                                       raises(DOMException);

'raises' in xpidl doesn't really do anything. So don't add this

::: dom/base/nsXMLHttpRequest.h
@@ +68,5 @@
> +   XML_HTTP_REQUEST_OPENED |                \
> +   XML_HTTP_REQUEST_HEADERS_RECEIVED |      \
> +   XML_HTTP_REQUEST_LOADING |               \
> +   XML_HTTP_REQUEST_DONE |                  \
> +   XML_HTTP_REQUEST_SENT)

Don't move these to the .h file. Instead make overrideMimeType not an inlined function.

::: testing/web-platform/meta/XMLHttpRequest/overridemimetype-done-state.htm.ini
@@ +5,1 @@
>  

Can't we simply remove this file instead?

::: testing/web-platform/meta/XMLHttpRequest/overridemimetype-loading-state.htm.ini
@@ +1,4 @@
>  [overridemimetype-loading-state.htm]
>    type: testharness
>    [XMLHttpRequest: overrideMimeType() in LOADING state]
> +    expected: PASS

Can't we simply remove this file instead?

Comment 3

[Thomas Wisniewski]

Attachment: 918733-throw-invalid-state-err-on-overridemimetype-during-xhr-loading-or-done-states.diff

New version of the patch with comments addressed.

Comment 4

[Thomas Wisniewski]

Attachment: 918733-throw-invalid-state-err-on-overridemimetype-during-xhr-loading-or-done-states.diff

And one more version without the pointless extra newline (sorry for the resulting bugspam).

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8757525 [details] [diff] [review]
918733-throw-invalid-state-err-on-overridemimetype-during-xhr-loading-or-done-states.diff

Review of attachment 8757525 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that. But we also need tests.

::: dom/base/nsXMLHttpRequest.cpp
@@ +3120,5 @@
>  
> +void nsXMLHttpRequest::OverrideMimeType(const nsAString& aMimeType, ErrorResult& aRv)
> +{
> +  if ((mState & XML_HTTP_REQUEST_LOADING) || (mState & XML_HTTP_REQUEST_DONE)) {
> +    aRv = NS_ERROR_DOM_INVALID_STATE_ERR;

aRv.Throw(NS_ERROR...);

@@ +3125,5 @@
> +    return;
> +  }
> +
> +  mOverrideMimeType = aMimeType;
> +  aRv = NS_OK;

The last line here should not be needed.

Comment 6

[Thomas Wisniewski]

Attachment: 918733-throw-invalid-state-err-on-overridemimetype-during-xhr-loading-or-done-states.diff

Sure, good thinking on adding a mochitest: it uncovered a corner case that I hadn't considered, where fetching over file rather than http was causing the responseXML to not be null (as the web platform tests expect). I added a ResetResponse() call to the patch before aRv.Throw()ing to make sure it passes, and since I made that change, I figured I'd r? again just in case.

I also added a new mochitest to this version of this patch, to cover the web platform test's case as well as the charset case that :hallvors mentioned in comment 0. If it would be best to split the mochitest changes into a separate patch, just let me know.

Comment 7

[Hallvord R. M. Steen [:hallvors]]

This bug does have a test already (in web-platform-test suite) - if the test gives insufficient coverage of the code paths let me know how to improve it! :)

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8757597 [details] [diff] [review]
918733-throw-invalid-state-err-on-overridemimetype-during-xhr-loading-or-done-states.diff

Review of attachment 8757597 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Hallvord, could you provide guidence for how to enable the webplatform tests that should cover this bug.

In the meantime, as long as this passes tryserver we should check this in.

Comment 9

[Thomas Wisniewski]

Attachment: 918733-mark-related-web-platform-tests-as-expected-passes.diff

Sure, here's a second patch that marks the two related tests as passing. I don't think it needs review, so not marking it as such.

Comment 10

[Thomas Wisniewski]

Attachment: 918733-throw-invalid-state-err-on-overridemimetype-during-xhr-loading-or-done-states.diff

Friendly ping. I'm guessing this is still due for the try servers, to which I don't have access.

Here's a rebased version of the patch plus the web platform test changes, in one file. Carrying over r+.

hallvors, the web platform tests do indeed cover the same bases as my mochitest, but they're run over http, not the file protocol. The specific problem I ran into only happened in the latter case, and I'm not sure if it's even possible to reproduce that in the web platform tests.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is there a keyword for help checking in to try?

Comment 12

[Carsten Book [:Tomcat]]

(In reply to Jonas Sicking (:sicking) from comment #11)
> Is there a keyword for help checking in to try?

Hey Jonas,

not really since L1 Access is easy to get. 

Thomas, can you follow https://www.mozilla.org/en-US/about/governance/policies/commit/ and i guess jonas will be happy to vouch for the access, so you can push to try,

Comment 13

[Thomas Wisniewski]

Alright, my request is in bug 1280050.

Comment 14

[Hallvord R. M. Steen [:hallvors]]

Thomas: thanks for adding those tests, you're right that web-platform-tests doesn't handle file:// . My main concern is that web-platform-tests is as complete as possible, so that we don't add feature tests in Mochitests that are not in w-p-t.

Comment 15

[Thomas Wisniewski]

Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9b706bc162b

I'm not seeing any failures related to the patch, but since this is my first try run, it'd be for the best if someone could double-check (especially if I missed running any jobs that I should have).

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87aee9d5e04d
have overrideMimeType throw INVALID_STATE_ERR if the XHR is in the DONE or LOADING states. r=sicking

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Thanks for the patch Thomas!!

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87aee9d5e04d