293046 - XMLHttpRequest.overrideMimeType() makes loading abort for non-XML

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Wladimir Palant]

It seems that XMLHttpRequest.overrideMimeType() has been only meant to set an
XML content type. If you use something like text/plain instead, NS_ERROR_FAILURE
is produced and loading is aborted (you can see it on the length of
XMLHttpRequest.responseText - it is something around 4096).

The problem: XMLHttpRequest is widely used for loading text data, not just XML.
One might use overrideMimeType('text/plain') to avoid useless parsing of an XML
document, or overrideMimeType('text/plain; charset=windows-1251') to set the
charset for a non-XML document. If you do this, in most cases you will only get
the first 4096 bytes of the document.

Testcase and patch with the next posts...

Comment 1

[Wladimir Palant]

Attachment: Testcase

Loads Bugzilla's main page after calling overrideMimeType(). This results in
the message: "Error: 4096". The real size of the page is somewhere around 7000
bytes, so one would expect the message: "Loaded: 7063".

Comment 2

[Wladimir Palant]

Attachment: Patch

This actually fixes two problems.

1. Loading isn't aborted anymore if mXMLParserStreamListener->OnDataAvailable()
returns an error. We stop parsing XML but finish reading the stream so that we
still have a valid result in responseText.
2. A call to overrideMimeType() doesn't always mean that we will get an XML
document. We still need to check the content type and determine whether we
should try to parse the document.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 182712 [details] [diff] [review]
Patch

-    rv = NS_NewByteInputStream(getter_AddRefs(copyStream), fromRawSegment,
count);
+    nsresult parsingResult = NS_NewByteInputStream(getter_AddRefs(copyStream),
fromRawSegment, count);

-    if (NS_SUCCEEDED(rv)) {
+    if (NS_SUCCEEDED(parsingResult)) {
       NS_ASSERTION(copyStream, "NS_NewByteInputStream lied");
-      rv =
xmlHttpRequest->mXMLParserStreamListener->OnDataAvailable(xmlHttpRequest->mRead
Request,xmlHttpRequest->mContext,copyStream,toOffset,count);
+      parsingResult =
xmlHttpRequest->mXMLParserStreamListener->OnDataAvailable(xmlHttpRequest->mRead
Request,xmlHttpRequest->mContext,copyStream,toOffset,count);
+    }
+
+    // No use to continue parsing if we failed here, but we
+    // should still finish reading the stream
+    if (NS_FAILED(parsingResult)) {
+      xmlHttpRequest->mState &= ~XML_HTTP_REQUEST_PARSEBODY;
     }

I think we should still abort the parsing if NS_NewByteInputStream() fails,
that will only fail if something goes really wrong and callers should know...
So just use parsingResult inside the first if statement above, and move the
second one inside the first one.

- In nsXMLHttpRequest::OnStartRequest():

+  nsresult status;
+  request->GetStatus(&status);
+
+  if (NS_SUCCEEDED(status) && !mOverrideMimeType.IsEmpty()) {
+    channel->SetContentType(mOverrideMimeType);
+  }
+
+  if (NS_SUCCEEDED(status)) {

Move the first if inside the second one to avoid double-checking
NS_SUCCEEDED(status) here.

r=jst with those changes made.

Comment 4

[Wladimir Palant]

Attachment: Updated and corrected patch

Requested changes made, taking review flags over from the old version.

Comment 5

[Darin Fisher]

Comment on attachment 190123 [details] [diff] [review]
Updated and corrected patch

Please break long lines of code at 80 characters for readability.

You should always put a space after a comma in the parameter list of a function
that you are calling.

sr=darin with those two nits fixed.

Comment 6

[Wladimir Palant]

(In reply to comment #5)
> Please break long lines of code at 80 characters for readability.
> 
> You should always put a space after a comma in the parameter list of a function
> that you are calling.

Heh... That line isn't really my code and it is by far not the last line with
bad style in this file. But I guess I can still fix it while we are already at it.

Comment 7

[Wladimir Palant]

Attachment: Patch to be checked in

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in extensions/xmlextras/base/src/nsXMLHttpRequest.cpp;
/cvsroot/mozilla/extensions/xmlextras/base/src/nsXMLHttpRequest.cpp,v  <-- 
nsXMLHttpRequest.cpp
new revision: 1.135; previous revision: 1.134
done

Comment 9

[Asa Dotzler [:asa]]

*** Bug 312482 has been marked as a duplicate of this bug. ***

Comment 10

[Asa Dotzler [:asa]]

Wladimir, Johnny, Darin, can you all tell us what kind of risk we'd be taking if
we tried to get this into the 1.8 branch? The bug duped against this was
nominated for blocking so I'm moving that over to this bug. 

Comment 11

[Wladimir Palant]

I voted against getting this into the 1.8 branch before as it seemed that the
probability of this becoming a real problem is rather low whereas this patch
changes the way how XMLHttpRequest works quite significantly. On the other hand,
the changes are pretty straightforward, we don't break anything anybody could
reasonably rely on and bug 312482 now shows a more obvious way to get into
trouble. In my opinion getting this into the 1.8 branch is reasonable but I'm
not experienced enough to have the last word here.

Comment 12

[Johnny Stenback (:jst)]

(In reply to comment #10)
> Wladimir, Johnny, Darin, can you all tell us what kind of risk we'd be taking if
> we tried to get this into the 1.8 branch?

I don't see much risk here at all, not by looking at the change (again) or for
the fact that there's no known regressions from this after 2 months on the
trunk. My vote would be to take this, but it's not an absolute showstopper IMO.

Comment 13

[Scott MacGregor]

Comment on attachment 190501 [details] [diff] [review]
Patch to be checked in

I think we're going to want to end up taking this patch based on the responses
I've read so far.

Comment 14

[Scott MacGregor]

Wladimir, can you get this landed on the branch today? If not, lemme know and
I'll land it for you. Thanks!

Comment 15

[Wladimir Palant]

I don't have CVS access, you have to do this.

Comment 16

[Scott MacGregor]

ok, i've checked this into the branch. Any testing help folks can provide in
tomorrow's builds would be greatly appreciated. 

Comment 17

[Shawn Neumann]

I'll check it out tomorrow.

Comment 18

[Shawn Neumann]

In terms of the issue in this bug, todays build now appears to work correctly. 
I tested with:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b5) Gecko/20051019 Firefox/1.5

and the result set was as expected.