Spelling suggestions creates a zombie compartment

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Menus
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Fanolian, Assigned: dao)

Tracking

Trunk
Firefox 11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 579429 [details]
Simple textarea as a test case

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111206 Firefox/11.0a1
Build ID: 20111206031117

Steps to reproduce:

1. In a text area, type a misspelled word.
2. Right-click on the misspelled word so that spelling suggestions appear in the context menu.
3. Close the tab. Open about:memory?verbose and minimize memory usage a dozen times.

A zombie compartment is created and it stays until I repeat it in another text area from another domain. But then the new compartment becomes a zombie. It is created whether I select a suggestion or not.
If I do not right-click on a misspelled word, or if I right-click on a correct word, no zombie compartment is created.
(Reporter)

Updated

6 years ago
Blocks: 668871
Whiteboard: [MemShrink]
(Reporter)

Comment 1

6 years ago
Comment on attachment 579429 [details]
Simple textarea as a test case

><html>
><body>
><textarea>
></textarea>
></body>
></html>
Attachment #579429 - Attachment mime type: text/plain → text/html

Comment 2

6 years ago
This is actually a spellcheck UI bug, I believe.

setTarget in nsContextMenu.js calls InlineSpellCheckerUI.initFromEvent which sets this.mWordNode to the mis-spelled word, but only if there is a mis-spelled word (see the !range check).

I don't see anything that clears this.mWordNode after that.

On the other hand, I also don't see anything that clears this.target in the context menu itself... what does that?

Comment 3

6 years ago
Oh, I see.  The context menu itself goes away: onpopuphiding we set gContextMenu to null.  But InlineSpellCheckerUI is a global variable, not something created anew each time.  So it ends up creating references to things from the chrome window.

Either we should make InlineSpellCheckerUI have the same lifetime as the context menu or we should explicitly clear out its state when taking down the context menu, right?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 4

6 years ago
> Either we should make InlineSpellCheckerUI have the same lifetime as the
> context menu or we should explicitly clear out its state when taking down
> the context menu, right?

yep
Component: Spelling checker → Menus
Product: Core → Firefox
QA Contact: spelling-checker → menus
Assignee: nobody → ehsan
Whiteboard: [MemShrink] → [MemShrink:P1]
Blocks: 356590
Keywords: regression
Blocks: 319315
No longer blocks: 356590
(Assignee)

Comment 5

6 years ago
Created attachment 579467 [details] [diff] [review]
patch

Note that this reverts the patch from bug 319315. As far as I know, onpopuphiding not getting called isn't a problem we currently have. Maybe bug 279703 fixed it.
Assignee: ehsan → dao
Status: NEW → ASSIGNED
Attachment #579467 - Flags: review?(gavin.sharp)
Created attachment 579470 [details] [diff] [review]
Patch (v1)
Assignee: dao → ehsan
Attachment #579470 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

6 years ago
Err, why would bug 319315 have caused this? This doesn't look like a regression to me.
Assignee: ehsan → dao
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Comment 8

6 years ago
Damn, stupid mid-airs.
Comment on attachment 579470 [details] [diff] [review]
Patch (v1)

Dao's patch is better than mine!
Attachment #579470 - Attachment is obsolete: true
Attachment #579470 - Flags: review?(gavin.sharp)
(In reply to Dão Gottwald [:dao] from comment #7)
> Err, why would bug 319315 have caused this? This doesn't look like a
> regression to me.

It's a regression in the sense that bug 319315 caused the spellchecking UI to hold on the old document until being invoked again.  Maybe regression is too strong a word, but that comment just made me curious, so I looked back in history.  :-)
Comment on attachment 579467 [details] [diff] [review]
patch

>diff --git a/toolkit/content/InlineSpellChecker.jsm b/toolkit/content/InlineSpellChecker.jsm

>+    if (!this.mSuggestionItems)

>+    if (!this.mDictionaryItems)

Why are these checks needed? I don't see how these could be null, they seem to always be reset to [].
Attachment #579467 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 12

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #7)
> > Err, why would bug 319315 have caused this? This doesn't look like a
> > regression to me.
> 
> It's a regression in the sense that bug 319315 caused the spellchecking UI
> to hold on the old document until being invoked again.  Maybe regression is
> too strong a word, but that comment just made me curious, so I looked back
> in history.  :-)

It used to hold on to the document before that, as uninit was ineffective regardless of when it would be called. Bug 319315's patch certainly didn't improve the situation, but since this was already broken, it didn't effectively regress anything, as I understand it.
Keywords: regression
(Assignee)

Comment 13

6 years ago
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #11)
> Comment on attachment 579467 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> >diff --git a/toolkit/content/InlineSpellChecker.jsm b/toolkit/content/InlineSpellChecker.jsm
> 
> >+    if (!this.mSuggestionItems)
> 
> >+    if (!this.mDictionaryItems)
> 
> Why are these checks needed? I don't see how these could be null, they seem
> to always be reset to [].

We call clearSuggestionsFromMenu and clearDictionaryListFromMenu without ever really initializing InlineSpellCheckerUI. But you're right that the checks aren't needed because toolkit/obsolete/content/inlineSpellCheckUI.js fake-initializes InlineSpellCheckerUI, which does nothing but calling uninit, which sets mSuggestionItems and mDictionaryItems to []...
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/06e75dde1c5a
Target Milestone: --- → Firefox 11

Updated

6 years ago
Blocks: 708260
https://hg.mozilla.org/mozilla-central/rev/06e75dde1c5a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.