Closed Bug 1688368 Opened 3 months ago Closed 2 months ago

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

Categories

(Core :: Internationalization, defect)

defect

Tracking

()

VERIFIED FIXED
87 Branch
Tracking Status
firefox86 + verified
firefox87 --- verified

People

(Reporter: kasumikagari, Assigned: hsivonen)

References

(Regression)

Details

Crash Data

Attachments

(2 files)

Attached file 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

Status: UNCONFIRMED → NEW
Crash Signature: [@ chardetng::EncodingDetector::feed]
Component: Untriaged → Internationalization
Ever confirmed: true
Product: Firefox → Core

Henri - can you triage?

Flags: needinfo?(hsivonen)

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

Assignee: nobody → hsivonen
Severity: -- → S2
Status: NEW → ASSIGNED
Flags: needinfo?(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.

Regressed by: 1648464

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.

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

Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70fa20a1aa2a
Check mFeedChardet before feeding the detector. r=emk
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch

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
Attachment #9198981 - Flags: approval-mozilla-beta?

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.

Attachment #9198981 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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).

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.