If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 50



DOM: Core & HTML
4 years ago
a year ago


(Reporter: hallvors, Assigned: Thomas Wisniewski)


Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)




(1 attachment, 5 obsolete attachments)



4 years ago
Test case:

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:

Comment 1

a year ago
Created attachment 8756932 [details] [diff] [review]

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?
Attachment #8756932 - Flags: review?(jonas)
Comment on attachment 8756932 [details] [diff] [review]

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_LOADING |               \
> +   XML_HTTP_REQUEST_DONE |                  \

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?
Attachment #8756932 - Flags: review?(jonas) → review-

Comment 3

a year ago
Created attachment 8757373 [details] [diff] [review]

New version of the patch with comments addressed.
Attachment #8756932 - Attachment is obsolete: true
Attachment #8757373 - Flags: review?(jonas)

Comment 4

a year ago
Created attachment 8757525 [details] [diff] [review]

And one more version without the pointless extra newline (sorry for the resulting bugspam).
Attachment #8757373 - Attachment is obsolete: true
Attachment #8757373 - Flags: review?(jonas)
Attachment #8757525 - Flags: review?(jonas)
Comment on attachment 8757525 [details] [diff] [review]

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)) {


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

The last line here should not be needed.
Attachment #8757525 - Flags: review?(jonas) → review+

Comment 6

a year ago
Created attachment 8757597 [details] [diff] [review]

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.
Attachment #8757525 - Attachment is obsolete: true
Attachment #8757597 - Flags: review?(jonas)

Comment 7

a year ago
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 on attachment 8757597 [details] [diff] [review]

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.
Attachment #8757597 - Flags: review?(jonas) → review+

Comment 9

a year ago
Created attachment 8761008 [details] [diff] [review]

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

a year ago
Created attachment 8762605 [details] [diff] [review]

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.
Attachment #8757597 - Attachment is obsolete: true
Attachment #8761008 - Attachment is obsolete: true
Is there a keyword for help checking in to try?
Keywords: checkin-needed
(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,
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed

Comment 13

a year ago
Alright, my request is in bug 1280050.

Comment 14

a year ago
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.


a year ago
Flags: needinfo?(wisniewskit)

Comment 15

a year ago
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).
Keywords: checkin-needed

Comment 16

a year ago
Pushed by cbook@mozilla.com:
have overrideMimeType throw INVALID_STATE_ERR if the XHR is in the DONE or LOADING states. r=sicking
Keywords: checkin-needed
Thanks for the patch Thomas!!

Comment 18

a year ago
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50


a year ago
Depends on: 1280682
Assignee: nobody → wisniewskit
You need to log in before you can comment on or make changes to this bug.