Closed Bug 1056322 Opened 5 years ago Closed 3 years ago

crash in nsEditorSpellCheck::DictionaryFetched(DictionaryFetcher*)

Categories

(Core :: Spelling checker, defect, critical)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: wsmwk, Assigned: m_kato)

References

Details

(Keywords: crash, regression, regressionwindow-wanted, Whiteboard: [regression:TB24.0][tbird crash])

Crash Data

Attachments

(1 file)

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
OS: Windows NT → All
Crash Signature: [@ nsEditorSpellCheck::DictionaryFetched(DictionaryFetcher*)] → [@ nsEditorSpellCheck::DictionaryFetched(DictionaryFetcher*)] [@ nsEditorSpellCheck::DictionaryFetched]
#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 <ehsan@mozilla.com>
   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);
Component: Message Compose Window → Spelling checker
Product: Thunderbird → Core
Maybe, nsEditorSpellCheck isn't initialized correctly.  So calling UpdateCurrentDictionary cause this crash.
Duplicate of this bug: 1051173
Whiteboard: [regression:TB24.0] → [regression:TB24.0][tbird crash]
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...
Attachment #8857423 - Flags: review?(masayuki) → review+
(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.
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/c1357df217a4
Don't run DictionaryFetcher when spllchecker isn't initialized. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/c1357df217a4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request Aurora and ESR52 approval on this when you get a chance.
Assignee: nobody → m_kato
Flags: needinfo?(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.
Flags: needinfo?(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.
Attachment #8857423 - Flags: approval-mozilla-esr52?
(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?
Flags: needinfo?(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.
Flags: needinfo?(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
Attachment #8857423 - Flags: approval-mozilla-beta?
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.
Attachment #8857423 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8857423 [details]
Bug 1056322 - Don't run DictionaryFetcher when spllchecker isn't initialized.

null check, needed for thunderbird, ESR52.2+
Attachment #8857423 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Blocks: 1444630
You need to log in before you can comment on or make changes to this bug.