Last Comment Bug 468761 - test_placesTxn.js leaks due to observer
: test_placesTxn.js leaks due to observer
Status: VERIFIED FIXED
: fixed1.9.1, mlk
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.1b3
Assigned To: Marco Bonardo [::mak]
:
:
Mentors:
Depends on:
Blocks: 449240
  Show dependency treegraph
 
Reported: 2008-12-09 19:11 PST by Marco Bonardo [::mak]
Modified: 2008-12-16 06:36 PST (History)
5 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (894 bytes, patch)
2008-12-11 02:05 PST, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2008-12-09 19:11:57 PST
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
Comment 1 Shawn Wilsher :sdwilsh 2008-12-09 19:22:26 PST
I have a hard time believing that this leaks:
http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageStatement.cpp#472
Comment 2 Shawn Wilsher :sdwilsh 2008-12-09 19:22:58 PST
that is, unless I'm totally wrong about the string api
Comment 3 Marco Bonardo [::mak] 2008-12-10 01:15:09 PST
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.
Comment 4 Marco Bonardo [::mak] 2008-12-10 01:46:42 PST
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.
Comment 5 Shawn Wilsher :sdwilsh 2008-12-10 08:03:24 PST
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?
Comment 7 Blake Kaplan (:mrbkap) 2008-12-10 15:56:31 PST
(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?
Comment 8 Shawn Wilsher :sdwilsh 2008-12-10 16:10:49 PST
(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.
Comment 9 Marco Bonardo [::mak] 2008-12-11 02:00:36 PST
ok issue solved, it is due to the not removed observer

Sorry for noise!
Comment 10 Marco Bonardo [::mak] 2008-12-11 02:05:43 PST
Created attachment 352510 [details] [diff] [review]
patch
Comment 11 Serge Gautherie (:sgautherie) 2008-12-11 05:35:41 PST
(In reply to comment #10)

Please, take the other part of the patch too, from bug 4449240.
Comment 12 Marco Bonardo [::mak] 2008-12-11 05:38:47 PST
(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?
Comment 13 Serge Gautherie (:sgautherie) 2008-12-11 05:54:26 PST
(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);
}
Comment 14 Marco Bonardo [::mak] 2008-12-11 06:08:40 PST
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?
Comment 15 Marco Bonardo [::mak] 2008-12-13 02:01:17 PST
http://hg.mozilla.org/mozilla-central/rev/7878ba715f64
Comment 16 Serge Gautherie (:sgautherie) 2008-12-14 15:07:58 PST
[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
Comment 17 Marco Bonardo [::mak] 2008-12-15 04:52:36 PST
removing wanted1.9.1? flag, i'll push this to branch with next bundle
Comment 18 Marco Bonardo [::mak] 2008-12-16 01:37:52 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c135a2a89778

Note You need to log in before you can comment on or make changes to this bug.