Ben Kelly [:bkelly, not reviewing] Reporter
	Description • 8 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 1 • 8 years ago

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.

	Eric Rahm [:erahm]
	Comment 2 • 8 years ago

Ghost windows on youtube seems pretty bad, Olli do you know who can look into this further?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 3 • 8 years ago

bkelly, can you perhaps reproduce this somehow?
I could write a possible patch, based on the guess I explained in comment 1.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 4 • 8 years ago

I think I should just make Touch object supporting weak refs.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 5 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 6 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 7 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 8 • 8 years ago

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.

	Pulsebot
	Comment 9 • 8 years ago

Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2d57054f83
release touch objects even if dispatching touchend fails, r=masayuki

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5d2d57054f83