Closed
Bug 1467179
Opened 6 years ago
Closed 6 years ago
ghost window on washingtonpost.com with youtube embed via MessagePort
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: bkelly, Assigned: baku)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [MemShrink])
Attachments
(1 file, 1 obsolete file)
6.68 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•6 years ago
|
||
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8983918 -
Flags: review?(bkelly)
Reporter | ||
Comment 2•6 years ago
|
||
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-
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8983918 -
Attachment is obsolete: true
Attachment #8984035 -
Flags: review?(bkelly)
Reporter | ||
Comment 4•6 years ago
|
||
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+
Assignee | ||
Comment 5•6 years ago
|
||
No, I'm not able to reproduce it using the test, and checking what the test was doing I saw this 'typo'.
Reporter | ||
Comment 6•6 years ago
|
||
Hmm, ok.
Reporter | ||
Comment 7•6 years ago
|
||
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
Comment 9•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(amarchesini)
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•