1733686 - Crash in [@ mozilla::mozHunspellCallbacks::GetLine]

Status: RESOLVED FIXED

(Core :: Spelling checker, defect, P2)

Description

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Crash report: https://crash-stats.mozilla.org/report/index/56a1a777-ff81-4f3a-a7f4-bf66f0211001

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(t_line)

Top 10 frames of crashing thread:

0 xul.dll static mozilla::mozHunspellCallbacks::GetLine extensions/spellcheck/hunspell/glue/mozHunspellRLBoxHost.cpp:153
1 xul.dll static rlbox::rlbox_sandbox<rlbox::rlbox_wasm2c_sandbox>::sandbox_callback_interceptor<bool, unsigned int, char**> third_party/rlbox/include/rlbox_sandbox.hpp:273
2 xul.dll static rlbox::rlbox_wasm2c_sandbox::callback_interceptor<1, bool, unsigned int, unsigned int> third_party/rlbox_wasm2c_sandbox/include/rlbox_wasm2c_sandbox.hpp:310
3 rlbox.dll w2c_FileMgr__getline_std____2__basic_string_char__std____2__char_traits_char___std____2__allocator_char_____ security/rlbox/rlbox.dll.wasm.c:33149
4 rlbox.dll w2c_HashMgr__load_tables_char_const___char_const__ security/rlbox/rlbox.dll.wasm.c:82026
5 rlbox.dll w2c_HashMgr__HashMgr_char_const___char_const___char_const__ security/rlbox/rlbox.dll.wasm.c:81066
6 rlbox.dll w2c_HunspellImpl__HunspellImpl_char_const___char_const___char_const__ security/rlbox/rlbox.dll.wasm.c:90836
7 rlbox.dll w2c_Hunspell_create security/rlbox/rlbox.dll.wasm.c:109083
8 xul.dll RLBoxHunspell::RLBoxHunspell extensions/spellcheck/hunspell/glue/RLBoxHunspell.cpp:78
9 xul.dll mozHunspell::SetDictionary extensions/spellcheck/hunspell/glue/mozHunspell.cpp:189

This is a release assert on the result of aSandbox.malloc_in_sandbox().

Comment 1

[Andrew McCreight [:mccr8] (PTO until 12-2)]

There were 8 of these crashes from 4 installations in the 20210929155324 build, but I don't know if that's just a fluke.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

All with Windows 32-bit. Regression from bug 1727266?

Comment 3

[Shravan Narayan]

investigating

Comment 4

[Shravan Narayan]

It looks like this bug is due to the hunspell rlbox sandbox running out of memory when processing the spell checking.

A couple of observations

• The likelihood that this happens on 32-bit platforms more than 64-bit is expected as the 32-bit platforms provide a lesser amount of memory per sandbox (8mb per sandbox currently) compared to sandboxes on 64-bit platforms (4gb per sandbox)
• It is surprising that this is seen only on windows 32. There should be no reason this is different from linux 32-bit. The only point of difference I can think of is that this code reads text files, and text files on windows end each line with "\r\n" instead of unix's "\n" making them slightly larger, but this is unlikely to be the culprit due to the small difference.

Despite the source of the issue, the next steps we have to take are identical

1. Try to repro the crashes by replicating the locales/platforms in the crash report (we tried this and so far no repro's found)
2. Land patch for bug 1734421 which is an unrelated bug (despite first appearances); however the fix for that bug happens to expands the 32-bit sandbox memory from 8mb to 16mb which will help here.
3. Land a patch that gracefully handles malloc failure and returns an error code instead of failing
4. Run additional tests to make sure there are no memory leaks within the sandboxed code that is causing this OOM

Comment 5

[Shravan Narayan]

Attachment: Bug 1733686 - Gracefully handle Hunspell rlbox sandbox OOM r=glandium

Comment 6

[Shravan Narayan]

Attachment: Bug 1733686 - Fix Hunspell memory cleanup and OOM handling r=glandium

Comment 7

[Shravan Narayan]

3. Land a patch that gracefully handles malloc failure and returns an error code instead of failing
4. Run additional tests to make sure there are no memory leaks within the sandboxed code that is causing this OOM

Confirmed that there is a memory leak in allocations made inside sandbox memory (point 4 above). While the memory leak is sandboxed, the sandbox eventually runs out of space.

The submitted phab handles both point 3 and point 4.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Bug 1727266 was backed out for 94.0b3, so hopefully we'll see these crashes go away there. Will call this fixed for 94 once we see that reflected in the crash reports.

Comment 9

[Shravan Narayan]

Attachment: Bug 1733686 part 1 - Update rlbox to support cleaner code patterns in libhunspell r=glandium

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/d65c4c465903
turn off sandboxing of spellchecker in 32-bit builds for causing frequent crashes. a=crash-fix DONTBUILD

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)

Bug 1727266 was backed out for 94.0b3, so hopefully we'll see these crashes go away there. Will call this fixed for 94 once we see that reflected in the crash reports.

No crashes in 94.0b3.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

This bug is now rather confusing. Some stuff was disabled, but not because of a patch in this bug, and then there are some other patches around.

Since this bug was apparently used for https://hg.mozilla.org/mozilla-central/rev/d65c4c465903, could the
rlbox fixes (which aren't really spellchecker related) moved to some new bug?

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See What Do You Triage for more information

Comment 15

[Shravan Narayan]

Attachment: Bug 1733686 part 3 - Re-enable hunspell rlbox sandbox on 32-bits platforms. r=glandium

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29ed3620fa91
part 1 - Update rlbox to support cleaner code patterns in libhunspell  r=glandium
https://hg.mozilla.org/integration/autoland/rev/d52827e69092
part 2 - Fix Hunspell memory cleanup and OOM handling r=glandium,deian
https://hg.mozilla.org/integration/autoland/rev/a6fef4dc35c2
part 3 - Re-enable hunspell rlbox sandbox on 32-bits platforms. r=glandium

Comment 17

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/29ed3620fa91
https://hg.mozilla.org/mozilla-central/rev/d52827e69092
https://hg.mozilla.org/mozilla-central/rev/a6fef4dc35c2

Comment 18

[Narcis Beleuzu [:NarcisB]]

Backed out for causing sandboxing crashes with the spellchecker (Bug 1736171)
Backout link: https://hg.mozilla.org/integration/autoland/rev/8ad4b1bf184499ef0550b1014f6e7326ed132750

Comment 19

[Marian-Vasile Laza]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/8ad4b1bf184499ef0550b1014f6e7326ed132750

Comment 20

[Phabricator Automation]

Comment on attachment 9246087 [details]
Bug 1733686 part 3 - Re-enable hunspell rlbox sandbox on 32-bits platforms. r=glandium

Revision D128571 was moved to bug 1737704. Setting attachment 9246087 [details] to obsolete.

Comment 21

[Phabricator Automation]

Comment on attachment 9244659 [details]
Bug 1733686 - Fix Hunspell memory cleanup and OOM handling r=glandium

Revision D127736 was moved to bug 1737740. Setting attachment 9244659 [details] to obsolete.

Comment 22

[Bobby Holley (:bholley)]

The specific issue that was causing these crashes was fixed in bug 1737740, and reenabling hunspell on 32-bit is tracked in bug 1737704.

I'm closing this bug out. Given the confusing history of this bug, please file new bugs for any further issues.