ghost window on washingtonpost.com with youtube embed via MessagePort

RESOLVED FIXED in Firefox 62

Status

()

defect
RESOLVED FIXED
Last year
5 months ago

People

(Reporter: bkelly, Assigned: baku)

Tracking

(Blocks 2 bugs)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment, 1 obsolete attachment)

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?
Flags: needinfo?(amarchesini)
Posted patch mp.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8983918 - Flags: review?(bkelly)
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
Attachment #8983918 - Flags: review?(bkelly) → review-
Posted patch mp.patchSplinter Review
Attachment #8983918 - Attachment is obsolete: true
Attachment #8984035 - Flags: review?(bkelly)
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.
Attachment #8984035 - Flags: review?(bkelly) → review+
No, I'm not able to reproduce it using the test, and checking what the test was doing I saw this 'typo'.
Hmm, ok.
Andrea, are you waiting to land this?  Just want to make sure it makes it into the merge next week.  Thanks.
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b352ed03f7
MessagePort should implement DisconnectFromOwner() instead using innerID comparison, r=bkelly
https://hg.mozilla.org/mozilla-central/rev/e4b352ed03f7
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: needinfo?(amarchesini)
Blocks: 1471240
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.