1467179 - ghost window on washingtonpost.com with youtube embed via MessagePort

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Today I got a set of ghost windows like:

├───18.86 MB (12.21%) -- window-objects
│   ├──18.50 MB (11.98%) -- top(none)/ghost
│   │  ├──12.54 MB (08.11%) ++ window(https://www.washingtonpost.com/)
│   │  ├───3.89 MB (02.52%) ++ window(https://www.youtube.com/embed/Lu1k5gikMpo?autohide=1&autoplay=1&controls=1&fs=1&iv_load_policy=3&modestbranding=1&rel=0&showinfo=0&wmode=transparent&playsinline=1&enablejsapi=1&origin=https%3A%2F%2Fwww.washingtonpost.com&widgetid=1)
│   │  └───2.08 MB (01.35%) -- (2 tiny)
│   │      ├──1.38 MB (00.89%) ++ window(https://imasdk.googleapis.com/js/core/bridge3.211.3_en.html#goog_165210117)
│   │      └──0.70 MB (00.45%) ++ window(https://cdn.krxd.net/partnerjs/xdi/proxy.3d2100fd7107262ecb55ce6847f01fa5.html#!kxcid=IbWIJ0xh&kxt=https%3A%2F%2Fwww.washingtonpost.com&kxcl=cdn&kxp=)

Tracing the washingtonpost.com window suggests its related to MessagePort in some way:

00000170A0E9DBB0 [MessagePort https://www.youtube.com/embed/Lu1k5gikMpo?autohide=1&autoplay=1&controls=1&fs=1&iv_load_policy=3&modestbranding=1&rel=0&showinfo=0&wmode=transparent&playsinline=1&enablejsapi=1&origin=https%3A%2F%2Fwww.washingtonpost.com&widgetid=1]
    --[mListenerManager]--> 000001709F1DFAF0 [EventListenerManager]
    --[mListeners event=onmessage listenerType=2 [i]]--> 000001709F91DA60 [JSEventHandler handlerName=onmessage]
    --[mTypedHandler.Ptr()]--> 000001709F91D700 [CallbackObject]
    --[mIncumbentGlobal]--> 000001709F289000 [nsGlobalWindowInner # 2147484064 inner https://www.youtube.com/embed/Lu1k5gikMpo?autohide=1&autoplay=1&controls=1&fs=1&iv_load_policy=3&modestbranding=1&rel=0&showinfo=0&wmode=transparent&playsinline=1&enablejsapi=1&origin=https%3A%2F%2Fwww.washingtonpost.com&widgetid=1]
    --[mTopInnerWindow]--> 000001708D108C00 [nsGlobalWindowInner # 2147484056 inner https://www.washingtonpost.com/]

    Root 00000170A0E9DBB0 is a ref counted object with 2 unknown edge(s).
    known edges:
       000001709FCC0100 [MessageChannel] --[mPort1]--> 00000170A0E9DBB0
       00000170A0E9DD50 [MessagePort https://www.youtube.com/embed/Lu1k5gikMpo?autohide=1&autoplay=1&controls=1&fs=1&iv_load_policy=3&modestbranding=1&rel=0&showinfo=0&wmode=transparent&playsinline=1&enablejsapi=1&origin=https%3A%2F%2Fwww.washingtonpost.com&widgetid=1] --[mUnshippedEntangledPort]--> 00000170A0E9DBB0

If I had to guess this might be an issue with how the MessagePort detects the window going away:

https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/dom/messagechannel/MessagePort.cpp#891

If document.open() has been used (possible with ads and such), then the mInnerID may not reflect its actual window any more.  MessagePort probably needs to replace the observer with an override of DisconnectFromOwner().

Andrea, what do you think?

Comment 1

[Andrea Marchesini [:baku]]

Attachment: mp.patch

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8983918 [details] [diff] [review]
mp.patch

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

This needs to call DOMEventTargetHelper::DisconnectFromOwner() from the override.

Also, it would be nice if we could get this to reproduce in a test.  I wonder why the test here does not trigger it:

https://searchfox.org/mozilla-central/source/dom/messagechannel/tests/test_event_listener_leaks.html

Comment 3

[Andrea Marchesini [:baku]]

Attachment: mp.patch

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8984035 [details] [diff] [review]
mp.patch

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

Looks good, but were you able to reproduce the leak in the test at all?  The test changes seemed cosmetic.

Comment 5

[Andrea Marchesini [:baku]]

No, I'm not able to reproduce it using the test, and checking what the test was doing I saw this 'typo'.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Hmm, ok.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Andrea, are you waiting to land this?  Just want to make sure it makes it into the merge next week.  Thanks.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b352ed03f7
MessagePort should implement DisconnectFromOwner() instead using innerID comparison, r=bkelly

Comment 9

[Narcis Beleuzu [:NarcisB]]

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