Bug 184108 – Align XMLHttpRequest's open() / send() / abort() with the latest specification

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Align XMLHttpRequest's open() / send() / abort() with the latest specification:
- https://xhr.spec.whatwg.org

[Chris Dumez]

Created attachment 336697 [details]
WIP Patch

[Chris Dumez]

Created attachment 336698 [details]
WIP Patch

[Chris Dumez]

Created attachment 336702 [details]
WIP Patch

[Chris Dumez]

Created attachment 336711 [details]
WIP Patch

[Chris Dumez]

Created attachment 336717 [details]
WIP Patch

[Chris Dumez]

Created attachment 336721 [details]
Patch

[Chris Dumez]

Created attachment 336725 [details]
Patch

[youenn fablet]

Comment on attachment 336725 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:338
> +    m_method = normalizeHTTPMethod(method);

We can probably normalize the method after the early returns, at the moment the spec says to set m_method.

> Source/WebCore/xml/XMLHttpRequest.cpp:381
>          changeState(OPENED);

changeState is checking for m_state so we can probably call changeState directly, name is not great though.

> Source/WebCore/xml/XMLHttpRequest.cpp:591
> +    // Also, only async requests support upload progress events.

Do we need this last comment?

> Source/WebCore/xml/XMLHttpRequest.cpp:622
> +            return { };

As per spec, we should check for m_state and m_sendFlag, but I am not clear why we are checking for m_loader?

> Source/WebCore/xml/XMLHttpRequest.cpp:-679
> -    m_state = UNSENT;

Above it is stated "// Clear headers as required by the spec", which is no longer the case.
Maybe we can get rid of this comment or the spec is missing something.
If we add a header, abort and add the same header, would the values be combined?

> LayoutTests/ChangeLog:14
> +        per specification and those tests were also failing in Firefox.

I wonder whether these 3 tests bring actually anything compared to their WPT equivalent.

[Chris Dumez]

Comment on attachment 336725 [details]
Patch

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

>> Source/WebCore/xml/XMLHttpRequest.cpp:338
>> +    m_method = normalizeHTTPMethod(method);
> 
> We can probably normalize the method after the early returns, at the moment the spec says to set m_method.

Good idea.

>> Source/WebCore/xml/XMLHttpRequest.cpp:381
>>          changeState(OPENED);
> 
> changeState is checking for m_state so we can probably call changeState directly, name is not great though.

Ok.

>> Source/WebCore/xml/XMLHttpRequest.cpp:591
>> +    // Also, only async requests support upload progress events.
> 
> Do we need this last comment?

I'll drop it.

>> Source/WebCore/xml/XMLHttpRequest.cpp:622
>> +            return { };
> 
> As per spec, we should check for m_state and m_sendFlag, but I am not clear why we are checking for m_loader?

I had to add it or some WPT tests would still fail when:
1. We dispatch the loadstart event (above)
2. In the loadstart event handler, the JS calls both open() and send() again

Because the JS has called open() and send() again, m_state is OPENED and m_sendFlag is true. open() normally cancels any pending load but we have no started the load yet (it happens a few lines below). Adding the m_loader check fixes the issue as we return early when we've re-entered and started a new load.

>> Source/WebCore/xml/XMLHttpRequest.cpp:-679
>> -    m_state = UNSENT;
> 
> Above it is stated "// Clear headers as required by the spec", which is no longer the case.
> Maybe we can get rid of this comment or the spec is missing something.
> If we add a header, abort and add the same header, would the values be combined?

I'll check.

[Chris Dumez]

Comment on attachment 336725 [details]
Patch

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

>> LayoutTests/ChangeLog:14
>> +        per specification and those tests were also failing in Firefox.
> 
> I wonder whether these 3 tests bring actually anything compared to their WPT equivalent.

My understanding is that we are not actively trying to avoid duplication between layout tests and WPT?

[Chris Dumez]

Comment on attachment 336725 [details]
Patch

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

>>> Source/WebCore/xml/XMLHttpRequest.cpp:622
>>> +            return { };
>> 
>> As per spec, we should check for m_state and m_sendFlag, but I am not clear why we are checking for m_loader?
> 
> I had to add it or some WPT tests would still fail when:
> 1. We dispatch the loadstart event (above)
> 2. In the loadstart event handler, the JS calls both open() and send() again
> 
> Because the JS has called open() and send() again, m_state is OPENED and m_sendFlag is true. open() normally cancels any pending load but we have no started the load yet (it happens a few lines below). Adding the m_loader check fixes the issue as we return early when we've re-entered and started a new load.

This is the test that would fail:
imported/w3c/web-platform-tests/XMLHttpRequest/loadstart-and-state.html

Diff:
-PASS open() during loadstart 
+FAIL open() during loadstart assert_equals: expected "BB-8" but got "R2-D2"

This test is passing in both Firefox and Chrome.

[youenn fablet]

> > I wonder whether these 3 tests bring actually anything compared to their WPT equivalent.
> 
> My understanding is that we are not actively trying to avoid duplication
> between layout tests and WPT?

That makes sense for tests that continue passing.
When a test is obsolete, we should either think of modifying it or removing it if superseded by another test.

I looked at http/tests/xmlhttprequest/onloadend-event-after-abort.html and it is not exactly a copy so they might still be useful.

> >> Source/WebCore/xml/XMLHttpRequest.cpp:622
> >> +            return { };
> > 
> > As per spec, we should check for m_state and m_sendFlag, but I am not clear why we are checking for m_loader?
> 
> I had to add it or some WPT tests would still fail when:
> 1. We dispatch the loadstart event (above)
> 2. In the loadstart event handler, the JS calls both open() and send() again
> 
> Because the JS has called open() and send() again, m_state is OPENED and
> m_sendFlag is true. open() normally cancels any pending load but we have no
> started the load yet (it happens a few lines below). Adding the m_loader
> check fixes the issue as we return early when we've re-entered and started a
> new load.

That makes sense, maybe that is worth a comment.

[Chris Dumez]

Created attachment 336750 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 336750 [details]
Patch

Clearing flags on attachment: 336750

Committed r230066: <https://trac.webkit.org/changeset/230066>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38985134>