Bug 891218 landed on Fx26, so this probably needs to go all the way up to beta.

Comment on attachment 824728 [details] [diff] [review] Patch Well, this is what smaug said to do, so I guess it is okay.

Comment on attachment 824728 [details] [diff] [review] Patch Based on function makeRemoteBrowser() this looks right, but would be good if a toolkit peer would look at this too.

Comment on attachment 824728 [details] [diff] [review] Patch (In reply to Olli Pettay [:smaug] from comment #4) > Based on function makeRemoteBrowser() this looks right, but > would be good if a toolkit peer would look at this too.

From #developers. I'm not sure if this counts as a review or not. ;) [10:25am] smaug: felipe: want to look at Bug 932867 [10:33am] felipe: smaug: i've seen it, looks fine

Yeah, go for it. This patch seems the right thing to me. Though to be honest I don't think this would be alone responsible for OOMing the tests, as the leaked iframes are empty.. Probably a threshold was just crossed and this happened to be in the way

(This bug is one of three holding the trees closed - bug 932781 comment 11)

Comment on attachment 824728 [details] [diff] [review] Patch https://hg.mozilla.org/integration/mozilla-inbound/rev/e57be1a13a00

Can someone use some words and describe the bug here for posterity? Comment 0 sucks.

The description from the dupe is a bit better: [09:05am] khuey: so the problem with social is that it uses "frame workers" [09:05am] khuey: where it creates a frame in the hidden window and runs code in that [09:06am] khuey: and it's leaving these frames in the hidden window [09:06am] khuey: so the thing I'm trying to figure out now is why Iframes are created in the hidden window, but not actually removed.

To add: inside this iframe, we create a <browser type="content"> which hosts the actual frameworker code. On clean-up (WorkerHandle.terminate), the browser was deleted correctly but not the parent iframe.

(In reply to Andrew McCreight [:mccr8] from comment #9) > Comment on attachment 824728 [details] [diff] [review] > Patch > > https://hg.mozilla.org/integration/mozilla-inbound/rev/e57be1a13a00 5 green Win7 debug mochitest-bc runs on this push is promising :)

Mea culpa :( As Ryan mentioned, this code has existed since Fx26 and we haven't made any changes recently that could account for this being more of a problem today than it was yesterday. And I think we were "only" leaking a few dozen empty iframes during a test run. Has something else happened recently that would account for this somewhat suddenly becoming such a serious problem?

The working theory is that we were already really close to OOM, or even OOMing benignly. And then other innocuous changes (possibly bug 927705) pushed us a tiny bit further, enough that we OOM much more frequently, and in a non-benign way.

With the pushed patch applied and the new/old leak detection from bug 932898 there still are a few leaks left: http://pastebin.mozilla.org/3388446 browser_blocklist.js doesn't actually leak, that's the BrowserNewTabPreloader. browser_share.js also doesn't look like a leak because browser-social.js doesn't destroy the iframe when the share thingy becomes hidden. It looks like there's some code to create the iframe lazily so I wonder if it's maybe better to actually remove it on hide?

(In reply to Tim Taubert [:ttaubert] from comment #16) > With the pushed patch applied and the new/old leak detection from bug 932898 > there still are a few leaks left: > > http://pastebin.mozilla.org/3388446 > > browser_blocklist.js doesn't actually leak, that's the > BrowserNewTabPreloader. browser_share.js also doesn't look like a leak > because browser-social.js doesn't destroy the iframe when the share thingy > becomes hidden. It looks like there's some code to create the iframe lazily > so I wonder if it's maybe better to actually remove it on hide? Other than browser_share.js and browser_social_window.js, the tests are leaking because they are setting a pref that causes Frameworker to set remote=true. If I remove that specific code in Frameworker.jsm, the tests no longer leak. Odd thing is, some other tests that are not leaking also set this pref. Still investigating but maybe markh will have some thoughts on this.

Please make sure to use the latest patch from bug 932898 (v3 currently) when testing. It does now exclude a few known false positives.

using v3... the final leaks are: browser_share.js - it doesn't remove the iframe for share at shutdown - sidebar unloading sets the sidebar frame to about:blank browser_social_window.js - sidebar unloading sets the sidebar frame to about:blank The sidebar is not a dynamically added frame, and hasn't been since it was added in fx17. I feel like that is a false positive. We could remove the share frame when the window is closed.

I've reset the assignee. AIUI, khuey fixed on problem but there are others remaining, and I think those aren't his responsibility. markh or mixedpuppy -- can either of you fix the remaining ones? The trees probably won't reopen until this bug is fixed.

This bug should just stay about the initial issue that khuey fixed. I filed bug 933551 for the remaining bugs mentioned in last few comments.

Fair enough. Thanks, Andrew.

Kyle, I don't suppose you could request approval for aurora/beta as needed - and write the summary etc? :-)

Comment on attachment 824728 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 891218 User impact if declined: Turning off the social API will cause leaks. Turning the social API on and off repeatedly will continue to leak proportionally. Testing completed (on m-c, etc.): On m-c. Risk to taking this patch (and alternatives if risky): Low risk. String or IDL/UUID changes made by this patch: None. NB: Taking this patch fixes a major source of leaks in our test suite, and may result in more stable Mbc runs.

Also note that this is a new regression in 26.

Thank you :-)

https://hg.mozilla.org/releases/mozilla-aurora/rev/b9ec345a1371 https://hg.mozilla.org/releases/mozilla-beta/rev/c70a86fd9e16

If this needs extra QA testing please remove the [qa-] whiteboard tag and add the verifyme keyword.