324145 - Memory leak at Dean Edwards' IE7 site

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Steve Chapel]

Reproducable: Always

Steps to Reproduce:
1. Start Firefox
2. Go to http://dean.edwards.name/IE7/
3. Click the Back button
4. Close Firefox

Expected Results: no memory leak
Actual Results: leak gauge reports leak of 3 DOM Windows and 3 documents

I've reproduced this with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060120 Firefox/1.6a1 with a new profile and SeaMonkey trunk build 2006011909 with an old profile. Going to the page multiple times by selecting it from the URL bar dropdown and clicking the Back button makes even more objects leak, but going back to the page clicking the Forward and Back buttons does not make more objects leak.

Comment 1

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060120 Firefox/1.6a1 ID:2006012005
I see this leak too.

Leaked inner window 16bdb38 (outer 2018e88) at address 16bdb38.
 ... with URI "http://dean.edwards.name/IE7/".
Leaked outer window 2018e88 at address 2018e88.
Leaked document at address 2873678.
 ... with URI "http://dean.edwards.name/my/behaviors/moz-behaviors.xml".
Leaked document at address 2871b28.
 ... with URI "http://dean.edwards.name/my/behaviors/bindings.xml".
Leaked document at address 266bf38.
 ... with URI "http://dean.edwards.name/IE7/".

Comment 2

[Boris Zbarsky [:bzbarsky]]

So.. I hate to say this, but would someone be willing to minimize this XBL/JS/etc?  As things stand, this thing is so huge I have no idea where to even start.

I _think_ the thing that's making us leak is:

b3dc9320 object 0x86c77b8 HTMLDocument via XPCWrappedNativeProto::mJSProtoObject(XPC_WN_ModsAllowed_Proto_JSClass @ 0xb3dc96d0).XPCWrappedNativeScope::mPrototypeJSObject(Object @ 0xb3dc9688).constructor(Function @ 0xb3dc9680).__proto__(Function @ 0xb3dc96b8).caller(Function @ 0x0895c8c8).__parent__(With @ 0x0895c3f0).__proto__(HTMLPreElement @ 0xb3ce37e8).__parent__(HTMLDocument @ 0xb3dc9320).

but I'm really not sure whether that's true and what the rest of the cycle is if so....

I also see us leaking an nsJSContext via the ref taken in NS_NewJSEventListener.  Not sure whether that's relevant here....

Comment 3

[Martijn Wargers (zombie)]

Here, I tried to minimise it:
http://wargers.org/mozilla/bug324145/
http://wargers.org/mozilla/bug324145/_IE7_.htm
Somehow, it only leaks 2 windows locally, not on the website, so this already might be a wrong minimalisation.
When I remove this code, the leak disappears:
            // load the behavior
            _httpRequest.open("GET", $url, false);
            _httpRequest.send(null);
            return _httpRequest.responseText;

Maybe this is (partly) related to bug 323454, because in this case, the string that I used for the xmlhttprequest, is also obtained by referencing an property from a dom node ( getComputedStyle(this, null).getPropertyValue("-moz-binding") ).

Comment 4

[Boris Zbarsky [:bzbarsky]]

That test case doesn't leak over here.  :(

Comment 5

[Martijn Wargers (zombie)]

Attachment: zipped testcase, not minimal

Well, at least I managed it reducing it to this. It's still terribly complicated, though. This leaks for me 2 dom windows and 3 documents.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: More reduced testcase, still nowhere close to minimal

A few more reduction steps.  The salient points now are:

1)  The node involved has an on* event attr.  Doesn't matter so much what the value is; "void(1)" leaks.

2)  The XBL loads a script via XMLHttpRequest and turns it into actual JS using new Function().

3)  The function body is completely wrapped in "with (this) { }".

4)  Whatever is going on, the call to parse() is needed to leak.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: More reduction and some code cleanup

So there are clearly two things here -- the rigmarole that actually executes this script, and the script itself.  I'm starting to wonder whether the XBL binding per se is even needed.... I'm also starting to wonder whether we're close to being able to nix the XMLHttpRequest silliness and just stick all this stuff into a string, even if the XBL binding is needed.

Martijn, do you think you can take a crack at removing whatever else you can in the "behavior" file?

Comment 8

[Martijn Wargers (zombie)]

Attachment: Minimised testcase

Thanks Boris! I admit, I wasn't too keen on minimising, because it's really tedious work (more tedious than html cases).

This leaks 2 docs and 2 windows for me. It doesn't involve xbl nor xmlhttprequest.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, I've done some more tracking, and I'm really not sure how we're managing to leak this stuff.  Here's what I know so far:

1)  The HTMLAnchorElement holds a strong ref to the outer window (via the
    nsJSEventListener object, which holds a strong ref to the nsJSContext, which
    locks the JSObject of the window, which keeps the window alive).
2)  The Window's scope seems to be cleared so it's not entraining anything that
    I can see.
3)  The XPCWrappedNativeProto that's entraining the HTMLAnchorElement and the
    HTMLDocument is the proto for HTMLDocument.

I'm really not sure how the window keeps the XPCWrappeNativeProto alive (which is what has to be happening).  Especially since we do NOT leak an XPCWrappedNativeScopes.

Comment 10

[Olli Pettay [:smaug]]

Using the minimized testcase I don't see window or document leaks anymore.

Comment 11

[Martijn Wargers (zombie)]

Yeah, doesn't seem to leak anymore. I tried the minimized testcase, the " zipped testcase, not minimal" and the site. All of them don't seem to leak anymore.
Marking worksforme?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I bet the cycle collector fixed this.  Most excellent!

Comment 13

[tombstonekane422]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Affects Firefox for Android]:
[Suggested wording]:
[Links (documentation, blog post, etc)]: