385082 - Memoryleak on wordpress.com

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Steve England [:stevee]]

1. New profile, start firefox with leak logging
2. Navigate to http://wordpress.com/
3. Wait for page to load, close firefox
4. Analyze log

Leaked inner window 1c94850 (outer 1ca0be0) at address 1c94850.
 ... with URI "about:blank".
Leaked inner window 2a80c68 (outer 2994f48) at address 2a80c68.
 ... with URI "http://wordpress.com/".
Leaked outer window 1ca0be0 at address 1ca0be0.
Leaked outer window 2994f48 at address 2994f48.
Leaked document at address 2a14c90.
 ... with URI "http://wordpress.com/".
Summary:
Leaked 4 out of 14 DOM Windows
Leaked 1 out of 43 documents
Leaked 0 out of 6 docshells

I see this in:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a6pre) Gecko/20070619 Minefield/3.0a6pre ID:2007061904

But not in:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.5pre) Gecko/2007053104 BonEcho/2.0.0.5pre

FWIW I went through the Alexa top 100 English sites (just each site's homepage, no navigation within each page) and wordpress was the only one I saw leaking on trunk. Tomorrow I will go clicking within each site and see if I can find any other leakage.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: cycle collector shutdown dump

This is the output of ExplainLiveExpectedGarbage in the cycle collector.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The reason we're holding on to the BackstagePass for http://mxr.mozilla.org/seamonkey/source/toolkit/components/passwordmgr/src/nsLoginManager.js is:

0x1410e00 BackstagePass 2aaab8103080 via interned_atom[5937](0x2aaab836b100 Function).__parent__(0x2aaab835b3c0 Call)._c(0x2aaab81eea00 Object).div(0x2aaab823eb00 Object).main(0x2aaab830cec0 HTMLDivElement).__parent__(0x14d2200 HTMLDocument).XPCWrappedNative::mNativeWrapper(0x14d24c0 XPCNativeWrapper).addEventListener(0x14d3900 Function).__proto__(0x1410e40 Function).__parent__

The reason we're holding on the BackstagePass for http://mxr.mozilla.org/seamonkey/source/toolkit/components/passwordmgr/src/storage-Legacy.js is:

0x2aaab8094100 BackstagePass 2aaab80af1e0 via interned_atom[5937](0x13940c0 Object).__parent__

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The HTML iframe element and the 3 HTML image elements are all leaked from nsDOMEvent::DuplicatePrivateData (assuming I'm matching line numbers correctly, they're target and originalTarget).  I think we need to make nsDOMEvent participate in cycle collection.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For what it's worth, the reason we leak the DOM events is:

0x16a4a00 Event 2aaab804b2d0     via interned_atom[5964](0x1d6d780 Function).__parent__(0x1d6d680 Call)._d(0x1d6d740 Array).0
0x160b140 Event 2aaab8033f40     via interned_atom[5964](0x1d6d600 Function).__parent__(0x1d6d500 Call)._d(0x1d6d5c0 Array).0
0x160b080 Event 2aaab8060600     via interned_atom[5964](0x1d6d480 Function).__parent__(0x1d6d340 Call)._d(0x1d6d440 Array).0
0x1f5d4c0 Event 1f659b0          via interned_atom[5964](0x1d6d2c0 Function).__parent__(0x1d6d1c0 Call)._d(0x1d6d280 Array).0

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Smaug, any chance I could talk you into hooking up nsDOMEvent and subclasses into cycle collection (See comment 3, and also the extra members for mutation events, and perhaps a few others)?  I'm not sure from the above that it'll fix this bug, but I suspect so, and I think it's probably necessary anyway (since somebody could entrain an event in an event listener closure).

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Make events ccollectable

Fixes the leak for me.

nsEvent specific logic is kept in nsDOMEvent and only nsDOMEvent and 
nsDOMUIEvent have nsCOMPtr members.
nsDOMTextEvent::mTextRange doesn't need to be traversed.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 269814 [details] [diff] [review]
Make events ccollectable

>Index: content/events/src/nsDOMEvent.cpp
>===================================================================

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsDOMEvent)
>+  if (tmp->mEventIsInternal) {
>+    tmp->mEvent->target = nsnull;

The other option is to add Traverse and Unlink functions to nsEvent and move this code in there. That would put the code closer to where the members are defined (and maybe help remind people to add traversal and unlinking when they add members?). I don't feel strongly about it, so it's up to you.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

whoever adds members to nsEvent, must go through nsDOMEvent code anyway,
at least DuplicatePrivateData. So keepin Unlink and Traverse in nsDOMEvent.

Comment 9

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a6pre) Gecko/20070626 Minefield/3.0a6pre ID:2007062604
No leak reporter now --> VERIFIED