Closed Bug 569504 Opened 10 years ago Closed 10 years ago

Spell checker should take part in cycle collection

Categories

(Core :: Spelling checker, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
blocking2.0 --- final+
status1.9.2 --- .5-fixed
status1.9.1 --- .11-fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: memory-leak)

Attachments

(1 file)

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.
Attached patch Patch (v1)Splinter Review
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: --- → ?
http://hg.mozilla.org/mozilla-central/rev/25442798da4a
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.
Attachment #448797 - Flags: approval1.9.2.5?
Attachment #448797 - Flags: approval1.9.1.11?
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.)
Attachment #448797 - Flags: approval1.9.2.5?
Attachment #448797 - Flags: approval1.9.2.5+
Attachment #448797 - Flags: approval1.9.1.11?
Attachment #448797 - Flags: approval1.9.1.11+
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
Depends on: 570350
Summary: The unit test for bug 433860 leaks 5 DOMWINDOWs → Spell checker should take part in cycle collection
blocking2.0: ? → final+
You need to log in before you can comment on or make changes to this bug.