337050 - Extension manager shutdown leaks

Status: RESOLVED WORKSFORME

(Toolkit :: Add-ons Manager, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD:  Current trunk Firefox debug build with GC_MARK_DEBUG defined

STEPS TO REPRODUCE:
1)  Start up firefox
2)  Quit

ACTUAL RESULTS:

Lots of GC leaks looking like:

081e6578 object 0x81f2bd0 Function via nsXPCWrappedJS::mJSObj[nsIObserver,0x8232088,0x81e7a28](Object @ 0x081e7a28).__proto__(Object @ 0x081e78f0)._confirmCancelDownloadsOnQuit(Function @ 0x081e6578).

Here the observer's __proto__ has a _confirmCancelDownloadsOnQuit, so the observer is the ExtensionManager.

I doubt that this code (XPConnect shutdown) is happening before ExtensionManager gets the xpcom-shutdown notification (if it did, we wouldn't exactly be able to call into C++ correctly).  So I'm guessing we're forgetting to remove ExternsionManager as an observer from somewhere.  No idea where.

Comment 1

[Robert Strong (they/them - no direct email)]

In case it isn't clear why I'd like you to verify this... this will assist in verifying there isn't anything else happening besides the absolute paths in extensions.ini causing this problem for you.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er... What does comment 1 have to do with this bug?  

Comment 3

[Robert Strong (they/them - no direct email)]

Sorry, wrong bug

Comment 4

[Robert Strong (they/them - no direct email)]

For reference, bug 286034 landed the changes for adding and removing observers. Bug 329045 includes one fix where the EM would repeatedly add observers and only remove them if there was a downloadload in progress on shutdown which was I believe was also introduced by bug 286034.

Comment 5

[Mike Connor [:mconnor]]

+ for eval

Comment 6

[Michael Wu [:mwu]]

Should this be blocking? I can reproduce it easily on trunk, but I'm not seeing it on branch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I rather assumed that since the UI is the same on branch and trunk the issue would be present on branch too.  If it's not, that's all good.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Over in bug 385237, I figured out that the cause of a content pref service leak is the nsIFactory that creates the service.  Some testing leads me to conclude that the factories in every JS component are likely to be leaking, which may be the cause of (some of) the leaks reported in the bugs to which I am adding this comment (bug 337050, bug 378618, bug 381239, and bug 380873/bug 380931).

Take a look at bug 385237, comment 2 for more details, and note that the fix for bug 180380 makes all the XPCWrappedNative and XPCWrappedNativeProto leaks (which were what the content pref service was leaking) go away and may similarly fix (some of) the leaks reported in this bug.

Comment 9

[Robert Strong (they/them - no direct email)]

Thanks Myk!

Comment 10

[Mike Connor [:mconnor]]

mwu, can you see if you can still repro now that bug 180380 is fixed?

Comment 11

[Michael Wu [:mwu]]

GC_MARK_DEBUG no longer exists, so I used its replacement - XPC_SHUTDOWN_HEAP_DUMP. I couldn't find any evidence of extension manager leaking from those logs, though I understand the output of XPC_SHUTDOWN_HEAP_DUMP even less than GC_MARK_DEBUG. Bloat logs also indicate no problems, so I don't think extension manager is leaking. bz, do you have any other suggestions?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yeah, this worksforme now.