Heribert Slama Reporter
	Description • 10 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140917194002

Steps to reproduce:

Go to URL http://downloadcenter.samsung.com/content/MC/201303/20130327211500937/EN/Eng/start_here.html (User Manual for tablet Samsung Galaxy Note 8.0). User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0.


Actual results:

User Manual's table of contents (panel on left-hand side) displays OK but content area (large panel on right-hand side) shows raw HTML preceded by 2 question marks (0x3f). See attachment Galaxy_Manual_Firefox.jpg


Expected results:

Content area should look as rendered by Google Chrome - see attachment Galaxy_Manual_Chrome.jpg

	Heribert Slama Reporter
	Comment 2 • 10 years ago

Note 1: Not sure about Version: 9 Branch and other keywords; I'm not a developer.
Note 2: The error also occurs with Firefox for Android (32.0.1) on tablet Samsung Galaxy Note 8.0 (GT-N5110) with Android 4.4.2. Chrome for Android renders properly.

	Matthias Versen [:Matti]
	Comment 3 • 10 years ago

I can confirm this with Firefox32

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 4 • 10 years ago

This is not an HTML parser bug.

The site uses jQuery's "ajax" function to retrieve the HTML as XHR's responseText. The HTML is little-endian UTF-16 with BOM.

For some reason that I haven't figured out at this time, XHR decodes the response as a rough ASCII superset instead of sniffing the BOM and decoding as UTF-16.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 5 • 9 years ago

Hallvors, could you check if there's wpt tests for this?

	Hallvord R. M. Steen [:hallvors]
	Comment 6 • 9 years ago

I reported this - thanks: https://github.com/w3c/web-platform-tests/issues/1586

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 7 • 8 years ago

Also a problem with UTF-8: see bug 1193947.

	Thomas Wisniewski Assignee
	Comment 10 • 7 years ago

I have a simple patch in the works which has XHRs sniff for a BOM and change decoders accordingly, before they actually process the response.

However, it's still not letting Firefox pass one of the tests in the WPT responsetext-decoding.htm, the one named: XMLHttpRequest: responseText decoding (text/plain %C2)

I'm honestly not sure how to fix that, nor if it's even related to this specific bug. Thoughts, Anne?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 11 • 7 years ago

