Last Comment Bug 708071 - Spelling suggestions creates a zombie compartment
: Spelling suggestions creates a zombie compartment
Status: RESOLVED FIXED
[MemShrink:P1]
:
Product: Firefox
Classification: Client Software
Component: Menus (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 11
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on:
Blocks: 319315 ZombieCompartments 708260
  Show dependency treegraph
 
Reported: 2011-12-06 13:14 PST by Fanolian
Modified: 2011-12-07 12:26 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simple textarea as a test case (57 bytes, text/html)
2011-12-06 13:14 PST, Fanolian
no flags Details
patch (6.35 KB, patch)
2011-12-06 14:54 PST, Dão Gottwald [:dao]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch (v1) (3.99 KB, patch)
2011-12-06 14:57 PST, :Ehsan Akhgari
no flags Details | Diff | Splinter Review

Description Fanolian 2011-12-06 13:14:58 PST
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.
Comment 1 Fanolian 2011-12-06 13:20:27 PST
Comment on attachment 579429 [details]
Simple textarea as a test case

><html>
><body>
><textarea>
></textarea>
></body>
></html>
Comment 2 Boris Zbarsky [:bz] 2011-12-06 13:27:57 PST
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 Boris Zbarsky [:bz] 2011-12-06 13:29:59 PST
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?
Comment 4 Dão Gottwald [:dao] 2011-12-06 13:57:07 PST
> 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
Comment 5 Dão Gottwald [:dao] 2011-12-06 14:54:46 PST
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.
Comment 6 :Ehsan Akhgari 2011-12-06 14:57:04 PST
Created attachment 579470 [details] [diff] [review]
Patch (v1)
Comment 7 Dão Gottwald [:dao] 2011-12-06 14:58:44 PST
Err, why would bug 319315 have caused this? This doesn't look like a regression to me.
Comment 8 Dão Gottwald [:dao] 2011-12-06 14:59:44 PST
Damn, stupid mid-airs.
Comment 9 :Ehsan Akhgari 2011-12-06 14:59:48 PST
Comment on attachment 579470 [details] [diff] [review]
Patch (v1)

Dao's patch is better than mine!
Comment 10 :Ehsan Akhgari 2011-12-06 15:01:15 PST
(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 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-06 15:10:13 PST
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 [].
Comment 12 Dão Gottwald [:dao] 2011-12-06 15:13:37 PST
(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.
Comment 13 Dão Gottwald [:dao] 2011-12-06 15:24:30 PST
(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 []...
Comment 15 Matt Brubeck (:mbrubeck) 2011-12-07 12:26:14 PST
https://hg.mozilla.org/mozilla-central/rev/06e75dde1c5a

Note You need to log in before you can comment on or make changes to this bug.