Closed Bug 737966 Opened 8 years ago Closed 8 years ago

Evaluating nsIXMLHttpRequest.responseText throws on certain parsing errors

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
firefox13 + fixed
firefox14 + fixed

People

(Reporter: alta88, Assigned: m_kato)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
Blocks: 590390
Keywords: regression
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.
Assignee: nobody → m_kato
Attached patch fixSplinter Review
Attachment #609627 - Flags: review?(jonas)
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.
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/da10fe58893d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
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+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.