421150 - XUL template leak (was: Firefox leaks on first start with Adblock Plus installed)

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[Carsten Book [:Tomcat]]

Attachment: leak log

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008030518 Firefox/3.0b5pre

Steps to reproduce:
- Install Adblock Plus
- On the first restart with Adblock Plus you get the Windows with the different subscription services
- Quit Firefox
-- Leak

Only leaks after first start and not on later sessions

I had also some tabs open (amo and default pages)

   0 TOTAL                                          27   237083   610546     7348 ( 1884.35 +/-  1991.02)  1741069     3852 ( 1998.01 +/-  3258.71)

Comment 1

[Damon Sicore (:damons)]

This is a popular extension, and we're leaking windows.  We should look into it.  blocking1.9+, P2.

Comment 2

[Damon Sicore (:damons)]

Also, we need an owner for this.  Who's going to sign up?  :)

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ugh, ok. Better than signing up for a social networking site.

Comment 4

[Damon Sicore (:damons)]

Thanks, bent.  :)

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

So I dug into this a bit today and I'm gonna say that this is the extension's fault. The leak here involves adblock's component and the subscriptions window. Basically what happens is that the component creates a strong cycle with some services (observer service, pref observers, maybe others) that is never broken. That component gets created in the window's scope and makes the scope uncollectable. Luckily that window only really comes up on the first run so it shouldn't be too terrible.

Anyway, blocking? again to see if drivers want more work done here.

Comment 6

[Damon Sicore (:damons)]

Nope.  -'ing.  Thanks for looking into it.

If anyone disagrees, feel free to re-nom.  Always looking for feedback.

Comment 7

[Jesse Ruderman]

Wladimir, you might find the analysis in comment 5 useful.

Comment 8

[Wladimir Palant]

I see the leak as well, thanks for bringing that to my attention. However, this subscriptions window is very simple and doesn't even access the component unless you add a subscription - I don't see how it managed to create a cycle. Investigating.

Comment 9

[Wladimir Palant]

As I suspected, the leak isn't caused by JavaScript at all. Instead, the XUL template (RDF datasource) is to be blamed. I will try to minimize the problematic code.

Comment 10

[Wladimir Palant]

Attachment: RDF file for the testcase

Comment 11

[Wladimir Palant]

Attachment: Minimized testcase

For whatever reason, this doesn't seem to leak if the RDF file is a data: or http: URL. To reproduce, both the XUL and the RDF file have to be downloaded to disk (RDF file should be rdf_leak.rdf in the same directory) - opening the XUL file from either file: or chrome: will leak.

Comment 12

[Jesse Ruderman]

I search for 'xul template mlk,leak' and found bug 394514 and bug 307160.  Is this leak the same as one of those or is it a previously unknown bug?

Comment 13

[Neil Deakin]

I get a similar leak log on Mac as in comment 0 with that testcase even if I remove the vbox, and in fact, just by opening and closing Firefox. I don't have AdBlock installed. I do not get a leak for the testcase in a build from around Feb 4.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Renominating, since this is a leak regression in the last month....

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #9)
> the XUL template (RDF datasource) is to be blamed

Yikes, yeah, I think my initial analysis was dead wrong (a good confirmation was to disable all the JS in the window). Sorry about that Wladimir. I'm going to grab this again because I think this will block again.

Comment 16

[Damon Sicore (:damons)]

+'ing, again.  

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This is basically the second patch from bug 307160 updated to trunk which fixes this leak.

The basic story of that bug, afaict, is this:

1. Bug 285076 was fixed, which tickled this leak.
2. Bug 307160 was fixed.
3. Bug 310833 was discovered, and it was bad.
4. Bug 307160 and Bug 285076 were backed out to fix Bug 310833.

The leak, however, still exists. We just didn't have a good way to reproduce it.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 308729 [details] [diff] [review]
Patch, v1

Oh, and I added an assertion to make sure that we find out about this leak if we somehow find ourselves in this spot again.

Comment 19

[Neil Deakin]

Comment on attachment 308729 [details] [diff] [review]
Patch, v1

This patch seems ok, but I'm very wary of changing this so late, especially since the first version of this patch caused a regression. Can we not just fix this leak for 1.9  and then have this patch as 'wanted-next'?

Comment 20

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

Well, better now than in a dot-release.

Is this a source of DEBUG_CC warnings? I.e. is it blocking effectively using leak monitor? If so I really think we want this for FF3 (or a dot release at least)

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We can fix the leak without changing behavior, yes. And it does affect DEBUG_CC output.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Quick fix

This is the quick leak-only fix requested.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 310061 [details] [diff] [review]
Quick fix

This is OK _if_ we clearly document along all possible callpaths that this method (and any callers that therefore end up with the same requirement) requires an additional non-XPCOM constraint on the out param: the pointer must be initialized on initial entry.  That is, this is an inout param, not an out param.

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed with proper documentation in the class decl.

Comment 25

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre and testcases+own testing -> no leak

--> verified fixed