Spell checker should take part in cycle collection

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Spelling checker
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({mlk})

Trunk
mozilla1.9.3a5
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+, status1.9.2 .5-fixed, status1.9.1 .11-fixed)

Details

Attachments

(1 attachment)

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.
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.
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+
http://hg.mozilla.org/mozilla-central/rev/25442798da4a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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+
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.  :/
status1.9.1: --- → .11-fixed
status1.9.2: --- → .5-fixed

Updated

7 years ago
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.