Closed Bug 569504 Opened 10 years ago Closed 10 years ago
Spell checker should take part in cycle collection
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.
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.
Attachment #448797 - Flags: review?(roc)
This is a very serious memory leak, and I think that it should both block Firefox 4 and branch releases.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Comment on attachment 448797 [details] [diff] [review] Patch (v1) Nice!!!
Attachment #448797 - Flags: review?(roc) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Component: Editor → Spelling checker
QA Contact: editor → spelling-checker
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 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.)
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 18.104.22.168 while checking in. :/
Summary: The unit test for bug 433860 leaks 5 DOMWINDOWs → Spell checker should take part in cycle collection
You need to log in before you can comment on or make changes to this bug.