1726080 - Crash in [@ mozilla::TextServicesDocument::OffsetEntryArray::SplitElementAt]

Status: RESOLVED FIXED

(Core :: Spelling checker, defect, P3)

Description

[Geoff Lankow (:darktrojan)]

+++ This bug was initially created as a clone of Bug #1722748 +++

This crash is still happening in Thunderbird 92.0b2, where it should be fixed according to bug 1722748.

https://crash-stats.mozilla.org/report/index/94613668-d598-45c1-ae50-a1c1b0210817

Here's a STR which seems to work reliably:

• Reply to a plain-text message
• Add two spelling mistakes
• Click the spell check button
• Replace each mistake with a suggestion
• Crash on the second replacement

Comment 1

[Roland Studer]

bug still in 92.0 b2
stream
write a new message
call the spell checker
try to "replace" a word
results : freeze of thunderbird

Comment 2

[Daniel Bodea [:danibodea]]

It appears that the same crash signature was fixed for Firefox, but not for Thunderbird v92.0.
Changing the product and component to better differentiate the 2 bugs.

Comment 3

[WaltS48 [:walts48]]

Reproducible using TB 92.0b2 on Fedora 34.

https://crash-stats.mozilla.org/report/index/907f4b6e-9e32-4db6-bdd2-a7ad00210817

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Perhaps, I got it.

leftEntry is reference to an array item before inserting new item. So, if the array data is reallocated, the reference becomes invalid.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1726080 - Make `TextServicesDocument` and `OffsetEntryArray` use raw pointer of `OffsetEntry` instead of reference to `UniquePtr` r=m_kato!

If array size grows up, the array data may be reallocated. Therefore, after
getting a reference of an array item, we shouldn't modify array.

This makes them use raw pointer if they need to modify the array. Otherwise,
keep using the reference, but adds a stack class to detect the bug only in debug
build.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/daf499187cb6
Make `TextServicesDocument` and `OffsetEntryArray` use raw pointer of `OffsetEntry` instead of reference to `UniquePtr` r=m_kato

Comment 7

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

Set release status flags based on info from the regressing bug 1718924

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9236752 [details]
Bug 1726080 - Make TextServicesDocument and OffsetEntryArray use raw pointer of OffsetEntry instead of reference to UniquePtr r=m_kato!

Beta/Release Uplift Approval Request

• User impact if declined: May meet random crash at typing something in editor.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: Unfortunately, there is no STR. And according to my investigation, this happens randomly unless we find specific text content 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): The cause is, storing reference to an array which is UniquePtr, then, inserting new item into the array, then, referring the stored UniquePtr reference, but if the array buffer is reallocated, the raw ptr in the UniquePtr is accessible, but the reference points invalid address. So, this patch changes to store raw ptr if following code adds new item to the array.

The fix is only 2 lines, the others are debug only code to detect same regression in the other places.

• String changes made/needed:

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Unfortunately, there is no STR

I mean, in Firefox.

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/daf499187cb6

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #8)

Comment on attachment 9236752 [details]
Bug 1726080 - Make TextServicesDocument and OffsetEntryArray use raw pointer of OffsetEntry instead of reference to UniquePtr r=m_kato!

Beta/Release Uplift Approval Request

• User impact if declined: May meet random crash at typing something in editor.

In addition, it is the #2 crash for Thunderbird 92.

Further - is this likely to this fix our #1 crash? mozilla::TextServicesDocument::OffsetEntryArray::SplitElementAt

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I hope so. I have no more idea to crash in the method for now.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9236752 [details]
Bug 1726080 - Make TextServicesDocument and OffsetEntryArray use raw pointer of OffsetEntry instead of reference to UniquePtr r=m_kato!

Approved for 92.0b7.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4b3ee09808dc