891904 - Turning spellcheck on and off in an unusual way leaves it in an unusable state

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[[e-mail]]

Thunderbird and SeaMonkey use the spell check override mechanism to turn spellchecking on and off on their editor elements.

When they close the window they have a recycling mechanism so that the window is simply hidden rather than being destroyed. Of interest is the fact that the editor is made readonly during this process. The spell check override actually overrides the readonly flag so that the spell checker is actually re-enabled rather than disabled as you might expect; only after the editor is made readonly does the spell checker override get turned off, at which point the spell checker really does get disabled.

There is a strange oddity in the inline spellchecking code in that if you turn it on when it is already on then the spellchecker rechecks the whole range. I don't know why it does this; it seems to date back to when the inline spellchecker was first written in bug 302050.

The changes in bug 856270 try to optimise this full recheck by disabling further spellcheck attempts until the full recheck completes.

However when you turn spellcheck off the spellchecker discards all pending rechecks without re-enabling further spellcheck attempts, so it then becomes impossible to turn the spellcheck back on.

Thus by making the editor readonly before disabling inline spellchecking Thunderbird and SeaMonkey manage to trigger this inline spellchecking bug.

As a workaround it may be possible to avoid disabling the editor immediately before turning off the spellchecker but I will file a separate bug for that.

Comment 1

[Drew Willcoxon :adw]

