Closed Bug 468761 Opened 11 years ago Closed 11 years ago

test_placesTxn.js leaks due to observer

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: fixed1.9.1, memory-leak)

Attachments

(1 file)

i can't still say why that happens, however in test_placesTxn we do
  var fldrId2 = bmsvc.getChildFolder(root, "Folder2");
and this leaks
                                              Per-Inst   Leaked    Total
m      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          25      336    28525
7 (  323,39 +/-   425,61)    95239        6 (  300,03 +/-   422,76)
   2 BackstagePass                                  24       24        1
1 (    1,00 +/-     0,00)     2364        2 (  111,00 +/-    38,89)
  22 XPCNativeScriptableShared                     108      108      576
1 (   11,76 +/-     1,86)        0        0 (    0,00 +/-     0,00)
  25 XPCWrappedNative                               56       56     1744
1 (  841,78 +/-   486,49)     7581        1 (  815,37 +/-   489,99)
  26 XPCWrappedNativeProto                          32       32      359
1 (  177,38 +/-   101,41)        0        0 (    0,00 +/-     0,00)
 167 nsStringBuffer                                  8        8     5488
1 (  666,82 +/-   498,43)     9367        1 (  631,28 +/-   480,33)
 177 nsSystemPrincipal                              36       36        1
1 (    1,00 +/-     0,00)       17        1 (    3,67 +/-     1,47)
 179 nsThread                                       72       72        4
1 (    2,29 +/-     1,11)     3750        1 (  837,48 +/-   498,60)

the leak is activated by doing bindStringParameter on aSubFolder
that is, unless I'm totally wrong about the string api
i know, indeed i can't see why that happens, but i clearly see the leak, and if i replace the bindStringParam(aSubFolder) with bindStringParam(NS_LITERAL_STRING("Folder2")) it does not leak anymore.
since there's a nsThread leak could be that the thread is destoyed after the leak log finishes, so it does not see it. If that's the case this will be an INVALID.
I'm cc'ing mrbkap who might know what's going on (or be able to help at least).

Marco - can you link to the JavaScript in question so he can take a look please?
(In reply to comment #4)
> since there's a nsThread leak could be that the thread is destoyed after the
> leak log finishes, so it does not see it. If that's the case this will be an
> INVALID.

I think that's what's happening here. The (single) leaked nsStringBuffer is probably a result of leaking the system principal (also leaked here). Do you not join all of your threads before shutting down?
(In reply to comment #7)
> I think that's what's happening here. The (single) leaked nsStringBuffer is
> probably a result of leaking the system principal (also leaked here). Do you
> not join all of your threads before shutting down?
Threads created with the thread manager shouldn't need that though:
http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThreadManager.cpp#108

We only use threads created by it.
ok issue solved, it is due to the not removed observer

Sorry for noise!
Summary: getChildFolder leaks nsStringBuffer → test_placesTxn.js leaks due to observer
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9.1
Attached patch patchSplinter Review
Attachment #352510 - Flags: review?(dietrich)
(In reply to comment #10)

Please, take the other part of the patch too, from bug 4449240.
Blocks: 449240
Flags: wanted1.9.1?
Keywords: mlk
(In reply to comment #11)
> (In reply to comment #10)
> 
> Please, take the other part of the patch too, from bug 4449240.

do you mean for autocomplete and url formatter?
(In reply to comment #12)
> do you mean for autocomplete and url formatter?

No, I meant;
{
-bmsvc.addObserver(observer, false);
 
 function run_test() {
+  bmsvc.addObserver(observer, false);
}
i'm not against that, but all our tests use that style of adding the observer after defining it, and leave in run_test only the minimum needed to allow for better test comprehension...
So i leave the choice to module owner for a style, and we'll respect that choice from now on, updating the patch is matter of seconds.

Dietrich, what do you think?
Attachment #352510 - Flags: review?(dietrich) → review+
http://hg.mozilla.org/mozilla-central/rev/7878ba715f64
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081214 Minefield/3.2a1pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/3d8e98242012)

V.Fixed
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
removing wanted1.9.1? flag, i'll push this to branch with next bundle
Flags: wanted1.9.1?
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c135a2a89778
Keywords: fixed1.9.1
Target Milestone: mozilla1.9.1 → mozilla1.9.1b3
You need to log in before you can comment on or make changes to this bug.