1190921 - Broadcastchannel keeps its containing iframe in memory when it has a user defined property.

Status: VERIFIED FIXED

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

Description

[Augustin Trancart [:autra]]

[Steps]
- visit http://autra.github.io/bug_test_case/iframe_bc_property/
this page creates 1000 iframes containing a broadcastchannel and defining a property 'foo' on this broadcastchannel, then remove all these iframes from the dom.
- open about:memory, click on GC, CC, minimize memory, and inspect the Web Content memory.

[Expected]
The iframes are freed.

[Actual]
The iframes stay in memory in 'detached' mode.

Comment 1

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

Mmm, that screams cycle collection bug.  Olli?

Comment 2

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

In fact, given that there is a strong edge from the observer service to the broadcast channel, how does it ever get cycle collected?

Comment 3

[Augustin Trancart [:autra]]

To add some clarification: the iframes are freed correctly if 
> port.foo = 'leak';
is removed.

Comment 4

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> In fact, given that there is a strong edge from the observer service to the
> broadcast channel, how does it ever get cycle collected?
Currently only when the relevant inner window goes away.
In general BC would need cross-thread/process CC.

But let me think if we can have some hack for this case.

Comment 5

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

Does this mean we are we not getting nsGlobalWindow::FreeInnerObjects calls? That would be rather ugly bug.

Comment 6

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

Oh oh, there is definitely some bug in BC impl

Comment 7

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

Attachment: patch

Comment 8

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

We really must remove the observer always, since we add it always.

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21777ea56fde

Comment 10

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

Comment on attachment 8643326 [details] [diff] [review]
patch

Review of attachment 8643326 [details] [diff] [review]:
-----------------------------------------------------------------

Doh!

Comment 11

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

[Tracking Requested - why for this release]:
Leaks, and looks like this is in all the branches >= 38
https://bugzilla.mozilla.org/show_bug.cgi?id=1121420

Comment 12

[Pulsebot]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

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

Comment on attachment 8643326 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Leaks, leading to higher cycle collection times.
User impact if declined: jank
Fix Landed on Version:32
Risk to taking this patch (and alternatives if risky): Should be very safe
String or UUID changes made by this patch: NA
[Feature/regressing bug #]: Bug 1121420

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Olli Pettay [:smaug] (vacation Aug 11 - Aug 18) from comment #14)
> Comment on attachment 8643326 [details] [diff] [review]
> patch
> 
> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR
> consideration:
> Leaks, leading to higher cycle collection times.
> User impact if declined: jank
> Fix Landed on Version:32
> Risk to taking this patch (and alternatives if risky): Should be very safe
> String or UUID changes made by this patch: NA
> [Feature/regressing bug #]: Bug 1121420

I think the fix landed on version should be 42 and not 32.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for FF41. Memory leaks are bad.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Augustin, could you please verify the fix on the latest Nighlty? The leak should be fixed. Thanks!

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8643326 [details] [diff] [review]
patch

[Triage Comment]
Patch seems safe, and memory leak fixes meet the beta release bar. There is also an automated test. Approved.

Comment 19

[Augustin Trancart [:autra]]

Seems perfect! Btw can I set that to VERIFIED myself? Or is it reserved for QA people?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a1ebb5bb64a4

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Augustin Trancart [:autra] from comment #19)
> Seems perfect! Btw can I set that to VERIFIED myself? Or is it reserved for
> QA people?

Augustin, you can set it to "Verified" and leave a comment that confirms which version you verified it against. I can do it on your behalf this time. 

And thank you for doing the verification!

Comment 22

[Augustin Trancart [:autra]]

Ok!

So VERIFIED FIXED against Firefox 43.0a1 (2015-08-12).

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8643326 [details] [diff] [review]
patch

Seems stable, fix verified on beta, includes a test. 
Taking this for ESR, fix for a bad memory leak.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/947b193e588a