Closed Bug 737966 Opened 10 years ago Closed 9 years ago
IXMLHttp Request .response Text throws on certain parsing errors
Tb feed code does the following: this.request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] .createInstance(Ci.nsIXMLHttpRequest); this.request.onprogress = this.onProgress; this.request.open("GET", this.url, true); .. this.request.overrideMimeType("text/xml"); this.request.onload = this.onDownloaded; this.request.onerror = this.onDownloadError; this.request.send(null); using an invalid feed url, like http://dilbert.com/rss/, returns a parsing error doc in .responseXML, but request.response|responseText throw on evaluation. this does not happen in Tb11, but does in Tb12 on. .status is 200. Error: Component returned failure code: 0x80500001 [nsIXMLHttpRequest.responseText] = <unknown> Source file: chrome://messenger-newsblog/content/Feed.js Line: 327 the feed code probably doesn't need to test .responseText and should be asking for responseType=document anyway, but surely the properties should be set to null and not throw. likely this is a common way to test..
This looks like a regression from bug 590390. dilbert.com has an invalid charset name "utf-8lias" in response headers. Before bug 590390, TryChannelCharset updates aCharset only when nsCharsetAlias::GetPreferred has succeeded. Note that nsCharsetAlias::GetPreferred will truncate the parameter if it failed. https://hg.mozilla.org/mozilla-central/diff/2893c5e0be5d/content/base/src/nsDocument.cpp nsXMLHttpRequest::GetResponseText will fail if the document charset is the empty string.
Requesting tracking, since this would be a web-visible regression. I wonder what the spec says to do here and whether it needs changing....
Simply ignoring the channel charset seems like the right thing to do here. That's our previous behavior, right?
We used to return "UTF-8" because document charset was initialized "UTF-8" and no body broke that. https://mxr.mozilla.org/mozilla-central/source/content/xml/document/src/nsXMLDocument.cpp#177 I think GetPreferred shouldn't break the parameter on failure.
Could you add a comment explaining why temporary variable (preferred) is required?
Comment on attachment 609627 [details] [diff] [review] fix Review of attachment 609627 [details] [diff] [review]: ----------------------------------------------------------------- Though would be great if you could add a mochitest as well to test that things work correctly in a webpage context.
Attachment #609627 - Flags: review?(jonas) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/da10fe58893d also, I file bug 742259 for more test.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
[Triage Comment] Sounds like a low risk fix - please nominate for Aurora 13 uplift as soon as possible for this regression.
Makoto: Does this patch apply to the aurora branch? If so, can you set the approval‑mozilla‑aurora flag for the patch to "?"
[Approval Request Comment] Regression caused by (bug #): Bug 590390 User impact if declined: This is regression by bug 590390. After landing bug 590390, defalut charset that has no charset in HTTP header isn't UTF-8. It should keeps UTF-8 as default for compatibility of older version of Firefox. Testing completed (on m-c, etc.): Yes. It has xpcshell test. Risk to taking this patch (and alternatives if risky): Low. This is regression. String changes made by this patch: No.
Attachment #613484 - Flags: approval-mozilla-aurora?
Comment on attachment 613484 [details] [diff] [review] rebase for aurora [Triage Comment] Low risk fix to a regression in FF13. Approved for Aurora.
Attachment #613484 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.