1688368 - Crash in [@ chardetng::EncodingDetector::feed ] when charset is manually set to Unicode on files over 1024 bytes.

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Kengo Ide]

Attachment: testcase-1024-bytes

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:86.0) Gecko/20100101 Firefox/86.0

Steps to reproduce:

1. Open test case here: http://www.eonet.ne.jp/~kagari/testcase-1024-bytes
2. Check the menu -> More -> Text Encoding. Make sure that the browser's encoding detector picked an wrong charset as the default (anything other than UTF-8).
3. Set the charset to "Unicode".

Actual results:

The browser crashes in [@ chardetng::EncodingDetector::feed].

Crash Report: https://crash-stats.mozilla.org/report/index/874f028a-5eaa-42d4-a13b-0205a0210122

Environment: Windows 10 20H2; x86_64
System Locale: Japanese
Webpage Language Settings: ja, en-us, en

Expected results:

No crash occurs.

I have put the test case as an attachment to the bug as well, but I have not been able to reproduce the crash when the file is read from local filesystem. On my system, opening the local file makes the browser to choose the Unicode charset, rendering you unable to manually set another charset on UI.

Also, this crash doesn't happens on a similar file which is tweaked to have its size one byte less (1023 bytes).
1023 Bytes Testcase: http://www.eonet.ne.jp/~kagari/testcase-1023-bytes

The crash goes away by adding '.txt' to the file name, too.
http://www.eonet.ne.jp/~kagari/testcase-1024-bytes.txt

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Henri - can you triage?

Comment 2

[Henri Sivonen (:hsivonen)]

This code path is not supposed to call chardetng::EncodingDetector::feed. Let's find out why it does.

Comment 3

[Henri Sivonen (:hsivonen)]

Thanks for reporting!

The fix is a one-line change, but the strange part is that plugging the test case into our test harness doesn't repro the problem even when the local server pretends to be a .jp domain. I'll try to see why, but it's a possibility that I end up timing out on trying to land a test case for this.

Comment 4

[Henri Sivonen (:hsivonen)]

It appears that this requires the network buffer to be flushed to the parser once before the 1024 byte mark instead of the whole file arriving at once. That's probably why I didn't see this problem in the first place.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1688368 - Check mFeedChardet before feeding the detector.

Comment 6

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fab8c01a1804fae572f14ba5790f655b3413a9a

Comment 7

[Henri Sivonen (:hsivonen)]

[Tracking Requested - why for this release]:
User-triggerable recent crash with one-line fix.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70fa20a1aa2a
Check mFeedChardet before feeding the detector. r=emk

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/70fa20a1aa2a

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 9198981 [details]
Bug 1688368 - Check mFeedChardet before feeding the detector.

Beta/Release Uplift Approval Request

• User impact if declined: Crash when selecting an item other than "Automatic" from the Text Encoding menu when the document is at least 1024 bytes long and the network stack flushes the buffer to the parser at least once during the first 1024 bytes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We have a boolean indicating whether we should take an action. Without the patch, we at one point take the action without checking the boolean. With the patch, we check the boolean. It's pretty clear that we shouldn't take the action when our boolean indicating whether we should says not to.
• String changes made/needed: None

Comment 11

[Pascal Chevrel:pascalc]

Comment on attachment 9198981 [details]
Bug 1688368 - Check mFeedChardet before feeding the detector.

Crash fix, not many crashes but we are still in early betas, approved for 86 beta 3, thanks.

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/687522a89b85

Comment 13

[Bogdan Maris, Desktop QA]

Reproduced the initial crash using 86.0b1, verified that using latest Beta build 86.0b4 and latest Nightly from today Firefox does not crash anymore using the steps from comment 0 across platforms (Windows 10, Ubuntu 18.04 and MacOS 11).