Closed Bug 708071 Opened 13 years ago Closed 13 years ago

Spelling suggestions creates a zombie compartment

Categories

(Firefox :: Menus, defect)

Product:
Firefox
Component:
Menus
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 11

People

(Reporter: Fanolian+BMO, Assigned: dao)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(2 files, 1 obsolete file)

Simple textarea as a test case
57 bytes, text/html
Details
patch
6.35 KB, patch
Gavin
: review+
Details | Diff | Splinter Review 
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.
Blocks: ZombieCompartments
Whiteboard: [MemShrink]
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
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?
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
> 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
Attached patch patch — — Splinter Review 
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)
Attached patch Patch (v1) (obsolete) — — Splinter Review 
Assignee: dao → ehsan

        Attachment #579470 -
        Flags: review?(gavin.sharp)

    
    

    
  
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

    
    

    
  
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+

    
    

    
  
(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

    
    

    
  
(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 []...

    
  
Blocks: 708260

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/06e75dde1c5a
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: