528630 - components.list causes leaks

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Some interaction with bug 519357 is causing leaks on Linux:

leaked 4984 bytes during test execution
leaked 5 instances of nsBinaryOutputStream with size 16 bytes each (80 bytes
total)
leaked 1 instance of nsBufferedInputStream with size 48 bytes
leaked 1 instance of nsBufferedOutputStream with size 56 bytes
leaked 2 instances of nsBufferedStream with size 40 bytes each (80 bytes total)
leaked 10 instances of nsFastLoadFileUpdater with size 240 bytes each (2400
bytes total)
leaked 10 instances of nsFastLoadFileWriter with size 228 bytes each (2280
bytes total)
leaked 1 instance of nsStringBuffer with size 8 bytes
leaked 1 instance of nsSystemPrincipal with size 32 bytes

It could be a component load order issue or something else strange, as this only shows up on 1.9.2, and only on Linux.

I have disabled components.list on linux on 1.9.2 so that we can get the necessary testing on windows.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Spoke too soon -- these leaks look to be everywhere.  I've disabled this code with an #if 0 now.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: potential fix

Here's a potential fix.  This changes nothing about our comp load order, but instead just checks each component against a list.  However, I have no idea if this fixes the problem -- I can't reproduce the leaks locally, and the try server behaves identically both with this patch and the current code (without the if 0, of course).. but no leaks are reported in either pass.  So only thing I can think of is to send this to 1.9.2 and see what happens there :/

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 412444 [details] [diff] [review]
potential fix

Mmm, that looks like a lot of looping over the list -- should we be keeping the list sorted (asserting in DEBUG) and doing a more efficient search?  How long is this list, typically?

r=shaver, but since we still (!) don't have unit tests, please at least detail in here how you've tested it (adding, removing, existing components, installer?)

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Well, the tinderbox is doing a pretty darn good job of testing the basic functionality itself, what with the tp-cmdline.js stuff and whatnot.  :p  I'm writing tests early next week, need to get sdwilsh's help on how best to do this and the dll blocklisting stuff, since I failed earlier on the latter.

I've tested complist stuff with removing the file, removing entries from the file, and adding bogus components to the directory.  There are some problems with compreg as detailed in the other bug, but they don't seem to affect new installs or upgrades because we reregister then.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This looks like it /finally/ stuck.  We should try to figure out which components are being loaded in a different order from the list though, because I think we want to go back to the previous (on-trunk) approach to this.  Or we just leave it like this on the branch, as the effect is the same...  Closing this as fixed, but the original bug still needs tests.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Vlad, can you please always add the changeset id? As I have said in the implementation bug it will help us a lot when we have to do regression tests.

This has been landed as:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/57171f47b2bd

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

There are no leaks reported on any platform by the leak tests on 1.9.2. Marking this bug as verified fixed.