1437360 - XHR PUT of JSON comes with a XML parsing error (not on Safari or Chrome)

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Paul Hammant]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180128191252

Steps to reproduce:

In the JavaScript of a page, try to PUT a JSON resource to the server:

```
let str = JSON.stringify(obj, null, 2);
let xhr = new XMLHttpRequest();   // new HttpRequest instance
xhr.open("PUT", "/path/to/resource.json");
xhr.setRequestHeader("Content-type", "application/json; charset=UTF-8");
xhr.onload = function () {
   if (xhr.readyState === 4 && xhr.status === 200) {
       console.log("PUT worked")
   } else {
        console.log("PUT failed - " + xhr.status + " " + xhr.readyState + " " + xhr.responseText)
   }
};
xhr.send(str);
```

To reproduce you'll need a WebServer that can handle put without complaining. Good with Python? Here's one atop https://github.com/lwsjs/local-web-server/issues/83


Actual results:

The PUT works just fine according to the server which responds with a 200. The code path executed by Firefox is the "PUT worked" path. But after that is output to the console, there is:

```
XML Parsing Error: no root element found
Location: http://localhost:8000/path/to/resource.json
Line Number 1, Column 1:
```

On the right hand side in the console to indicate the line of code in error is:

```
resource.json:1:1
```


Expected results:

It should not have printed the XML error to the console. The resource is JSON not XML and why is it parsing it after a PUT?  

    From: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/PUT
    "Successful   response has body 	No"

The code works fine in Safari and Chrome and does not spit out the same post-PUT error message.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Hello,
I've tried to reproduce your issue by creating a test case: https://devtools-html-put-error.glitch.me/ (edit URL is https://glitch.com/edit/#!/devtools-html-put-error?path=server.js:31:18), but I can't see the error you experienced.

Am I missing something in my code to reproduce the error ?

Comment 2

[Paul Hammant]

Thanks for the effort. As you say this is not showing the error in my Firefox (so my particular FF and other WebExtensions/Plugins are not the variable).

I have other variables involved - like a Python dev-local webserver. And Vue.js although the snippet of code I shipped was vanilla XHR rather than more elegant 'Vue-router' logic.

I'll use your app as a starting point to in order to introduce things one by one towards a reproduction (and root cause via process of elimination). Or not of course. I may end up filing an issue elsewhere and updating this one to say so. I'd not seen glitch before - neat.

Comment 3

[Paul Hammant]

Yeah it is the kitchen-sink PUT-ized Python SimpleHttpServer that is the root cause. It handles and saves the payload just fine, but the response isn't enough - probably content length, or type or something else. I'll just switch to Node as a PUT handler for my "local development server".

Should I close this? 

Or is the problem statement different now: "In PUT situations, Firefox reports an XML error when it is actually incomplete response from the server" ?

Thanks again for your repro effort, Nicolas.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

I think we should keep the issue and rename it to accurately represent what it is.
Could you post the headers from the response so we know what's wrong in the response and can reproduce it ?

Comment 5

[Paul Hammant]

Firefox listed nothing at all for the response from the Python process. I'm going to have to crank up Charles (logging proxy) or similar to see what actually came back. Stand by ..

Comment 6

[Paul Hammant]

The sample PUT enhanced SimpleHttpServer needs to look like this to work:

```
import SimpleHTTPServer
import BaseHTTPServer

class SputHTTPRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler):
    def do_PUT(self):
        length = int(self.headers["Content-Length"])
        path = self.translate_path(self.path)
        if ".json" not in path:
            raise Exception("not allowed")
        with open(path, "wb") as dst:
            dst.write(self.rfile.read(length))
        self.send_response(200)
        self.send_header("Content-Type", "text/plain; charset=utf-8")

if __name__ == '__main__':
    SimpleHTTPServer.test(HandlerClass=SputHTTPRequestHandler)
```

The XML parsing error goes away. There was a missing content type (regardless of the absence of content)

If the content type line is commented out the XML error appears in Firefox (regardless of the absence of content). 

If the content type and '200' line are reordered then there is a different error - neither the 'PUT worked' or 'PUT failed' callback is invoked.

With just the two in the correct order, FF works fine. It doesn't wibble about a missing content length, or missing content. At least not in the console.

SeaMonkey has this issue too, so the issue is pre-quantum which probably doesn't surprise any of the veterans of the larger Moz codebase.

Comment 7

[joncfoo]

I've encountered this as well in a webapp I work on.  If Content-Type is missing Firefox attempts to parse the response as XML which results in the console error. The workaround for webapps is to send a Content-Type of "text/plain" but it'd be nice if Firefox did not attempt to parse the content like Safari and Chrome do.

Comment 8

[Benjamin Dobell]

Mostly just a "me too" post, but I thought it may be worth adding that this occurs with AWS S3 direct uploads. Specifically S3 will respond to a PUT file upload with 200 status code and "Content-Length: 0" without any Content-Type being included.

Comment 9

[Thayne]

I think that if Content-Length is zero, then firefox should not try to parse the body as JSON.

Comment 10

[Thayne]

(In reply to Thayne from comment #9)

I think that if Content-Length is zero, then firefox should not try to parse the body as JSON.

Oops, I meant XML not JSON.

Comment 11

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1437360 - do not attempt to parse XHRs as XML if content-length=0, to prevent logging "no root element found" errors; r?smaug

do not attempt to parse XHRs as XML if content-length=0, to prevent logging "no root element found" errors

Comment 12

[Thomas Wisniewski [:twisniewski]]

I'm not seeing anything worrying in my latest try-run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97918741df04c28dd807366d9921136ff3c1ec9d

Just one failure that reproduces locally for me, but this patch has no effect on.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b9aee567032
do not attempt to parse XHRs as XML if content-length=0, to prevent logging "no root element found" errors; r=smaug

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6b9aee567032

Comment 15

[Honza Bambas (:mayhemer)]

This shows in the likely regression range for crash bug 1538098 that involves POST/PUT request sending (hitting the code patch we crash on). We have already tried to back out few other patches that could cause that crash w/o success.

Can you think of this one being a possible cause?

Comment 16

[Thomas Wisniewski [:twisniewski]]

I'm certainly not against us backing this out to verify, but I don't see how it would affect that crash if it's related to requests (as this patch just tells us to not process XHR responses as XML).

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Thomas Wisniewski [:twisniewski] from comment #16)

I'm certainly not against us backing this out to verify, but I don't see how it would affect that crash if it's related to requests (as this patch just tells us to not process XHR responses as XML).

It's a very wild theory of mine, only reason I'm pinging you is that it's related to POST/PUT and in the reg range and we are running out of ideas.

Let's wait a bit how that crash evolves in couple next days. If we really got desperate, we may ask you for a backout.

Comment 18

[Honza Bambas (:mayhemer)]

Thomas, we have identified the problem, no need to back this bug out :) Sorry for the churn.

Comment 19

[Thomas Wisniewski [:twisniewski]]

Thanks for the notice! Glad it was figured out.