Closed Bug 1190921 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 + fixed
firefox42 --- verified
firefox43 --- verified
firefox-esr38 41+ fixed

People

(Reporter: autra, Assigned: smaug)

Details

Attachments

(1 file)

[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.
Mmm, that screams cycle collection bug.  Olli?
In fact, given that there is a strong edge from the observer service to the broadcast channel, how does it ever get cycle collected?
Flags: needinfo?(bugs)
To add some clarification: the iframes are freed correctly if 
> port.foo = 'leak';
is removed.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Flags: needinfo?(bugs) → needinfo?(amarchesini)
Does this mean we are we not getting nsGlobalWindow::FreeInnerObjects calls? That would be rather ugly bug.
Oh oh, there is definitely some bug in BC impl
Assignee: nobody → bugs
Attached patch patchSplinter Review
Attachment #8643326 - Flags: review?(khuey)
We really must remove the observer always, since we add it always.
Comment on attachment 8643326 [details] [diff] [review]
patch

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

Doh!
Attachment #8643326 - Flags: review?(khuey) → review+
[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
https://hg.mozilla.org/mozilla-central/rev/d3c82823fb48
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Flags: needinfo?(amarchesini)
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
Attachment #8643326 - Flags: approval-mozilla-esr38?
Attachment #8643326 - Flags: approval-mozilla-aurora?
(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.
Augustin, could you please verify the fix on the latest Nighlty? The leak should be fixed. Thanks!
Flags: needinfo?(augustin.trancart)
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.
Attachment #8643326 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Seems perfect! Btw can I set that to VERIFIED myself? Or is it reserved for QA people?
Flags: needinfo?(augustin.trancart)
(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!
Ok!

So VERIFIED FIXED against Firefox 43.0a1 (2015-08-12).
Status: RESOLVED → VERIFIED
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.
Attachment #8643326 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
You need to log in before you can comment on or make changes to this bug.