(In reply to [e-mail] from comment #0)
> The changes in bug 856270 try to optimise this full recheck by disabling
> further spellcheck attempts until the full recheck completes.

I don't understand what that means, and it doesn't sound like something bug 856270 was trying to do.

Is the problem simply: before bug 856270, mozInlineSpellChecker::SetEnableRealTimeSpell(true) always scheduled a spell check, but after bug 856270, mozInlineSpellChecker::SetEnableRealTimeSpell(true) does not schedule a spell check when the mozInlineSpellChecker's nsEditorSpellCheck is being initialized (i.e., when mPendingSpellCheck is non-null)?  That change in behavior wasn't intentional, so we can call it a regression I guess.

Comment 2

[[e-mail]]

(In reply to Drew Willcoxon from comment #1)
> (In reply to comment #0)
> > The changes in bug 856270 try to optimise this full recheck by disabling
> > further spellcheck attempts until the full recheck completes.
> I don't understand what that means, and it doesn't sound like something bug
> 856270 was trying to do.
My bad, I had my bug numbers mixed up. It's actually bug 684648 part 5 that suppresses spellcheck events until the full recheck completes, but never unsuppresses them if spellcheck is turned off first.

Comment 3

[(no longer active)]

(In reply to comment #2)
> (In reply to Drew Willcoxon from comment #1)
> > (In reply to comment #0)
> > > The changes in bug 856270 try to optimise this full recheck by disabling
> > > further spellcheck attempts until the full recheck completes.
> > I don't understand what that means, and it doesn't sound like something bug
> > 856270 was trying to do.
> My bad, I had my bug numbers mixed up. It's actually bug 684648 part 5 that
> suppresses spellcheck events until the full recheck completes, but never
> unsuppresses them if spellcheck is turned off first.

Wrong bug number again?

Comment 4

[[e-mail]]

Typo, I meant bug 684638.

Comment 5

[(no longer active)]

Boris, do you have cycles to take a look?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Probably not for at least a few weeks...

Comment 7

[(no longer active)]

Aryeh, this bug is affecting Thunderbird 24 which was released today.  Any chance you could take a look at this, please?  Thanks!

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I can't really do anything for over a week, sorry.  If it's still open then I could look at it.

Comment 9

[(no longer active)]

OK, thanks!  This needs an owner, but I really don't have enough time to take this on myself.  Sorry.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Could you please provide steps to reproduce, preferably that work in Firefox and not just Thunderbird?  This looks easy enough to fix if your description is right, but I can't be sure my fix works if I can't test it.

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Possible patch, not tested

This patch should fix the issue described in such excellent detail in comment #0 and comment #4, but I have no way of telling whether it fixes the actual problem.  If so, it still needs a test to land.

Comment 12

[[e-mail]]

(In reply to comment #0)
> There is a strange oddity in the inline spellchecking code in that if you
> turn it on when it is already on then the spellchecker rechecks the whole
> range. I don't know why it does this; it seems to date back to when the
> inline spellchecker was first written in bug 302050.
This oddity applied to disabling the editor because of the spell check override. That sequence of operations was triggered by discarding a composed message. It turned out to be straightforward to rearrange the sequence of operations so that the spell check override was disabled first and this was done in bug 880595.

However it turns out that the recheck oddity also applies to enabling the editor, then disabling the spell check override. Bug 917027 was filed when it was discovered that this sequence of operations was triggered by sending a message (either immediately or later). Unfortunately because the enabling operation lives in code generic to several compose tasks it is not easy to detect when the window will be subsequently recycled.

With the attached patch I can confirm that not only does the bug 917027 case no longer exhibit but also the bug 880595 case does not exhibit even when attachment 773324 [details] [diff] [review] is backed out.

Note that since there were three contributing factors I will not claim that this means that the patch is the correct fix for the bug but rather merely agree that it is a possible fix.

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 812020 [details] [diff] [review]
Possible patch, not tested

This looks extremely logical and is tested to fix the problem (see comment #12), but I don't know of any way to include an automated test.  I don't have any proof that this even changes behavior in Firefox.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Is autoland dead?

https://tbpl.mozilla.org/?tree=Try&rev=45b0f72548c0

Comment 15

[(no longer active)]

(In reply to :Aryeh Gregor from comment #14)
> Is autoland dead?

I think so. :(

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee68e01a13cb

This doesn't have a test, but I don't expect anyone will ever write one, so I'm not sure what to set in-testsuite to.  Thus I'm leaving it blank.  If anyone actually looks at the value of that field, please set it to whatever you think is appropriate.

Comment 17

[Ed Morley [:emorley]]

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

Comment 18

[Mark Banner (:standard8)]

Once we verify the fix in nightly, is there any objections to this being back-ported to branches?

Comment 19

[(no longer active)]

(In reply to comment #18)
> Once we verify the fix in nightly, is there any objections to this being
> back-ported to branches?

Isn't the next Thunderbird release on ESR29 or something?

Comment 20

[Mark Banner (:standard8)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> (In reply to comment #18)
> > Once we verify the fix in nightly, is there any objections to this being
> > back-ported to branches?
> 
> Isn't the next Thunderbird release on ESR29 or something?

It would be ESR 31. So I'd be looking to port this onto aurora, beta & esr24.

Comment 21

[(no longer active)]

(In reply to comment #20)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> > (In reply to comment #18)
> > > Once we verify the fix in nightly, is there any objections to this being
> > > back-ported to branches?
> > 
> > Isn't the next Thunderbird release on ESR29 or something?
> 
> It would be ESR 31. So I'd be looking to port this onto aurora, beta & esr24.

Hmm, why Aurora and Beta?  Not that I'm necessarily objecting, but I want to understand why you need the patch on those branches.

Comment 22

[Mark Banner (:standard8)]

Well primarily SeaMonkey will want it (they still follow the rapid release cycles), but it'd be useful to get on aurora soon for some wider testing than just trunk anyway (beta is probably less useful, although I do plan to do a beta release from there soon).

Comment 23

[(no longer active)]

(In reply to comment #22)
> Well primarily SeaMonkey will want it (they still follow the rapid release
> cycles), but it'd be useful to get on aurora soon for some wider testing than
> just trunk anyway (beta is probably less useful, although I do plan to do a
> beta release from there soon).

OK, sounds good, let's uplift it after it bakes a while on Nightly.

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 812020 [details] [diff] [review]
Possible patch, not tested

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Thunderbird compose window is extremely crippled after one run.
User impact if declined: In-line spell check doesn't work in the compose window
Fix Landed on Version: Nightly, requesting approval to bring forward
Risk to taking this patch (and alternatives if risky): Low, risk to inline spell checking only
String or UUID changes made by this patch: None

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

This has been tested on TB trunk by various TB users.

Comment 25

[Mark Banner (:standard8)]

I pushed this to a Thunderbird specific relbranch for 24.0.1 (although obviously it'd be great if we could land this in core):

https://hg.mozilla.org/releases/mozilla-esr24/rev/1e1b6b2d3cf4

Comment 26

[(no longer active)]

(In reply to comment #25)
> I pushed this to a Thunderbird specific relbranch for 24.0.1 (although
> obviously it'd be great if we could land this in core):
> 
> https://hg.mozilla.org/releases/mozilla-esr24/rev/1e1b6b2d3cf4

If that's enough for your needs, I'd rather not take this patch for esr24 at this point.  Ideally we should just let it ride the trains if Thunderbird is unblocked by this.

Comment 27

[Mark Banner (:standard8)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #26)
> (In reply to comment #25)
> > I pushed this to a Thunderbird specific relbranch for 24.0.1 (although
> > obviously it'd be great if we could land this in core):
> > 
> > https://hg.mozilla.org/releases/mozilla-esr24/rev/1e1b6b2d3cf4
> 
> If that's enough for your needs, I'd rather not take this patch for esr24 at
> this point.  Ideally we should just let it ride the trains if Thunderbird is
> unblocked by this.

As long as it gets there by the next release cycle I don't mind. I can cope with one or two relbranch landings, but I don't want that all the way through esr (that's just asking for something to be forgotten).

Comment 28

[Alex Keybl [:akeybl]]

(In reply to Mark Banner (:standard8) from comment #27)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #26)
> > (In reply to comment #25)
> > > I pushed this to a Thunderbird specific relbranch for 24.0.1 (although
> > > obviously it'd be great if we could land this in core):
> > > 
> > > https://hg.mozilla.org/releases/mozilla-esr24/rev/1e1b6b2d3cf4
> > 
> > If that's enough for your needs, I'd rather not take this patch for esr24 at
> > this point.  Ideally we should just let it ride the trains if Thunderbird is
> > unblocked by this.
> 
> As long as it gets there by the next release cycle I don't mind. I can cope
> with one or two relbranch landings, but I don't want that all the way
> through esr (that's just asking for something to be forgotten).

Sorry, what version of Gecko are you identifying as the "next release cycle"?

Comment 29

[Mark Banner (:standard8)]

(In reply to Alex Keybl [:akeybl] from comment #28)
> > As long as it gets there by the next release cycle I don't mind. I can cope
> > with one or two relbranch landings, but I don't want that all the way
> > through esr (that's just asking for something to be forgotten).
> 
> Sorry, what version of Gecko are you identifying as the "next release cycle"?

I was thinking of Gecko 26 to have it on ESR by. I can cope with manually creating a relbranch for the 25 cycle, but I don't want to have to push it much beyond that.

Comment 30

[(no longer active)]

I'll leave this decision up to the release drivers, I have no objections to Standard8's proposal.

Comment 31

[Justin Wood (:Callek)]

(In reply to Mark Banner (:standard8) from comment #29)
> (In reply to Alex Keybl [:akeybl] from comment #28)
> > > As long as it gets there by the next release cycle I don't mind. I can cope
> > > with one or two relbranch landings, but I don't want that all the way
> > > through esr (that's just asking for something to be forgotten).
> > 
> > Sorry, what version of Gecko are you identifying as the "next release cycle"?
> 
> I was thinking of Gecko 26 to have it on ESR by. I can cope with manually
> creating a relbranch for the 25 cycle, but I don't want to have to push it
> much beyond that.

Though, for reference SeaMonkey is still releasing off the Gecko 25 cycle, and would need to dual land this for all our betas and our final release, and any chemspills off our final release, so if this can land on default will save us lots of manual trouble/pain.

Comment 32

[Alex Keybl [:akeybl]]

Comment on attachment 812020 [details] [diff] [review]
Possible patch, not tested

Ok, we'll take this on Aurora 26 now, and ESR 24 next cycle

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/375edf59197a

Comment 34

[Bogdan Maris, Desktop QA]

Is there a way to reliably reproduce the issue and then verify it on Firefox Desktop?

Comment 35

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I was never able to reproduce the issue at all in Firefox.  The only report I'm aware of is Thunderbird-only, reported by Neil.  I don't have any reason to believe the bug can be triggered in Firefox, although it's plausible that it could somehow.

Comment 36

[Ioana (away)]

Removing the keyword per comment 35.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/21d67fb5f148