Closed Bug 1277722 Opened 4 years ago Closed 4 years ago

youtube window leaked through Touch object

Categories

(Core :: DOM: Events, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
platform-rel --- ?
firefox50 --- fixed

People

(Reporter: bkelly, Assigned: smaug)

Details

(Whiteboard: [MemShrink:P1] [platform-rel-Youtube])

Attachments

(1 file)

I got a ghost window in fennec 46.0.1 on my nexus 5x:

cc-edges.9200.1464909131.log:0xc6ba4800 [rc=51] nsGlobalWindow #136 inner https://www.youtube.com/embed/cmGr0RszHc8

Which seems to be leaked through a Touch object:

$ /c/devel/heapgraph/find_roots.py cc-edges.9200.1464909131.log 0xc6ba4800
Parsing cc-edges.9200.1464909131.log. Done loading graph.

0xb41d1190 [Touch]
    --[mTarget]--> 0xc1ea26f0 [FragmentOrElement (xhtml) div class='ytp-thumbnail-overlay ytp-cued-t
humbnail-overlay' (orphan) https://www.youtube.com/embed/cmGr0RszHc8]
    --[mNodeInfo]--> 0xcba5ba10 [NodeInfo (xhtml) div]
    --[mOwnerManager]--> 0xc4e871c0 [nsNodeInfoManager]
    --[mDocument]--> 0xc315c800 [nsDocument normal (xhtml) https://www.youtube.com/embed/cmGr0RszHc8
]
    --[mDocumentTimeline]--> 0xcc4c7ac0 [AnimationTimeline]
    --[mWindow]--> 0xc6ba4800 [nsGlobalWindow #136 inner https://www.youtube.com/embed/cmGr0RszHc8]

    Root 0xb41d1190 is a ref counted object with 1 unknown edge(s).
    known edges:
       0xb973a980 [TouchList] --[mPoints[i]]--> 0xb41d1190

I'm not sure if it matters, but that Touch object is in a very large cycle with the TouchList:

$ /c/devel/heapgraph/find_roots.py cc-edges.9200.1464909131.log 0xb973a980
Parsing cc-edges.9200.1464909131.log. Done loading graph.

0xb41d1190 [Touch]
    --[mTarget]--> 0xc1ea26f0 [FragmentOrElement (xhtml) div class='ytp-thumbnail-overlay ytp-cued-t
humbnail-overlay' (orphan) https://www.youtube.com/embed/cmGr0RszHc8]
    --[mNodeInfo]--> 0xcba5ba10 [NodeInfo (xhtml) div]
    --[mOwnerManager]--> 0xc4e871c0 [nsNodeInfoManager]
    --[mDocument]--> 0xc315c800 [nsDocument normal (xhtml) https://www.youtube.com/embed/cmGr0RszHc8
]
    --[mListenerManager]--> 0xcbceda60 [EventListenerManager]
    --[mListeners event=onmouseup listenerType=3 [i]]--> 0xcc277d00 [CallbackObject]
    --[mCallback]--> 0xc44922c0 [JS Object (Function - M/<)]
    --[fun_environment]--> 0xd350f100 [JS Object (Call)]
    --[y]--> 0xc24c5d30 [JS Object (Object)]
    --[F]--> 0xc335bb10 [JS Object (Array)]
    --[objectElements[243]]--> 0xc70580c0 [JS Object (Function - z/r)]
    --[fun_environment]--> 0xca00d3c0 [JS Object (Call)]
    --[h]--> 0xc7058da0 [JS Object (Function - z/h<)]
    --[objectElements[36]]--> 0xc2c7a190 [JS Object (Arguments)]
    --[arguments[0]]--> 0xc5ebb0e0 [JS Object (TouchEvent)]
    --[UnwrapDOMObject(obj)]--> 0xb41d1200 [Event]
    --[mChangedTouches]--> 0xb973a980 [TouchList]

    Root 0xb41d1190 is a ref counted object with 1 unknown edge(s).
    known edges:
       0xb973a980 [TouchList] --[mPoints[i]]--> 0xb41d1190

I think CC should clean that up and its just the unknown edge holding it alive, though.  Olli, is that correct?
Flags: needinfo?(bugs)
If CC knows about all the edges in some cycle, it can clean up the stuff.

My guess is that gCaptureTouchList in TouchManager has the reference.
TouchManager::PreHandleEvent should evict such touch when starting a new touch transaction.
Though, I wonder if we should always explicitly remove the touch from gCaptureTouchList in EvictTouchPoint and not rely on dispatching eTouchEnd which then causes remove to be called in TouchManager::PreHandleEvent.
Flags: needinfo?(bugs)
platform-rel: --- → ?
Whiteboard: [MemShrink] → [MemShrink] [platform-rel-Youtube]
Ghost windows on youtube seems pretty bad, Olli do you know who can look into this further?
Flags: needinfo?(bugs)
Whiteboard: [MemShrink] [platform-rel-Youtube] → [MemShrink:P1] [platform-rel-Youtube]
bkelly, can you perhaps reproduce this somehow?
I could write a possible patch, based on the guess I explained in comment 1.
Flags: needinfo?(bugs) → needinfo?(bkelly)
I think I should just make Touch object supporting weak refs.
Unfortunately I don't have STR this.  Its something I noticed running about:memory on fennec at one point.  Unfortunately I don't check about:memory as frequently there.
Flags: needinfo?(bkelly)
Attached patch guess fixSplinter Review
Whoever reviews need to go through the relevant code, and no one has been hacking that lately, so perhaps masayuki you could do it.

I believe this leak should be still just temporary, and a new touch transaction would evict all touches.

Currently if eTouchEnd is dispatched, we try to remove the touch in
TouchManager::PreHandleEvent, if we ever end up to that code.
So this patch should just ensure that even if dispatching the event somehow fails, we end up removing the touch from the list.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=37cb1d5491c0bd0f762695ff954a89b171323d3c
Assignee: nobody → bugs
Attachment #8767294 - Flags: review?(masayuki)
Even with the patch it is possible that the hashtable keeps Touch objects alive a bit too long, but at least it should be guaranteed that they are evicted when the next transaction starts.
And couldn't use the weakref approach, since need to keep the Touch objects alive during transaction.
Comment on attachment 8767294 [details] [diff] [review]
guess fix

I tried to understand why there is the return.

It was added by you at this changeset:
https://hg.mozilla.org/mozilla-central/diff/a6b5e42fb847/layout/base/nsPresShell.cpp#l6200
It keeps original behavior in most causes but adding the cleaning up when it's impossible to dispatch eTouchEnd. So, the return statement was not appended later for any specific purposes.

Looks like that the function assumes that dispatching eTouchEnd causes cleaning up but that's not so in some cases like this bug.

So, I believe that this is safe fix.
Attachment #8767294 - Flags: review?(masayuki) → review+
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2d57054f83
release touch objects even if dispatching touchend fails, r=masayuki
https://hg.mozilla.org/mozilla-central/rev/5d2d57054f83
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.