Crash in <T>::operator() | mozilla::MozPromise<T>::FunctionThenValue<T>::DoResolveOrRejectInternal when trying to init editorspellchecker

RESOLVED FIXED in Firefox 55

Status

()

--
critical
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: assafdd, Assigned: m_kato)

Tracking

({crash, regression})

55 Branch
mozilla55
crash, regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55+ fixed)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8868260 [details]
crash_report_details.txt

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.96 Safari/537.36

Steps to reproduce:

1. Open a new tab. 
2. Open developer web console.
3. paste the following script once line after another.

var spellCheckerInput = document.documentElement.appendChild(document.createElementNS("http://www.w3.org/1999/xhtml", "input"));
var editor = spellCheckerInput.QueryInterface(Ci.nsIDOMNSEditableElement).editor;
var spellChecker =  Components.classes["@mozilla.org/editor/editorspellchecker;1"].createInstance(Ci.nsIEditorSpellCheck);
spellChecker.InitSpellChecker(editor, false);

The code seem pretty close to the following test:
https://dxr.mozilla.org/mozilla-central/source/editor/composer/test/test_bug1219928.html



Actual results:

Firefox crashes 


Expected results:

Spellchecker should be initialized.
(Reporter)

Comment 1

a year ago
I have noticed that the IDL of this service has 3rd optional callback parameter. 
Once i have added it the issues was solved. 
So, I guess it is not optional anymore.
I think it's worth updating the the docs.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIEditorSpellCheck#InitSpellChecker()

Comment 2

a year ago
Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c885f6b0de36ab24fa52b4a96d949c04a0dd00d5&tochange=0a82045b156396cad9d76643b7346abb9af79c78

Makoto Kato — Bug 1330912 - Part 1. Add async version of SetCurrentDictionary using list. r=Ehsan

CR:
https://crash-stats.mozilla.com/report/index/4010a4b3-d250-4604-8041-4d9a00170517
Blocks: 1330912
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ <T>::operator() | mozilla::MozPromise<T>::FunctionThenValue<T>::DoResolveOrRejectInternal ]
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
tracking-firefox55: --- → ?
Component: Untriaged → Editor
Ever confirmed: true
Flags: needinfo?(m_kato)
Keywords: crash, regression
Product: Firefox → Core
Summary: Firefox 55 crashes when trying to init @mozilla.org/editor/editorspellchecker → Crash in <T>::operator() | mozilla::MozPromise<T>::FunctionThenValue<T>::DoResolveOrRejectInternal when trying to init editorspellchecker
(Assignee)

Updated

a year ago
Assignee: nobody → m_kato
Flags: needinfo?(m_kato)
Comment hidden (mozreview-request)
Comment on attachment 8868902 [details]
Bug 1365383 - Spellchecker should consider that callback isn't set.

https://reviewboard.mozilla.org/r/140530/#review143962

::: editor/composer/test/test_bug1365383.html:40
(Diff revision 1)
> +  .onSpellCheck(textarea, () => {
> +    // Callback parameter isn't set
> +    spellChecker.UpdateCurrentDictionary();
> +
> +    var canSpellCheck = spellChecker.canSpellCheck();
> +    is(canSpellCheck, true, 'spellCheck is enabled');

Why don't you use ok()?
Attachment #8868902 - Flags: review?(masayuki) → review+
tracking as new crash in 55.
tracking-firefox55: ? → +
Comment hidden (mozreview-request)

Comment 7

a year ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/f8b25d64b513
Spellchecker should consider that callback isn't set. r=masayuki

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f8b25d64b513
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox-esr52: --- → unaffected
assafdd:

I have a question. How did you find this bug? I mean, why did you try to create an instance of nsIEditorSpellCheck?

I'm working on improving editor's performance, and my patches hit unexpected behavior in an automated test added by this bug's fix. So, if it was just for creating a XUL addon, not allowing to create an instance of nsIEditorSpellCheck from JS, we can get faster editor with simple patch. So, I want to disable it if there is no problem.
Flags: needinfo?(assafdd)
Oh, I realized that a lot of addons creates instances f nsIEditorSpellCheck. So, perhaps, we need to keep supporting it.

But if you don't mind, let me know why did you found this bug. Thanks.
Flags: needinfo?(assafdd)
(Reporter)

Comment 11

8 months ago
If I recall correctly, I found it while testing my XUL addon on nightly. 
AFAIK webexentsions do not have spell check API (which is a little sad), so I do not think this issue is to interesting looking forward.
Thank you. Yes, XUL addons are banned by Firefox and this kind of bugs are not valid on Firefox anymore, but not so by Thunderbird. And I'd like to support Tb's XUL addon's unless it'd cause something too bad for Firefox users because editor and spellchecker are mostly touched by them.
You need to log in before you can comment on or make changes to this bug.