Closed Bug 891904 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27
Tracking Flags:
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
firefox-esr24 26+ fixed

People

(Reporter: neil, Assigned: ayg)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Possible patch, not tested
832 bytes, patch
ehsan.akhgari
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta-
akeybl
: approval-mozilla-esr24+
Details | Diff | Splinter Review 
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.
(In reply to neil@parkwaycc.co.uk 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.
(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.
Blocks: 684638
No longer blocks: 856270
(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?
Typo, I meant bug 684638.
Boris, do you have cycles to take a look?
Flags: needinfo?(bzbarsky)
Probably not for at least a few weeks...
Flags: needinfo?(bzbarsky)
Aryeh, this bug is affecting Thunderbird 24 which was released today.  Any chance you could take a look at this, please?  Thanks!
Flags: needinfo?(ayg)
I can't really do anything for over a week, sorry.  If it's still open then I could look at it.
Flags: needinfo?(ayg)
OK, thanks!  This needs an owner, but I really don't have enough time to take this on myself.  Sorry.
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.
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.
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
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.
Attachment #812020 - Flags: review?(ehsan)
Whiteboard: [autoland-try:-b do -p all -u all[x64] -t none]
Whiteboard: [autoland-try:-b do -p all -u all[x64] -t none] → [autoland-try:-b do -p all -u all -t none]
Is autoland dead?

https://tbpl.mozilla.org/?tree=Try&rev=45b0f72548c0
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Whiteboard: [autoland-try:-b do -p all -u all -t none]
(In reply to :Aryeh Gregor from comment #14)
> Is autoland dead?

I think so. :(
Attachment #812020 - Flags: review?(ehsan) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/ee68e01a13cb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Once we verify the fix in nightly, is there any objections to this being back-ported to branches?
(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?
(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.
(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.
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).
(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.
Blocks: 917027
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.
Attachment #812020 - Flags: approval-mozilla-esr24?
Attachment #812020 - Flags: approval-mozilla-beta?
Attachment #812020 - Flags: approval-mozilla-aurora?
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
(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.
(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).
(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"?
Flags: needinfo?(mbanner)
(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.
Flags: needinfo?(mbanner)
I'll leave this decision up to the release drivers, I have no objections to Standard8's proposal.
(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 on attachment 812020 [details] [diff] [review]
Possible patch, not tested

Ok, we'll take this on Aurora 26 now, and ESR 24 next cycle
Attachment #812020 - Flags: approval-mozilla-beta?
Attachment #812020 - Flags: approval-mozilla-beta-
Attachment #812020 - Flags: approval-mozilla-aurora?
Attachment #812020 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Is there a way to reliably reproduce the issue and then verify it on Firefox Desktop?
Flags: needinfo?(ayg)
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.
Flags: needinfo?(ayg)
Removing the keyword per comment 35.
Keywords: verifyme
Attachment #812020 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Keywords: checkin-needed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.