Firefox leaks SelectionDetails after running for a while

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
General
P2
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Tomcat, Assigned: Uri Bernstein (Google))

Tracking

({fixed1.9.1, memory-leak})

Trunk
mozilla1.9.1b2
x86
Mac OS X
fixed1.9.1, memory-leak
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

10 years ago
Created attachment 343736 [details] [diff] [review]
patch

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".
Assignee: nobody → uriber
Status: NEW → ASSIGNED
Attachment #343736 - Flags: review?(roc)
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

10 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

10 years ago
Created attachment 343942 [details] [diff] [review]
mochitest

This leaks one SelectionDetails object without the fix.
(Assignee)

Comment 6

10 years ago
http://hg.mozilla.org/mozilla-central/rev/6c2f8bd79cbc
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
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 → ---
Gah, helps if you disable the test properly
 http://hg.mozilla.org/mozilla-central/rev/6e9a29a235ce
(Assignee)

Comment 11

10 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

10 years ago
Seems to be working now.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.