test_placesTxn.js leaks due to observer

VERIFIED FIXED in mozilla1.9.1b3

Status

()

Toolkit
Places
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 1 bug, {fixed1.9.1, mlk})

Trunk
mozilla1.9.1b3
fixed1.9.1, mlk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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
I have a hard time believing that this leaks:
http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageStatement.cpp#472
that is, unless I'm totally wrong about the string api
(Assignee)

Comment 3

9 years ago
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.
(Assignee)

Comment 4

9 years ago
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?
(Assignee)

Comment 6

9 years ago
sure, http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/unit/test_placesTxn.js#275
(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.
(Assignee)

Comment 9

9 years ago
ok issue solved, it is due to the not removed observer

Sorry for noise!
(Assignee)

Updated

9 years ago
Summary: getChildFolder leaks nsStringBuffer → test_placesTxn.js leaks due to observer
(Assignee)

Updated

9 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9.1
(Assignee)

Comment 10

9 years ago
Created attachment 352510 [details] [diff] [review]
patch
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
(Assignee)

Comment 12

9 years ago
(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);
}
(Assignee)

Comment 14

9 years ago
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+
(Assignee)

Comment 15

9 years ago
http://hg.mozilla.org/mozilla-central/rev/7878ba715f64
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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+
(Assignee)

Comment 17

9 years ago
removing wanted1.9.1? flag, i'll push this to branch with next bundle
Flags: wanted1.9.1?
(Assignee)

Comment 18

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