Closed Bug 1437360 Opened 2 years ago Closed 1 year ago

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

Categories

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

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: paul, Assigned: twisniewski)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file)

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.
Component: Untriaged → Developer Tools: Console
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 ?
Flags: needinfo?(paul)
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.
Flags: needinfo?(paul)
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.
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 ?
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 ..
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.
Product: Firefox → DevTools
Priority: -- → P4
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.
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.
Component: Console → Networking
Product: DevTools → Core
Priority: P4 → P2
Whiteboard: [necko-triaged]

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

(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.

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

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.

Assignee: nobody → twisniewski
Pushed by twisniewski@mozilla.com:
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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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?

Component: Networking → DOM: Networking
Flags: needinfo?(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).

Flags: needinfo?(twisniewski)

(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.

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

Thanks for the notice! Glad it was figured out.

Duplicate of this bug: 1550384
You need to log in before you can comment on or make changes to this bug.