Andrew McCreight [:mccr8] Reporter
	Description • 9 years ago

I have a bunch of ghost windows from Google plus.

Here's the start of the path entraining one of the nsGlobalWindows:

0x136659480 [DOMEventTargetHelper https://plus.google.com/u/1/_/notifications/frame?sourceid=23&hl=en&origin=https%3A%2F%2Fmail.google.com&uc=1&jsh=m%3B%2F_%2Fscs%2Fabc-static%2F_%2Fjs%2Fk%3Dgapi.gapi.en.6zwg_pEP_2A.O%2Fm%3D__features__%2Fam%3DAAQ%2Frt%3Dj%2Fd%3D1%2Frs%3DAItRSTNy-00ELgURzlKy-V-78pEvv5BpHA#pid=23&rpctoken=424140231&_methods=onError%2ConInfo%2ChideNotificationWidget%2CpostSharedMessage%2Creauth%2CsetNotificationWidgetHeight%2CsetNotificationWidgetSize%2CswitchTo%2CnavigateTo%2CsetNotificationText%2CsetNo]
    --[mListenerManager]--> 0x1392b2310 [EventListenerManager]
    --[mListeners event=onmessage listenerType=1 [i]]--> 0x13abdc970 [JSEventHandler handlerName=onmessage]
    --[mTypedHandler.Ptr()]--> 0x13abdc940 [CallbackObject]
    --[mCallback]--> 0x1616a5700 [JS Object (Function - Mda/b.port1.onmessage)]
    --[group_function, function]--> 0x164a96ec0 [JS Object (Function - Mda/b.port1.onmessage)]

    Root 0x136659480 is a ref counted object with 1 unknown edge(s).
    known edges:
       0x135f63580 [MessageChannel] --[mPort1]--> 0x136659480

I'll see if I can figure out how to reproduce it. You have to look for window/detached because until you close gmail it won't be treated as a ghost window.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 9 years ago

Just curious, is this e10s or non-e10s?

Though, that probably doesn't matter.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 9 years ago

baku, could you take a look at this?

What happens to MessagePorts between windowA and windowB when windowA is closed?

	Andrea Marchesini [:baku] Assignee
	Comment 3 • 9 years ago

When WindowA is closed, MessagePort receives a inner-window-destroyed notification and it calls Close().
At this point we send a PBackground message to close the other side and we release the MessagePort.
When the port hosted in windowB receives the Close(), we release that port.
Similar story if we have a worker. And we have some mochitest for this.

Would be nice if we can reproduce it somehow. Maybe we can debug it together today.

	Andrew McCreight [:mccr8] Reporter
	Comment 4 • 9 years ago

I found a way to consistently reproduce something that looks like this:
1. Go to washingtonpost.com, scroll down to the bottom so it all starts loading up, then up to the top.
2. Click on a news story, scroll down to the bottom then back up to the top.
3. While it is still loading, close the tab.

This causes both washingtonpost.com and the news story pages to be leaked.

	Andrew McCreight [:mccr8] Reporter
	Comment 5 • 9 years ago

Now that there is STR maybe baku could investigate. :)

	Andrew McCreight [:mccr8] Reporter
	Comment 6 • 9 years ago

I can reproduce this both with and without e10s.

[Tracking Requested - why for this release]: severe memory leaks

	Andrea Marchesini [:baku] Assignee
	Comment 7 • 9 years ago

The issue is that, when 1 MessageChannel is created, 2 MessagePorts are created too and they know each other to optimize postMessages in the same window.
They both are observers for inner-window-destroy.
When the window is closed, the first port receives the notification and it close itself and the other port. At this point we forget to call RemoveObserver.

This patch does:
1. RemoveObserver is called before call the manual Release(). Note: MessagePort needs to do this because we don't have multi-thread CC.
2. ::Observe() must be received only when the MessagePort is keeping itself alive.

	Andrew McCreight [:mccr8] Reporter
	Comment 9 • 9 years ago

Comment on attachment 8626924 [details] [diff] [review]
mp2.patch

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

Sounds reasonable to me. Thanks for the quick fix.

	Andrea Marchesini [:baku] Assignee
	Comment 10 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 11 • 9 years ago

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

	Andrew McCreight [:mccr8] Reporter
	Comment 13 • 9 years ago

I'm going to retroactively mark this as a MemShrink P1.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 15 • 9 years ago

Adding a tracking flag for FF42 as this a memory leak issue.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 16 • 9 years ago

Andrew, I am a little confused whether the fix for this bug is in FF42 or not. This landed on 6/29 in m-c, I believe that was FF41 at that point in time. So, is this fix in 41 and 42 both? Or just 41?

	Andrew McCreight [:mccr8] Reporter
	Comment 17 • 9 years ago

That's odd. I guess I got confused somehow. You are right, it looks like landed on m-c when it was 41, so 42 shouldn't be affected.

Here's the patch in Aurora and m-c:
https://hg.mozilla.org/releases/mozilla-aurora/rev/de8d49d52430
https://hg.mozilla.org/mozilla-central/rev/de8d49d52430