Last Comment Bug 737966 - Evaluating nsIXMLHttpRequest.responseText throws on certain parsing errors
: Evaluating nsIXMLHttpRequest.responseText throws on certain parsing errors
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Windows 7
-- normal (vote)
: mozilla14
Assigned To: Makoto Kato [:m_kato]
: Andrew Overholt [:overholt]
Depends on:
Blocks: 590390
  Show dependency treegraph
Reported: 2012-03-21 11:45 PDT by alta88
Modified: 2012-04-11 02:29 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (3.04 KB, patch)
2012-03-26 23:33 PDT, Makoto Kato [:m_kato]
jonas: review+
Details | Diff | Splinter Review
rebase for aurora (3.01 KB, patch)
2012-04-09 20:49 PDT, Makoto Kato [:m_kato]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image alta88 2012-03-21 11:45:22 PDT
Tb feed code does the following:

    this.request = Cc[";1"]
    this.request.onprogress = this.onProgress;"GET", this.url, true);
    this.request.onload = this.onDownloaded;
    this.request.onerror = this.onDownloadError;

using an invalid feed url, like, 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 User image Boris Zbarsky [:bz] (still a bit busy) 2012-03-21 20:40:40 PDT
Comment 2 User image Masatoshi Kimura [:emk] 2012-03-22 01:29:49 PDT
This looks like a regression from bug 590390. 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.
nsXMLHttpRequest::GetResponseText will fail if the document charset is the empty string.
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2012-03-22 07:08:45 PDT
Requesting tracking, since this would be a web-visible regression.

I wonder what the spec says to do here and whether it needs changing....
Comment 4 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-03-22 10:10:19 PDT
Simply ignoring the channel charset seems like the right thing to do here. That's our previous behavior, right?
Comment 5 User image Masatoshi Kimura [:emk] 2012-03-22 10:19:19 PDT
We used to return "UTF-8" because document charset was initialized "UTF-8" and no body broke that.
I think GetPreferred shouldn't break the parameter on failure.
Comment 6 User image Makoto Kato [:m_kato] 2012-03-26 23:33:08 PDT
Created attachment 609627 [details] [diff] [review]
Comment 7 User image Masatoshi Kimura [:emk] 2012-03-27 09:51:50 PDT
Could you add a comment explaining why temporary variable (preferred) is required?
Comment 8 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-03-27 10:03:36 PDT
Comment on attachment 609627 [details] [diff] [review]

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.
Comment 9 User image Makoto Kato [:m_kato] 2012-04-04 05:02:26 PDT

also, I file bug 742259 for more test.
Comment 11 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-04-09 15:51:21 PDT
[Triage Comment]
Sounds like a low risk fix - please nominate for Aurora 13 uplift as soon as possible for this regression.
Comment 12 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-04-09 16:24:50 PDT
Makoto: Does this patch apply to the aurora branch? If so, can you set the approval‑mozilla‑aurora flag for the patch to "?"
Comment 13 User image Makoto Kato [:m_kato] 2012-04-09 20:49:12 PDT
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:
Comment 14 User image Alex Keybl [:akeybl] 2012-04-10 12:18:43 PDT
Comment on attachment 613484 [details] [diff] [review]
rebase for aurora

[Triage Comment]
Low risk fix to a regression in FF13. Approved for Aurora.

Note You need to log in before you can comment on or make changes to this bug.