Bug 77854 – XMLHttpRequest status & statusText should be 0 & "" following an abort()

• Component: WebCore Misc.
• Status: RESOLVED DUPLICATE

[Mark Toller]

Apologies if this isn't on the correct component...

Here's the scenario:

A page creates an XMLHttpRequest for a file (there is a global variable 'req' which is set to the XMLHttpRequest).  The onreadystatechange handler contains the following code:

 function() {
        if (req.readyState !== 4 || req.status !== 200) {
            return;
        }
        var txt = req.responseText;
        req.onreadystatechange = null;
        configreq = null;
        eval('(' + txt + ')');
    };

There is also a plugin (NPAPI) running, which has a javascript function property 'onChannelChangeSucceeded'. This is set to the following code:

function() {
    if (req) {
        req.abort();
        req.onreadystatechange = null;
        req = false;
    }
}

It seems that the plugin onChannelChangeSucceeded can be called and execute the 'abort()' method whilst the onreadystatechange is being processed... i.e. the XMLHttpRequest completes, and starts processing the onreadstatechange handler, this passes the if(readyState || status) statement, but before the responseText is read into 'txt', the plugin onChannelChangeSucceeded is called, and this resets the XMLHttpRequest by calling 'abort()'.  The result is that the reponseText is then empty when read in the onreadystatechange handler.

I wouldn't expect the contents of the XMLHttpRequest to be changed *during* the execution of the onreadystatechange handler.

[Alexey Proskuryakov]

I also wouldn't expect that.

Do you have a test case that you could share?

[Mark Toller]

Created attachment 125801 [details]
HTML file to generate this bug using just XMLHttpRequest.

This HTML page (can be served as XHTML as well) will generate the bug on my setup, using ONLY XmlHttpRequests (no plugin required).
The file 'test.txt' this page retrieves simply contains the text "Bug 77854".
I ran this test on an Ubuntu Linux server running webkit-gtk (build 97042, a bit old I know), accessing an apache server on the same machine.

This test doesn't fail for Safari, Firefox, I.E. or Chrome (either from the Ubuntu server in the case of Chrome, or another PC for the others).

[Alexey Proskuryakov]

Thank you. Sounds like Gtk folks should take a look first then.

[Mark Toller]

Created attachment 169165 [details]
Updated test to simply reproduce the problem.

Simple page to reproduce the bug.

[Mark Toller]

Now I've had some time to actually debug this issue, it appears it is a duplicate of 54162 - or at least one portion (status and statusText) of 54162.

If the abort() occurs following HEADERS_RECEIVED, the status is set to 200, and then the state is changed to 'DONE', which makes the code successfully pass

if (req.readyState !== 4 || req.status !== 200) {
    return
}

passing this code expects the responseText to be available and correct, but it isn't.

[Mark Toller]

Created attachment 169167 [details]
Patch

[Build Bot]

Comment on attachment 169167 [details]
Patch

Attachment 169167 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14435002

New failing tests:
http/tests/xmlhttprequest/status-after-abort.html

[WebKit Review Bot]

Comment on attachment 169167 [details]
Patch

Attachment 169167 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14397620

New failing tests:
http/tests/xmlhttprequest/status-after-abort.html

[Alexey Proskuryakov]

Comment on attachment 169167 [details]
Patch

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

r- for lack of ChangeLog, test cases, and for broken regression test.

I do not understand how this Gtk-only bug has come to modify cross-platform behavior. Could you please explain that in more detail?

> Source/WebCore/xml/XMLHttpRequest.cpp:1024
> +    // From the http://www.w3.org/TR/XMLHttpRequest/#the-status-attribute:
> +    // If the error flag is set, return 0 and terminate these steps.

This comment is not very helpful. It just describes what the code below does, and provides a link to XHR spec.

First, anyone working on this code would know where the spec is anyway. But also, matching an ancient version of the spec will not be relevant five years from now. We make such behavior decisions based on multiple factors, including spec compliance, and our own compatibility testing. The desired behavior is captured in regression tests, not in code comments, unless there is something exceptionally interesting to say.

[Mark Toller]

When I first reported this bug, it was from a 'user' point of view, and I could only reproduce on GTK.  Now I've had a chance to actually debug it, I've found the real cause, which is common to all platforms, I've updated the test and can now reproduce on all platforms.

I've also changed the title to reflect the actual problem.

The failing testcase (http/tests/xmlhttprequest/status-after-abort.html) was created in http://trac.webkit.org/changeset/30665 5 years ago. At that time the XHR spec did no require the status and statusText to return 0 & "" on error (i.e. after an abort()), and the change was made to align with Firefox behaviour (?) at the time.

[Mark Toller]

Created attachment 169404 [details]
Patch

[Alexey Proskuryakov]

As you said, this looks like a duplicate of bug 54162. Why did you choose to continue work in a separate patch?

The difficult part in moving it forward is not just implementing what a spec says. We need to make sure that we don't break sites that users depend on. With over 80% mobile market share belonging to WebKit, any change like this has a big chance of being bad for users and for WebKit, because authors don't even test with other engines.

[Mark Toller]

(In reply to comment #12)
> As you said, this looks like a duplicate of bug 54162. Why did you choose to continue work in a separate patch?

A number of reasons actually:

1) This is a bug that actually occurred on our device in the field, so it's a fix we are likely to roll out.
2) It matches the spec, and also the behaviour of Opera re: status and statusText being 0 & "".
3) Internet Explorer throws (incorrectly I believe) an exception when accessing status or statusText after the abort() 
   in this case (but certainly doesn't return values prior to the abort() call).
4) The other bug (54162) tries to fix all outstanding issues to match the spec (throwing exceptions, etc), and 
   someone said it might be better to split and fix the issues seperately. As I was fixing this anyway for our SW, it 
   seemed like the sensible option.

> The difficult part in moving it forward is not just implementing what a spec says. We need to make sure that we don't break sites that users depend on. With over 80% mobile market share belonging to WebKit, any change like this has a big chance of being bad for users and for WebKit, because authors don't even test with other engines.

I can appreciate that. I'm coming from the view that this was reported to us as a defect. I can't see the 
use case for being able to check what status and statusText were before an abort() was called (after all, 
the application could check before calling abort()). Following the call to abort(), the data is no longer 
available, and signalling DONE and status == 200 can (and in our case did) cause the application handler to 
behave as if it had succeeded.

How *do* you go about checking that changes like this don't break sites users depend on?

[Mark Toller]

Any further thoughts on this patch?

[Anders Carlsson]

Comment on attachment 169404 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[youenn fablet]

This bug is handled by bug 45994 last patch (which goes a little bit beyond what its bug title says).
Once bug 45994 is closed, this bug should probably be closed as well.

[youenn fablet]

*** This bug has been marked as a duplicate of bug 45994 ***