Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Evaluating nsIXMLHttpRequest.responseText throws on certain parsing errors

RESOLVED FIXED in Firefox 13

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: alta88, Assigned: m_kato)

Tracking

({regression})

unspecified
mozilla14
x86_64
Windows 7
regression
Points:
---

Firefox Tracking Flags

(firefox13+ fixed, firefox14+ fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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..

Comment 1

5 years ago
Jonas?
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

Comment 3

5 years ago
Requesting tracking, since this would be a web-visible regression.

I wonder what the spec says to do here and whether it needs changing....
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
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.

Updated

5 years ago
tracking-firefox13: ? → +
tracking-firefox14: ? → +
(Assignee)

Updated

5 years ago
Assignee: nobody → m_kato
(Assignee)

Comment 6

5 years ago
Created attachment 609627 [details] [diff] [review]
fix
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 9

5 years ago
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
Last Resolved: 5 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 "?"
(Assignee)

Comment 13

5 years ago
Created attachment 613484 [details] [diff] [review]
rebase for aurora

[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+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/fbd93a3a58f5
status-firefox13: --- → fixed
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.