1056322 - crash in nsEditorSpellCheck::DictionaryFetched(DictionaryFetcher*)

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Wayne Mery (:wsmwk)]

Earliest crash I find is 24.0. None for 17.0.x.  #89 crash for TB24.6.0.  crash-stats can't helps us determine if it started between 24 and 17.  Crash does not exist for Firefox

This bug was filed from the Socorro interface and is 
report bp-1fdaefc6-4d1d-4939-b834-005402140815.
=============================================================
 0 	xul.dll	nsEditorSpellCheck::DictionaryFetched(DictionaryFetcher*)	editor/composer/src/nsEditorSpellCheck.cpp
1 	xul.dll	DictionaryFetcher::HandleCompletion(unsigned short)	editor/composer/src/nsEditorSpellCheck.cpp
2 	xul.dll	NS_InvokeByIndex	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp

Comment 1

[Wayne Mery (:wsmwk)]

#101 crash for Thunderbird 38.6.0  https://crash-stats.mozilla.com/search/?signature=%3DnsEditorSpellCheck%3A%3ADictionaryFetched&date=%3E2016-02-01&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports   

But the stacks are whacky
bp-f6e0f50f-16b4-4d71-a2d4-9486d2160309
bp-46211c92-52e5-408c-80a9-c29ab2160217
bp-353a8eff-2603-4d61-9dcb-582ca2160320
bp-34220463-ee82-4329-a3e2-743792160201
bp-abfe14e0-9752-4644-8e98-aa3142160312


But only 1 Firefox crashes in past 2 months, vs 1,500 Thunderbird crashes ?

Comment 2

[Wayne Mery (:wsmwk)]

#119 is crash ranking for version 45 - not much different from 38x

bp-8b45b348-ffde-4fe5-b9de-1a9592161109 
 0 	XUL	nsEditorSpellCheck::DictionaryFetched(DictionaryFetcher*)	editor/composer/nsEditorSpellCheck.cpp:815
1 	XUL	DictionaryFetcher::HandleCompletion(unsigned short)	editor/composer/nsEditorSpellCheck.cpp:136
2 	XUL	NS_InvokeByIndex	xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:174
3 	XUL	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)	js/xpconnect/src/XPCWrappedNative.cpp:2097
4 	XUL	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1115
5 	XUL	js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct)	js/src/jscntxtinlines.h:240 

https://hg.mozilla.org/releases/mozilla-esr45/annotate/1496935d4503/editor/composer/nsEditorSpellCheck.cpp#l136
 263771 ac4464790ec4 Bug 1145631 - Part 1: Replace MOZ_OVERRIDE and MOZ_FINAL with override and final in the tree; r=froydnj
Ehsan Akhgari <[e-mail]>
   134  NS_IMETHOD HandleCompletion(uint16_t reason) override
   135  {
   136    mSpellCheck->DictionaryFetched(this);

https://hg.mozilla.org/releases/mozilla-esr45/annotate/1496935d4503/editor/composer/nsEditorSpellCheck.cpp#l815
   813  // We obtain a list of available dictionaries.
   814   nsTArray<nsString> dictList;
   815   rv2 = mSpellChecker->GetDictionaryList(&dictList);

Comment 3

[Makoto Kato [:m_kato]]

Maybe, nsEditorSpellCheck isn't initialized correctly.  So calling UpdateCurrentDictionary cause this crash.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized.

When CanSpellCheck is called yet, mSpellChecker won't be initialized.  So we should check it before running DictionaryFetcher.

I think that UpdateCurrentDictionary is called by OnFocus as long as nsEditorSpellCheck::InitSpellChecker isn't called.  But I cannot find test case to reproduce this...

Review commit: https://reviewboard.mozilla.org/r/129420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129420/

Comment 6

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

Comment on attachment 8857423 [details]
Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized.

https://reviewboard.mozilla.org/r/129420/#review131984

::: commit-message-f40e2:3
(Diff revision 1)
> +Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized. r?masayuki
> +
> +When CanSpellCheck is called yet, mSpellChecker won't be initialized.  So we should check it before running DictionaryFetcher.

isn't called yet?

::: commit-message-f40e2:5
(Diff revision 1)
> +Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized. r?masayuki
> +
> +When CanSpellCheck is called yet, mSpellChecker won't be initialized.  So we should check it before running DictionaryFetcher.
> +
> +I think that UpdateCurrentDictionary is called by OnFocus as long as nsEditorSpellCheck::InitSpellChecker isn't called.  But I cannot find test case to reproduce this...

Yeah, me too...

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] from comment #6)
> Comment on attachment 8857423 [details]
> Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized.
> 
> https://reviewboard.mozilla.org/r/129420/#review131984
> 
> ::: commit-message-f40e2:3
> (Diff revision 1)
> > +Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized. r?masayuki
> > +
> > +When CanSpellCheck is called yet, mSpellChecker won't be initialized.  So we should check it before running DictionaryFetcher.
> 
> isn't called yet?

Ah, yes. I will need enough sleep.

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8857423 [details]
Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129420/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1357df217a4
Don't run DictionaryFetcher when spllchecker isn't initialized. r=masayuki

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora and ESR52 approval on this when you get a chance.

Comment 12

[Makoto Kato [:m_kato]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> Please request Aurora and ESR52 approval on this when you get a chance.

There is no Firefox crash , but it is only Thunderbird.  So it is unnecessary for 54, but ESR52 might be necessary for Thunderbird.

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 8857423 [details]
Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Start up crash on Thunderbird

Fix Landed on Version:
55

Risk to taking this patch (and alternatives if risky): 
No, it is null check only

String or UUID changes made by this patch: 
No

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to Makoto Kato [:m_kato] from comment #12)
> There is no Firefox crash , but it is only Thunderbird.  So it is
> unnecessary for 54, but ESR52 might be necessary for Thunderbird.

TB ships betas that track mozilla-beta, don't they?

Comment 15

[Makoto Kato [:m_kato]]

(In reply to [Out of Office Until 24-April] Ryan VanderMeulen [:RyanVM] from comment #14)
> (In reply to Makoto Kato [:m_kato] from comment #12)
> > There is no Firefox crash , but it is only Thunderbird.  So it is
> > unnecessary for 54, but ESR52 might be necessary for Thunderbird.
> 
> TB ships betas that track mozilla-beta, don't they?

Ah Yes, I don't know how number people uses Thuderbird beta channel and updating cycle for it as auto updating.  OK, I will request uplift.

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8857423 [details]
Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized.

Approval Request Comment
[Feature/Bug causing the regression]:
No
[User impact if declined]:
Crash on Thunderbird.  I don't know the operation to crash this.

[Is this code covered by automated tests?]:
No.  This is crash issue and I don't know how to crash by this signature.

[Has the fix been verified in Nightly?]:
Yes.  no crash in Thunderbird 55.0.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
Nothing

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
null check only.

[String changes made/needed]:
No

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8857423 [details]
Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized.

Fix a crash. Beta54+. Should be in 54 beta 2.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/04e5c485c165

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8857423 [details]
Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized.

null check, needed for thunderbird, ESR52.2+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/f076a30f6c29