Last Comment Bug 569504 - Spell checker should take part in cycle collection
: Spell checker should take part in cycle collection
Status: RESOLVED FIXED
: mlk
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla1.9.3a5
Assigned To: :Ehsan Akhgari
:
: Jet Villegas (:jet)
Mentors:
Depends on: 570350
Blocks: 433860
  Show dependency treegraph
 
Reported: 2010-06-01 17:56 PDT by :Ehsan Akhgari
Modified: 2010-09-15 15:34 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.5-fixed
.11-fixed


Attachments
Patch (v1) (35.92 KB, patch)
2010-06-02 11:20 PDT, :Ehsan Akhgari
roc: review+
mbeltzner: approval1.9.2.5+
mbeltzner: approval1.9.1.11+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2010-06-01 17:56:13 PDT
When I submitted the unit test for bug 433860 to the try server, I noticed that it leaks 5 DOMWINDOWs:

<http://tests.themasta.com/tinderboxpushlog/leak-analysis/?id=1275429579.1275431155.18170.gz&tree=MozillaTry>

leaked 1 DOMWINDOW(s)
/tests/browser/base/content/test/test_contextmenu.html leaked 5 DOMWINDOW(s)
/tests/content/xbl/test/test_bug542406.xhtml leaked 1 DOMWINDOW(s)

(I don't yet know what's going on with test_bug542406.xhtml yet)

Based on my initial investigations, the leak could be caused by the fact that spell checker objects do not participate in cycle collection (but I could be wrong).

I'm currently building with cycle collection debug enabled to try to figure out what's going on there.
Comment 1 :Ehsan Akhgari 2010-06-02 11:20:50 PDT
Created attachment 448797 [details] [diff] [review]
Patch (v1)

mozInlineSpellChecker holds strong nsCOMPtrs, which result in cycles, which effectively mean that once spell checking finds a mistake on a web page, several object form a cycle and will never be freed.  This is pretty serious, and I'm so glad that I wrote the test for bug 433860 which caused me to find this problem.

This patch adds the required cycle collection cruft to all the classes which exist in the cycle.  With this patch, the test for bug 433860 does not leak.

I'm not sure who the best person to review this is, so I'm asking roc for now.
Comment 2 :Ehsan Akhgari 2010-06-02 11:21:35 PDT
This is a very serious memory leak, and I think that it should both block Firefox 4 and branch releases.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-03 04:09:03 PDT
Comment on attachment 448797 [details] [diff] [review]
Patch (v1)

Nice!!!
Comment 5 :Ehsan Akhgari 2010-06-03 10:19:25 PDT
Comment on attachment 448797 [details] [diff] [review]
Patch (v1)

This patch fixes leaks in pages where the user uses the spell checker on.  Without this patch, we keep a lot of the data structures for those pages in memory indefinitely.
Comment 6 Mike Beltzner [:beltzner, not reading bugmail] 2010-06-04 06:32:31 PDT
Comment on attachment 448797 [details] [diff] [review]
Patch (v1)

a=beltzner for mozilla-1.9.1 default and mozilla-1.9.2 default, this is a test needed for bug 433860 (which fixes a problem that affects Facebook due to a change in their code.)
Comment 7 :Ehsan Akhgari 2010-06-04 10:18:26 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4ce833a970d7
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3821fddf5025

For some reason I thought we're at 1.9.1.15 while checking in.  :/

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