460532 - Firefox leaks SelectionDetails after running for a while

Status: RESOLVED FIXED

Description

[Carsten Book [:Tomcat]]

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

Comment 1

[Uri Bernstein (Google)]

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".

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure if this is a regression. If it is it might be a blocker.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 4

[Uri Bernstein (Google)]

(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.

Comment 5

[Uri Bernstein (Google)]

Created attachment 343942 [details] [diff] [review]
mochitest

This leaks one SelectionDetails object without the fix.

Comment 6

[Uri Bernstein (Google)]

http://hg.mozilla.org/mozilla-central/rev/6c2f8bd79cbc

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's great, thanks!

Comment 8

[Nick Thomas [:nthomas] (UTC+13)]

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

Comment 9

[Nick Thomas [:nthomas] (UTC+13)]

Gah, helps if you disable the test properly
 http://hg.mozilla.org/mozilla-central/rev/6e9a29a235ce

Comment 10

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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.

Comment 11

[Uri Bernstein (Google)]

(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.

Comment 12

[Uri Bernstein (Google)]

Seems to be working now.