Closed
Bug 460532
Opened 17 years ago
Closed 17 years ago
Firefox leaks SelectionDetails after running for a while
Categories
(Core :: General, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b2
People
(Reporter: cbook, Assigned: uriber)
References
Details
(Keywords: fixed1.9.1, memory-leak)
Attachments
(2 files)
2.78 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
2.65 KB,
patch
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081017 Firefox/3.1b2pre
Steps to reproduce:
-> 3 Tabs open with various sites + running firefox for a while and and was filing a bug :)
--> Leak
== BloatView: ALL (cumulative) LEAK STATISTICS
|<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
Per-Inst Leaked Total Rem Mean StdDev Total Rem Mean StdDev
0 TOTAL 23 32 18138505 2 ( 4630.88 +/- 3494.33) 32592776 0 ( 4443.49 +/- 6241.90)
74 SelectionDetails 16 32 4646 2 ( 2.13 +/- 0.98) 0 0 ( 0.00 +/- 0.00)
nsTraceRefcntImpl::DumpStatistics: 939 entries
nsStringStats
=> mAllocCount: 184172
=> mReallocCount: 22570
=> mFreeCount: 184172
=> mShareCount: 172248
=> mAdoptCount: 18508
=> mAdoptFreeCount: 18508
Assignee | ||
Comment 1•17 years ago
|
||
Funny coincidence - I just stumbled upon this code a couple of days ago and noticed it's leaky.
This patch should fix the leak, and also gets rid of an unnecessary "if".
Not sure if this is a regression. If it is it might be a blocker.
Flags: blocking1.9.1?
Comment on attachment 343736 [details] [diff] [review]
patch
awesome, thanks!
So to reproduce this, you just have to have a multiple selection and click in the first part of the selection, right? Can we write a mochitest for that that leaks without this fix?
I guess what we really want is for SelectionDetails to not be such an error-prone mess...
Attachment #343736 -
Flags: superreview+
Attachment #343736 -
Flags: review?(roc)
Attachment #343736 -
Flags: review+
Assignee | ||
Comment 4•17 years ago
|
||
(In reply to comment #3)
> So to reproduce this, you just have to have a multiple selection and click in
> the first part of the selection, right?
Not exactly - you need more than one selection range (either of the same type or of different types) to overlap the clicked point (in practice, this probably occurs when you click in a selection that is also marked as misspelled or highlighted by "Highlight all"). I'll try to make a mochitest with two overlapping normal selection ranges.
Assignee | ||
Comment 5•17 years ago
|
||
This leaks one SelectionDetails object without the fix.
Assignee | ||
Comment 6•17 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
That's great, thanks!
Comment 8•17 years ago
|
||
Uri left a message on the tinderbox waterfall at 16:32 that he'd pushed a fix for the test bustage (on linux, mochitest is orange with focus errors after starting test_word_movement.html; on win32, redness from hanging test_character_movement.html). That doesn't appear in the pushlog.
#developers suggested disabling the new test for this bug, to try to isolate if it leaves mochitest in a bad state for subsequent tests, or if there is a problem with the code change. That's http://hg.mozilla.org/mozilla-central/rev/fba837242af0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 9•17 years ago
|
||
Gah, helps if you disable the test properly
http://hg.mozilla.org/mozilla-central/rev/6e9a29a235ce
You could also disable the test like this:
http://hg.mozilla.org/mozilla-central/annotate/c4c46a5f9681/accessible/tests/mochitest/Makefile.in#l66
if you want.
Assignee | ||
Comment 11•17 years ago
|
||
(In reply to comment #8)
> Uri left a message on the tinderbox waterfall at 16:32 that he'd pushed a fix
> for the test bustage (on linux, mochitest is orange with focus errors after
> starting test_word_movement.html; on win32, redness from hanging
> test_character_movement.html). That doesn't appear in the pushlog.
Sorry. My push failed and I didn't notice.
I now pushed the fix to the test and re-enabled it. I'll follow the tree.
Assignee | ||
Comment 12•17 years ago
|
||
Seems to be working now.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•