Media: fix memory leak because of the document holding on an EventListener.

Media: fix memory leak because of the document holding on an EventListener.

MediaControls checks if they are part of a document or not in order to unregister
most of its event listeners when removed from a document to avoid leaking memory
until the document is closed.

The approach taken in this CL is the most "web based" because the controls are
using events. Another approach would be to use internal callbacks (from
ContainerNode) or even have a `addWeakEventListener` method in EventTarget. The
latter might be an interesting solution.

BUG=680898
R=zqzhang@chromium.org,ojan@chromium.org

Review-Url: https://codereview.chromium.org/2725873002
Cr-Commit-Position: refs/heads/master@{#454060}
Committed: https://chromium.googlesource.com/chromium/src/+/8daac32c17f494d496fb2ca3768d67d9bd77db5e

[mlamouri (slow - plz ping), 2017-03-01 15:28:59]

zqzhang@, can you take an early look? The CL makes it so the HTMLMediaElement
destroys the MediaControls when it's removed from a document. The downside is
that we have to re-create a new instance when we are inserted again but that has
the benefit of being a safe fix (need to be sent to beta and maybe stable).

[Zhiqiang Zhang (Slow), 2017-03-01 15:37:03]

sgtm in general.

One question:
In case the element is inserted back to the document, are we sure that
ensureMediaControls() will be called so we create a new MediaControls?

[Zhiqiang Zhang (Slow), 2017-03-01 16:22:37]

https://codereview.chromium.org/2725873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp
(right):

https://codereview.chromium.org/2725873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:18:
mediaElement().addEventListener(EventTypeNames::volumechange, this, false);
Same concern as MediaControlsOrientationLockDelegate, when we add/remove the
element multiple times, we will create many listeners and they will be always
alive until the element is gc-ed.

https://codereview.chromium.org/2725873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2725873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:79:
document().removeEventListener(EventTypeNames::fullscreenchange, this, true);
Should we also remove the listeners from the video element? In case the page
adds/removes the element to/from the page multiple times, we are creating many
MediaControl objects and this will prevent the
MediaControlsOrientationLockDelegate objects to be gc-ed.

[mlamouri (slow - plz ping), 2017-03-01 20:18:55]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 20:19:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2017-03-01 20:22:01]

Description was changed from

==========
Media: fix memory leak because of the document holding on an EventListener.

<DESCRIPTION TODO>

BUG=680898
R=zqzhang@chromium.org
==========

to

==========
Media: fix memory leak because of the document holding on an EventListener.

MediaControls checks if they are part of a document or not in order to
unregister
most of its event listeners when removed from a document to avoid leaking memory
until the document is closed.

The approach taken in this CL is the most "web based" because the controls are
using events. Another approach would be to use internal callbacks (from
ContainerNode) or even have a `addWeakEventListener` method in EventTarget. The
latter might be an interesting solution.

BUG=680898
R=zqzhang@chromium.org,ojan@chromium.org
==========

[mlamouri (slow - plz ping), 2017-03-01 20:22:33]

mlamouri@chromium.org changed reviewers:
+ ojan@chromium.org

[mlamouri (slow - plz ping), 2017-03-01 20:22:33]

zqzhang@ PTAL

+ojan@ for third_party/WebKit/Source/core/BUILD.gn

https://codereview.chromium.org/2725873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp
(right):

https://codereview.chromium.org/2725873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:18:
mediaElement().addEventListener(EventTypeNames::volumechange, this, false);
On 2017/03/01 at 16:22:36, Zhiqiang Zhang wrote:
> Same concern as MediaControlsOrientationLockDelegate, when we add/remove the
element multiple times, we will create many listeners and they will be always
alive until the element is gc-ed.

Ack.

https://codereview.chromium.org/2725873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2725873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:79:
document().removeEventListener(EventTypeNames::fullscreenchange, this, true);
On 2017/03/01 at 16:22:36, Zhiqiang Zhang wrote:
> Should we also remove the listeners from the video element? In case the page
adds/removes the element to/from the page multiple times, we are creating many
MediaControl objects and this will prevent the
MediaControlsOrientationLockDelegate objects to be gc-ed.

Good point :) Though, the approach the first iteration was taking didn't work so
I had to change.

[ojan, 2017-03-01 21:02:50]

lgtm

[Zhiqiang Zhang (Slow), 2017-03-01 21:20:52]

lgtm w/ nit

https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp
(right):

https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp:66:
document().body()->appendChild(video());
Can GC happen between these two lines? Maybe keep a strong reference before
removing?

[mlamouri (slow - plz ping), 2017-03-01 21:44:01]

https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp
(right):

https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp:66:
document().body()->appendChild(video());
On 2017/03/01 at 21:20:52, Zhiqiang Zhang wrote:
> Can GC happen between these two lines? Maybe keep a strong reference before
removing?

I don't think it is expected to happen in the middle of C++ code. Especially for
tests :)

