932867 - SocialAPI leaks the host iframe for the frameworker

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 2

[Ryan VanderMeulen [:RyanVM]]

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

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 824728 [details] [diff] [review]
Patch

Well, this is what smaug said to do, so I guess it is okay.

Comment 4

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

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 5

[Ed Morley [:emorley]]

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.

Comment 6

[Andrew McCreight [:mccr8]]

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

Comment 7

[:Felipe Gomes (needinfo for replies!)]

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

Comment 8

[Ed Morley [:emorley]]

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

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 824728 [details] [diff] [review]
Patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/e57be1a13a00

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(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 :)

Comment 14

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 15

[Nicholas Nethercote [inactive]]

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.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 17

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 19

[Shane Caraveo (:mixedpuppy)]

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.

Comment 20

[Nicholas Nethercote [inactive]]

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.

Comment 21

[Andrew McCreight [:mccr8]]

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.

Comment 22

[Nicholas Nethercote [inactive]]

Fair enough.  Thanks, Andrew.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e57be1a13a00

Comment 24

[Ed Morley [:emorley]]

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

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Also note that this is a new regression in 26.

Comment 27

[Ed Morley [:emorley]]

Thank you :-)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c70a86fd9e16

Comment 30

[u279076]

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