Closed Bug 1467179 Opened 2 years ago Closed 2 years ago

ghost window on with youtube embed via MessagePort


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

Not set



Tracking Status
firefox62 --- fixed


(Reporter: bkelly, Assigned: baku)


(Blocks 2 open bugs)


(Whiteboard: [MemShrink])


(1 file, 1 obsolete file)

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(
│   │  ├───3.89 MB (02.52%) ++ window(
│   │  └───2.08 MB (01.35%) -- (2 tiny)
│   │      ├──1.38 MB (00.89%) ++ window(
│   │      └──0.70 MB (00.45%) ++ window(!kxcid=IbWIJ0xh&

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

00000170A0E9DBB0 [MessagePort]
    --[mListenerManager]--> 000001709F1DFAF0 [EventListenerManager]
    --[mListeners event=onmessage listenerType=2 [i]]--> 000001709F91DA60 [JSEventHandler handlerName=onmessage]
    --[mTypedHandler.Ptr()]--> 000001709F91D700 [CallbackObject]
    --[mIncumbentGlobal]--> 000001709F289000 [nsGlobalWindowInner # 2147484064 inner]
    --[mTopInnerWindow]--> 000001708D108C00 [nsGlobalWindowInner # 2147484056 inner]

    Root 00000170A0E9DBB0 is a ref counted object with 2 unknown edge(s).
    known edges:
       000001709FCC0100 [MessageChannel] --[mPort1]--> 00000170A0E9DBB0
       00000170A0E9DD50 [MessagePort] --[mUnshippedEntangledPort]--> 00000170A0E9DBB0

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

If 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)
Attached patch mp.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8983918 - Flags: review?(bkelly)
Comment on attachment 8983918 [details] [diff] [review]

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:
Attachment #8983918 - Flags: review?(bkelly) → review-
Attached patch mp.patchSplinter Review
Attachment #8983918 - Attachment is obsolete: true
Attachment #8984035 - Flags: review?(bkelly)
Comment on attachment 8984035 [details] [diff] [review]

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
MessagePort should implement DisconnectFromOwner() instead using innerID comparison, r=bkelly
Closed: 2 years ago
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.