433406 - Once spell checker is disabled, it can't be enabled again

Status: VERIFIED FIXED

(Core :: Spelling checker, defect)

Description

[Sylvain Pasche]

I saw this issue while investigating something else (bug 432225).

1) Open an editor, like http://www.mozilla.org/editor/midasdemo/
2) Right click on the editing area, and disable "Check Spelling"
3) Right click again and enable "Check Spelling"

Actual result: check spelling is not enabled
On a debug build, you get an assertion:
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004003: file .../mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp, line 725
(nserror gives NS_ERROR_INVALID_POINTER)

Expected: no assertion, and your spellnig chekc is back

This is a regression from bug 421083, I'm not getting the assertion by reverting that patch.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is the spell checker enabled again after a restart or aren't you able to activate it again?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

What I see is that you have to reload the tab to re-activate the spell checker. It happens for each text field.

Line that fails:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp&rev=1.47&mark=725&#720

Is it worth asking for blocking1.9?

Comment 3

[Sylvain Pasche]

Right, you don't need to restart the browser.

Some facts to consider if this should block the release:
It only happens with Midas, with html <input type="text"> or <textarea>, there is no issue. Some editors like google docs override the right click, so you don't have a possibility to disable the spell check (meaning you're not affected in that case).

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

On OS X I can see this behavior for any text field (input, textarea) on Bugzilla and not only Midas.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase

Comment 6

[Martijn Wargers (dead)]

The testcase needs enhanced privileges.

Comment 7

[Martijn Wargers (dead)]

Attachment: mochitest

Comment 8

[Mike Schroepfer]

Given that this is midas only and this is a workaround I don't think it should block.  If can get a fix we'd happily take in any upcoming release.

Comment 9

[Martijn Wargers (dead)]

This is not for designMode only, it also happens for text inputs and textareas.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mike, perhaps I wasn't clear enough in my last comments but this is not Midas only.

Steps:
1. Open http://www.google.com
2. Activate spell checker (if not already done)
3. Deactivate spell checker
4. Try to activate the spell checker again

=> With step 4 you are not able to activate the spell checker anymore

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry Martijn, I removed the flag after a mid-air collision. Reasking.

Comment 12

[Mike Schroepfer]

(In reply to comment #10)
> Mike, perhaps I wasn't clear enough in my last comments but this is not Midas
> only.
> 
> Steps:
> 1. Open http://www.google.com
> 2. Activate spell checker (if not already done)
> 3. Deactivate spell checker
> 4. Try to activate the spell checker again
> 
> => With step 4 you are not able to activate the spell checker anymore
> 

Re-reading your comments the confusion was on my end.   Testing this out locally it looks like a page reload or a browser restart resolves the issue.   Given that, and the fact that I don't think people toggle this on and off repeatedly I still don't think this should cause an RC2...

Comment 13

[Martijn Wargers (dead)]

You're probably right that this should not cause an RC2, but it seems to me this should be fixed as soon as possible.
Maybe this can be fixed in an RC2 if is decided an RC2 comes out (and a reviewed patch is attached to this bug)?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I did a check with my debug build and got following information:

The assertion which was reported in comment 0 happens because mEditor is null.

(gdb) frame 1
#1  0x12a12656 in mozInlineSpellChecker::SetEnableRealTimeSpell (this=0x3e1cfde0, aEnabled=1) at /Users/Shared/Projects/mozilla/source/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp:724
724	      res = spellchecker->InitSpellChecker(editor, PR_FALSE);
(gdb) frame 0
#0  nsEditorSpellCheck::InitSpellChecker (this=0x35e06cc0, aEditor=0x0, aEnableSelectionChecking=0) at /Users/henrik/Projects/mozilla/source/mozilla/editor/composer/src/nsEditorSpellCheck.cpp:119
119	  NS_ENSURE_SUCCESS(rv, rv);

mEditor is set to 0x0 within Cleanup():
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp&rev=1.47&mark=587&#574

This change was done by the fix from Mat on bug 421083. I hope it's ok to assign you, Mat.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I missed a frame:

(gdb) frame 0
#0  nsTextServicesDocument::InitWithEditor (this=0x3e1224c0, aEditor=0x0) at /Users/henrik/Projects/mozilla/source/mozilla/editor/txtsvc/src/nsTextServicesDocument.cpp:221

221  if (!aEditor)
222    return NS_ERROR_NULL_POINTER;

After reloading the page mEditor is present again. So why it's getting deleted when deactivating the spell checker?

Comment 16

[Martijn Wargers (dead)]

Attachment: patch

Mats, do you think this is a good patch?
The problem is that mEditor is made null now in the cleanup function.
GetInlineSpellChecker didn't call init on the spellchecker if it already existed.

It passes the mochitest, attached to the bug (I haven't run all mochitests).

Comment 17

[Mike Schroepfer]

(In reply to comment #13)
> You're probably right that this should not cause an RC2, but it seems to me
> this should be fixed as soon as possible.
> Maybe this can be fixed in an RC2 if is decided an RC2 comes out (and a
> reviewed patch is attached to this bug)?
> 

Agreed.  So I'm going to take this off blocking list - flag it with [RC2?] on the whiteboard plus wanted 1.9.0.x.  This fix will go in whichever release is next.

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 320996 [details] [diff] [review]
patch

This looks good to me. r=mats

Comment 19

[Martijn Wargers (dead)]

Comment on attachment 320996 [details] [diff] [review]
patch

Ok, the patch passes all existing mochitests.

Comment 22

[Martijn Wargers (dead)]

Roc, could you sr? (in case an RC2 will be planned)

Comment 23

[Martijn Wargers (dead)]

Comment on attachment 320996 [details] [diff] [review]
patch

Not sure what to do with the patch now.
If there is a new tree, can somebody check it in the new tree?

Comment 24

[Mike Schroepfer]

Comment on attachment 320996 [details] [diff] [review]
patch

a+ schrep for 3.0.1 or RC2 please land on cvs trunk.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

mozilla/editor/libeditor/base/nsEditor.cpp 	1.510 

Comment 27

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052907 Minefield/3.0pre