(In reply to Thomas Wisniewski from comment #10)
> I have a simple patch in the works which has XHRs sniff for a BOM and change
> decoders accordingly, before they actually process the response.

Does XHR expose detail that makes the built-in morphing of mozilla::Decoder according to BOM not suffice?

	Anne (:annevk)
	Comment 12 • 7 years ago

Thomas, I think that might be due to XMLHttpRequest not properly signaling EOF. I thought Henri fixed that, but maybe not for all callers?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 13 • 7 years ago

(In reply to Anne (:annevk) from comment #12)
> Thomas, I think that might be due to XMLHttpRequest not properly signaling
> EOF. I thought Henri fixed that, but maybe not for all callers?

I've made both the EOF and the BOM bugs in XHR fix*able*, but I haven't fixed the XHR callers.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 14 • 7 years ago

(In reply to Thomas Wisniewski from comment #10)
> I'm honestly not sure how to fix that, nor if it's even related to this
> specific bug. Thoughts, Anne?

Anne is right that that's about EOF. XHR needs to signal the EOF to the decoder, and the decoder will do the right thing.

	Thomas Wisniewski Assignee
	Comment 15 • 7 years ago

I just tried adding this to the XHR OnStopRequest handler, but no dice (the same three failures still happen on the web platform test):

>  // Signal EOF to mDecoder
>  if (mDecoder) {
>    Span<uint8_t> src; // empty span
>    nsString dst;
>    dst.SetLength(1024);
>    uint32_t result;
>    size_t read;
>    size_t written;
>    bool hadErrors;
>    Tie(result, read, written, hadErrors) = mDecoder->DecodeToUTF16(src, dst, true);
>    Unused << hadErrors;
>    MOZ_ASSERT(read == 0, "How come an empty span was read form?");
>  }

Assuming that's sufficient to send the EOF signal, then something else must be tripping up the decoder. Thoughts?

	Thomas Wisniewski Assignee
	Comment 16 • 7 years ago

I figured I might as well post my patch here just in case.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 17 • 7 years ago

(In reply to Thomas Wisniewski from comment #15)
> I just tried adding this to the XHR OnStopRequest handler, but no dice (the
> same three failures still happen on the web platform test):
> 
> >  // Signal EOF to mDecoder
> >  if (mDecoder) {
> >    Span<uint8_t> src; // empty span
> >    nsString dst;
> >    dst.SetLength(1024);
> >    uint32_t result;
> >    size_t read;
> >    size_t written;
> >    bool hadErrors;
> >    Tie(result, read, written, hadErrors) = mDecoder->DecodeToUTF16(src, dst, true);
> >    Unused << hadErrors;
> >    MOZ_ASSERT(read == 0, "How come an empty span was read form?");
> >  }
> 
> Assuming that's sufficient to send the EOF signal, then something else must
> be tripping up the decoder. Thoughts?

That should make the decoder process the eof, but your code quote doesn't show anything done with dst afterwards. Are you saying dst doesn't end up with a REPLACEMENT CHARACTER in it on the test case that has a single non-ASCII byte labeled as UTF-8?

(In reply to Thomas Wisniewski from comment #16)
> Created attachment 8912078 [details] [diff] [review]
> process_bom_in_xhrs.diff
> 
> I figured I might as well post my patch here just in case.

The patch seems to manage BOM sniffing manually unnecessarily. That is, I don't see the manual sniffing interacting with the XML declaration or anything like that.

If you do mDecoder = encoding->NewDecoder();, mDecoder will handle sniffing for you. After mDecoder has seen 3 or more bytes, mDecoder->Encoding() is guaranteed to return the post-sniffing encoding if you need to expose it.

	Thomas Wisniewski Assignee
	Comment 18 • 7 years ago

Ah, thanks, I was clearly too tired last night to really think my code through. The WPT sending just a %C2 character does indeed pass when I add this code-block to OnStopRequest instead:

>  if (mDecoder) {
>    Span<uint8_t> src; // empty span
>    XMLHttpRequestStringWriterHelper helper(mResponseText);
>    for (;;) {
>      if (!helper.AddCapacity(1024)) {
>        return NS_ERROR_OUT_OF_MEMORY;
>     }
>     auto dst = MakeSpan(helper.EndOfExistingData(), 1024);
>      uint32_t result;
>      size_t read;
>      size_t written;
>      bool hadErrors;
>      Tie(result, read, written, hadErrors) =
>        mDecoder->DecodeToUTF16(src, dst, true);
>      Unused << hadErrors;
>      MOZ_ASSERT(read == 0, "How come an empty span was read from?");
>      helper.AddLength(written);
>      if (result != kOutputFull) {
>        break; 
>      }
>    } 
>  }   

Unfortunately, it still doesn't pass the case when a UTF-16BE/LE BOM is sent with no other characters (or is sent twice/doubled, like %FE%FF%FE%FF). The stream ends up getting %FD%FD instead of the BOM being stripped (or twice that if the BOM was doubled). That is, it's picking UTF-8, even if I start it off with the correct encoding.

But my patch passes these cases, so I'm not sure what's going on here. Any hints?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 19 • 7 years ago

(In reply to Thomas Wisniewski from comment #18)
> >     auto dst = MakeSpan(helper.EndOfExistingData(), 1024);

1024 seems excessive. mDecoder->MaxUTF16BufferLength(src.Length()) should be much less than 1024. (The argument of MaxUTF16BufferLength() is misleadingly named in Encoding.h; I should fix that.)

> Unfortunately, it still doesn't pass the case when a UTF-16BE/LE BOM is sent
> with no other characters (or is sent twice/doubled, like %FE%FF%FE%FF). The
> stream ends up getting %FD%FD instead of the BOM being stripped (or twice
> that if the BOM was doubled). That is, it's picking UTF-8, even if I start
> it off with the correct encoding.

How do you instantiate mDecoder? (Should be encoding->NewDecoder().)

	Thomas Wisniewski Assignee
	Comment 20 • 7 years ago

>Should be encoding->NewDecoder()

Ah, that was it, thanks! (I did try that, but it turns out there were two places where we instantiate a decoder in the XHR code, and I had only changed one of them for my testing).

I'll get a patch ready ASAP.

	Thomas Wisniewski Assignee
	Comment 21 • 7 years ago

Actually, I decided to skip writing all of that code in favor of just calling the existing XHR function AppendToResponseText which reads from the decoder (but changing that function to send DecodeToUTF16 aLast=true when given an empty source buffer as input).

That does the trick, and makes us start passing a good number of web platform tests. Curiously, two tests in encoding/unsupported-encodings.html still don't pass, and I'm not sure why:

UTF-32be with BOM should decode as windows-1252
utf-32be with BOM should decode as windows-1252

Meanwhile, the analogous tests with no BOM or with UTF-32le *do* pass:

UTF-32be with no BOM should decode as windows-1252
utf-32be with no BOM should decode as windows-1252
UTF-32LE with BOM should decode as UTF-16LE
utf-32le with BOM should decode as UTF-16LE

I'm guessing we can figure those cases out in a follow-up bug, however.

	Thomas Wisniewski Assignee
	Comment 24 • 7 years ago

Alright, sent my patch up for review; it seems to pass try just fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89c3acb2372a4e6229e7e78622aa973f68d284b0

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 25 • 7 years ago
mozreview-review

Comment on attachment 8914573 [details]
Bug 1070763 - Ensure that XHRs sniff the BOM for non-JSON responseTypes, and flush the decoder upon end-of-stream;

https://reviewboard.mozilla.org/r/185910/#review190926

r+ provided that the EOF signaling is made explicit.

::: dom/xhr/XMLHttpRequestMainThread.cpp:569
(Diff revision 1)
>    size_t written;
>    bool hadErrors;
>    Tie(result, read, written, hadErrors) = mDecoder->DecodeToUTF16(
>      AsBytes(MakeSpan(aSrcBuffer, aSrcBufferLen)),
>      MakeSpan(helper.EndOfExistingData(), destBufferLen.value()),
> -    false);
> +    aSrcBufferLen == 0);

I don't see enough assurances that Necko won't report a zero-length segment in the middle of the stream. Please add `bool aLast = false` as an argument of `AppendToResponseText` and pass `true` explicitly on EOF instead of inferring it from the input length.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 26 • 7 years ago

(In reply to Thomas Wisniewski from comment #21)
> Curiously, two tests in encoding/unsupported-encodings.html
> still don't pass, and I'm not sure why:
> 
> UTF-32be with BOM should decode as windows-1252
> utf-32be with BOM should decode as windows-1252

What do those decode as?

> I'm guessing we can figure those cases out in a follow-up bug, however.

OK.

	Thomas Wisniewski Assignee
	Comment 29 • 7 years ago

Ah, good idea to just pass an explicit parameter. Done.

Requesting checkin.

	Pulsebot
	Comment 30 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4bac27faa19b
Ensure that XHRs sniff the BOM for non-JSON responseTypes, and flush the decoder upon end-of-stream; r=hsivonen

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 31 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4bac27faa19b

	Thomas Wisniewski Assignee
	Comment 32 • 7 years ago

KWierso, I'd like to request backing this patch out, as it is causing unexpected crashes (see bug 1405571) and I can't be sure whether the fix will be trivial.

	Thomas Wisniewski Assignee
	Comment 33 • 7 years ago

Actually, I think we did find the fix, so let's not back it out after all.

	Gian-Carlo Pascutto [:gcp]
	Comment 34 • 7 years ago

The fix seems to have broken the internet :(

	Thomas Wisniewski Assignee
	Comment 35 • 7 years ago

Yeah, I can only apologize for the bad timing (I didn't expect this much fallout to happen while I was on PTO).

The good news is that it seems like :baku may have already found the fix to unbreak things in bug 1407573.

	Pulsebot
	Comment 36 • 7 years ago

Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/f3da88fdee4a
Backed out changeset 4bac27faa19b for crashes and broken websites. r=backout a=backout

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 37 • 7 years ago

Backed out bug 1070763 and bug 1405571 for crashes and broken websites:

bug 1070763
https://hg.mozilla.org/mozilla-central/rev/f3da88fdee4ab79a2212f8d2d1efd37fb5009b58

bug 1405571
https://hg.mozilla.org/mozilla-central/rev/32fdad6bf6f0417d9b094a13e0670ddb76a085b8

See bug 1405571 and the dependencies (broken websites).

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 38 • 7 years ago

It seems to me that a relanding should include a robust fix for the root cause of the CORS proxy causing double calls to OnStopRequest.

	Mike Taylor [:miketaylr]
	Comment 41 • 7 years ago

I just duped Bug 1405571 (crash fallout from this patch landing) against this bug. Please ensure the issues raised there are addressed in the re-landing, thanks.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 42 • 7 years ago

Marking this affected for 58 based on bug 1405571.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 43 • 7 years ago

Tracking this for 58 since it seems complex and I am coming across duplicates.

	Thomas Wisniewski Assignee
	Comment 44 • 7 years ago

I don't think it's necessary to track this one, since it's an old bug that hasn't been causing any major issues that I'm aware of (the real issues began when I tried to fix this, causing bug 1405571, which has been resolved for 57).

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 45 • 7 years ago

OK, I'll leave it to you then. Thanks!

	Firefox Bug Husbandry Bot
	Comment 46 • 6 years ago

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Update_affecteds

	Thomas Wisniewski [:twisniewski]
	Comment 47 • 6 years ago

So I've deduced that the failure which caused this to be backed out in bug 1405571 is *not* caused by OnStopRequest being called multiple times, but rather because of these early returns in OnStartRequest:

This can happen when any of the following early-returns happen in OnStartRequest, because mDecoder is only cleared after

>NS_IMETHODIMP
>XMLHttpRequestMainThread::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
>{ 
>  // snip for brevity
> 
>  if (request != mChannel) {
>    // Can this still happen?
>    return NS_OK;
>  }
> 
>  // Don't do anything if we have been aborted
>  if (mState == XMLHttpRequest_Binding::UNSENT) {
>    return NS_OK;
>  }
> 
>  /* Apparently, Abort() should set UNSENT.  See bug 361773.
>     XHR2 spec says this is correct. */
>  if (mFlagAborted) {
>    NS_ERROR("Ugh, still getting data on an aborted XMLHttpRequest!");
> 
>    return NS_ERROR_UNEXPECTED;
>  }
> 
>  // Don't do anything if we have timed out.
>  if (mFlagTimedOut) {
>    return NS_OK;
>  }

When you realize that mDecoder is only set later in this function (during the call to DetectCharset), the reason for the crash becomes obvious: if an XHR object is re-used after hitting one of these early returns, it will have a stale mDecoder from its previous request. And of course, re-using that decoder rightly triggers the crash.

The fix for this is trivial; just nullify mDecoder during each call to open().

Unfortunately, engineering a test for the first early-return above is beyond me, but I managed to come up with a tests for the other three (I wanted to use crashtests instead of mochitests, but have no idea how to allow a crashtest to hit the network, which is necessary to trigger these cases as data, blob and file URLs won't ever hit these cases). Alternatively, I could move them into a new web platform test instead, but I'm not sure if that's the right thing to do for crashtests.

I'm doing a try-run with my rebased patch here to see what else I may have missed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f0df0841d7bcf8cf5e8a17c89b8ddfea2939033

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 48 • 6 years ago

Using wpt is fine.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 49 • 6 years ago

This bug makes us look bad on http://w3c-test.org/encoding/unsupported-encodings.any.html , because the test harness uses XHR for testing BOM handling.

	Thomas Wisniewski [:twisniewski]
	Comment 50 • 6 years ago

Henri, I could use a little help here. I'm unable to grasp why my revised patch is failing the final test in the (recently-added) WPT: http://w3c-test.org/xhr/overridemimetype-edge-cases.window.html

The failure message is:

>assert_equals: expected "\ufffd" but got "\ufffd\ufffd"

It's doing an XHR to:

>const testURL = "resources/status.py?type=" + encodeURIComponent("text/plain;charset=windows-1252") + "&content=%C2%F0";

The UTF-8 decoder is being selected as expected, and indeed only 2 chars are being sent to it in AppendToResponseText, 0xC2 and 0xF0.

But the call to UTF8Decoder->DecodeToUTF16 is converting that input into two replacement characters instead of the one the test expects. (For reference, the call is also returning hadError=true, and written=1).

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 51 • 6 years ago

Why does the patch make XHR do its own BOM sniffing instead of letting mozilla::Decoder do it?

> The UTF-8 decoder is being selected as expected, and indeed only 2 chars are being sent to it in AppendToResponseText, 0xC2 and 0xF0.

The test looks wrong. 0xC2 and 0xF0 are both UTF-8 lead bytes. There are fewer REPLACEMENT CHARACTERs than bogus bytes if there are some UTF-8 trail bytes following a lead byte such that the bogus sequence forms a prefix of what would be a valid sequence. In this case 0xC2, 0xF0 is not a prefix of a valid sequence. 0xC2 is a prefix and 0xF0 is another prefix, so there are two REPLACEMENT CHARACTERs.

	Thomas Wisniewski [:twisniewski]
	Comment 52 • 6 years ago

>Why does the patch make XHR do its own BOM sniffing instead of letting mozilla::Decoder do it?

It does... I guess you mean my commit message is too vague? (All the patch really does is change to using a non-BOM-stripping Decoder, call its DecodeToUTF16 one last time when the stream is over with aLast=true, and add a line to reset the decoder on open()).

>The test looks wrong.

Ah, ok, that makes sense to me. Anne, I'll change the test as part of this patch so it expects the correct number of REPLACEMENT CHARACTERS, unless you feel that's not the way to go?

	Anne (:annevk)
	Comment 53 • 6 years ago

Sounds good to me, thanks for cleaning this up! (I suspect the test ended up being wrong here due to EOF decoding handling only being recently fixed in Firefox and not yet in other implementations.)

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 54 • 6 years ago

(In reply to twisniewski from comment #52)
> >Why does the patch make XHR do its own BOM sniffing instead of letting mozilla::Decoder do it?
> 
> It does... I guess you mean my commit message is too vague?

I was looking at the wrong attachment. I was looking at attachment 8912078 [details] [diff] [review].

	Thomas Wisniewski [:twisniewski]
	Comment 56 • 6 years ago

Alright, I've revised the WPT in this version of the patch. A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63a0269304f14a831e785d8cbaecd5841dac4e2a

That said, I could not push a fresh review to MozReview (no permission to re-open the review there), so I decided to try this fancy new Phabricator thing we have going on now.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 57 • 6 years ago

Comment on attachment 8999002 [details]
Bug 1070763 - Ensure that XHRs sniff the BOM for non-JSON responseTypes, and flush the decoder upon end-of-stream; r?hsivonen

Henri Sivonen (:hsivonen) has approved the revision.

	Thomas Wisniewski [:twisniewski]
	Comment 58 • 6 years ago

Thanks Henri! Requesting check-in. Sheriffs, please note that the Phabricator patch is the one intended to land; there was an earlier MozReview one which is now stale, and had been backed out.

	Pulsebot
	Comment 59 • 6 years ago

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b97d3b93472
Ensure that XHRs sniff the BOM for non-JSON responseTypes, and flush the decoder upon end-of-stream; r=hsivonen

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 60 • 6 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12411 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 61 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/12411
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/414556015?utm_source=github_status&utm_medium=notification)

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 62 • 6 years ago

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 63 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8b97d3b93472

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 64 • 6 years ago

Upstream PR merged