[mlamouri (slow - plz ping), 2017-03-01 21:44:18]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-03-01 21:44:20]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-03-01 21:45:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2017-03-01 21:45:35]

https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp
(right):

https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp:66:
document().body()->appendChild(video());
On 2017/03/01 21:44:01, mlamouri wrote:
> On 2017/03/01 at 21:20:52, Zhiqiang Zhang wrote:
> > Can GC happen between these two lines? Maybe keep a strong reference before
> removing?
> 
> I don't think it is expected to happen in the middle of C++ code. Especially
for
> tests :)

Conservative GCs can happen at any allocation.

[mlamouri (slow - plz ping), 2017-03-01 21:48:01]

On 2017/03/01 at 21:45:35, sigbjornf wrote:
>
https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp
(right):
> 
>
https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp:66:
document().body()->appendChild(video());
> On 2017/03/01 21:44:01, mlamouri wrote:
> > On 2017/03/01 at 21:20:52, Zhiqiang Zhang wrote:
> > > Can GC happen between these two lines? Maybe keep a strong reference
before
> > removing?
> > 
> > I don't think it is expected to happen in the middle of C++ code. Especially
for
> > tests :)
> 
> Conservative GCs can happen at any allocation.

You mean, in this exact situation?

[sof, 2017-03-01 21:50:31]

On 2017/03/01 21:48:01, mlamouri wrote:
> On 2017/03/01 at 21:45:35, sigbjornf wrote:
> >
>
https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp:66:
> document().body()->appendChild(video());
> > On 2017/03/01 21:44:01, mlamouri wrote:
> > > On 2017/03/01 at 21:20:52, Zhiqiang Zhang wrote:
> > > > Can GC happen between these two lines? Maybe keep a strong reference
> before
> > > removing?
> > > 
> > > I don't think it is expected to happen in the middle of C++ code.
Especially
> for
> > > tests :)
> > 
> > Conservative GCs can happen at any allocation.
> 
> You mean, in this exact situation?

removeChild() will allocate, or do you know that it won't?

[mlamouri (slow - plz ping), 2017-03-01 22:05:46]

On 2017/03/01 at 21:50:31, sigbjornf wrote:
> On 2017/03/01 21:48:01, mlamouri wrote:
> > On 2017/03/01 at 21:45:35, sigbjornf wrote:
> > >
> >
https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2725873002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp:66:
> > document().body()->appendChild(video());
> > > On 2017/03/01 21:44:01, mlamouri wrote:
> > > > On 2017/03/01 at 21:20:52, Zhiqiang Zhang wrote:
> > > > > Can GC happen between these two lines? Maybe keep a strong reference
> > before
> > > > removing?
> > > > 
> > > > I don't think it is expected to happen in the middle of C++ code.
Especially
> > for
> > > > tests :)
> > > 
> > > Conservative GCs can happen at any allocation.
> > 
> > You mean, in this exact situation?
> 
> removeChild() will allocate, or do you know that it won't?

I will write a quick test change tomorrow morning to be on the safe side. This
fix is urgent so I don't want to slow down the CQ. Though, I would be *very*
surprised if we ever GC between these two calls.

[commit-bot: I haz the power, 2017-03-01 22:10:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488404660413100,
"parent_rev": "1337a0b763dc552f6eeadb17df348429b34a3501", "commit_rev":
"8daac32c17f494d496fb2ca3768d67d9bd77db5e"}

[commit-bot: I haz the power, 2017-03-01 22:10:50]

Description was changed from

==========
Media: fix memory leak because of the document holding on an EventListener.

MediaControls checks if they are part of a document or not in order to
unregister
most of its event listeners when removed from a document to avoid leaking memory
until the document is closed.

The approach taken in this CL is the most "web based" because the controls are
using events. Another approach would be to use internal callbacks (from
ContainerNode) or even have a `addWeakEventListener` method in EventTarget. The
latter might be an interesting solution.

BUG=680898
R=zqzhang@chromium.org,ojan@chromium.org
==========

to

==========
Media: fix memory leak because of the document holding on an EventListener.

MediaControls checks if they are part of a document or not in order to
unregister
most of its event listeners when removed from a document to avoid leaking memory
until the document is closed.

The approach taken in this CL is the most "web based" because the controls are
using events. Another approach would be to use internal callbacks (from
ContainerNode) or even have a `addWeakEventListener` method in EventTarget. The
latter might be an interesting solution.

BUG=680898
R=zqzhang@chromium.org,ojan@chromium.org

Review-Url: https://codereview.chromium.org/2725873002
Cr-Commit-Position: refs/heads/master@{#454060}
Committed:
https://chromium.googlesource.com/chromium/src/+/8daac32c17f494d496fb2ca3768d...
==========

[commit-bot: I haz the power, 2017-03-01 22:10:51]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8daac32c17f494d496fb2ca3768d...