[Media, Controls] Use a weak ref to the listener from callback.

[Media, Controls] Use a weak ref to the listener from callback.

Seems like holding a strong reference causes the leakage.

BUG=717094
TEST=run the failing tests locally with --enable-leak-detection
TBR=haraken
NOTREECHECKS=true
NOTRY=true

Review-Url: https://codereview.chromium.org/2848363002
Cr-Commit-Position: refs/heads/master@{#468525}
Committed: https://chromium.googlesource.com/chromium/src/+/27e76cce52a397ea93c9bcf38454c2b4013b7db9

[whywhat, 2017-05-01 21:01:34]

avayvod@chromium.org changed reviewers:
+ haraken@chromium.org

[whywhat, 2017-05-01 21:01:34]

PTaL

[whywhat, 2017-05-01 21:01:38]

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

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

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-02 01:14:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 01:14:29]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-05-02 01:18:34]

Description was changed from

==========
[Media, Controls] Use a weak ref to the listener from callback.

Seems like holding a strong reference causes the leakage.

BUG=717094
TEST=run the failing tests locally with --enable-leak-detection
==========

to

==========
[Media, Controls] Use a weak ref to the listener from callback.

Seems like holding a strong reference causes the leakage.

BUG=717094
TEST=run the failing tests locally with --enable-leak-detection
TBR=haraken
NOTREECHECKS=true
NOTRY=true
==========

[whywhat, 2017-05-02 01:18:37]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2017-05-02 01:19:03]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-02 01:23:36]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493687917937060, "parent_rev":
"e05a6fdeff3d39f64b4af1d1468251602e9ccc6b", "commit_rev":
"27e76cce52a397ea93c9bcf38454c2b4013b7db9"}

[commit-bot: I haz the power, 2017-05-02 01:23:46]

Description was changed from

==========
[Media, Controls] Use a weak ref to the listener from callback.

Seems like holding a strong reference causes the leakage.

BUG=717094
TEST=run the failing tests locally with --enable-leak-detection
TBR=haraken
NOTREECHECKS=true
NOTRY=true
==========

to

==========
[Media, Controls] Use a weak ref to the listener from callback.

Seems like holding a strong reference causes the leakage.

BUG=717094
TEST=run the failing tests locally with --enable-leak-detection
TBR=haraken
NOTREECHECKS=true
NOTRY=true

Review-Url: https://codereview.chromium.org/2848363002
Cr-Commit-Position: refs/heads/master@{#468525}
Committed:
https://chromium.googlesource.com/chromium/src/+/27e76cce52a397ea93c9bcf38454...
==========

[commit-bot: I haz the power, 2017-05-02 01:23:47]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/27e76cce52a397ea93c9bcf38454...

[haraken, 2017-05-02 01:27:05]

Is it not guaranteed that OnRemotePlaybackAvailabilityChanged gets called
unconditionally?

[haraken, 2017-05-03 01:02:17]

On 2017/05/02 01:27:05, haraken wrote:
> Is it not guaranteed that OnRemotePlaybackAvailabilityChanged gets called
> unconditionally?

ping? I don't fully understand why this reference needs to be weak.

[whywhat, 2017-05-03 02:04:22]

On 2017/05/03 at 01:02:17, haraken wrote:
> On 2017/05/02 01:27:05, haraken wrote:
> > Is it not guaranteed that OnRemotePlaybackAvailabilityChanged gets called
> > unconditionally?
> 
> ping? I don't fully understand why this reference needs to be weak.

TL;DR I think having a strong reference creates a link in the reference loop
that's not understandable by the Blink GC.

OnRemotePlaybackAvailabilityChanged will be called but that doesn't remove the
callback as the availability changes over time and the callback is not a
one-shot.

The callback is only removed when HTMLMediaElement is removed from the DOM so
that MediaControlsMediaEventListener::Detach is called.
Until that the listener will always be referenced by the callback, the listener
references the controls that references HTMLMediaElement, that references the
Document.

Maybe there's some other event when we can remove the callback to avoid leaking
the document but I don't know about it.

[haraken, 2017-05-03 09:16:35]

On 2017/05/03 02:04:22, whywhat wrote:
> On 2017/05/03 at 01:02:17, haraken wrote:
> > On 2017/05/02 01:27:05, haraken wrote:
> > > Is it not guaranteed that OnRemotePlaybackAvailabilityChanged gets called
> > > unconditionally?
> > 
> > ping? I don't fully understand why this reference needs to be weak.
> 
> TL;DR I think having a strong reference creates a link in the reference loop
> that's not understandable by the Blink GC.
> 
> OnRemotePlaybackAvailabilityChanged will be called but that doesn't remove the
> callback as the availability changes over time and the callback is not a
> one-shot.
> 
> The callback is only removed when HTMLMediaElement is removed from the DOM so
> that MediaControlsMediaEventListener::Detach is called.
> Until that the listener will always be referenced by the callback, the
listener
> references the controls that references HTMLMediaElement, that references the
> Document.
> 
> Maybe there's some other event when we can remove the callback to avoid
leaking
> the document but I don't know about it.

Thanks, that makes sense. We could probably add a comment about why it needs to
be weak.

[whywhat, 2017-05-03 14:54:15]

On 2017/05/03 at 09:16:35, haraken wrote:
> On 2017/05/03 02:04:22, whywhat wrote:
> > On 2017/05/03 at 01:02:17, haraken wrote:
> > > On 2017/05/02 01:27:05, haraken wrote:
> > > > Is it not guaranteed that OnRemotePlaybackAvailabilityChanged gets
called
> > > > unconditionally?
> > > 
> > > ping? I don't fully understand why this reference needs to be weak.
> > 
> > TL;DR I think having a strong reference creates a link in the reference loop
> > that's not understandable by the Blink GC.
> > 
> > OnRemotePlaybackAvailabilityChanged will be called but that doesn't remove
the
> > callback as the availability changes over time and the callback is not a
> > one-shot.
> > 
> > The callback is only removed when HTMLMediaElement is removed from the DOM
so
> > that MediaControlsMediaEventListener::Detach is called.
> > Until that the listener will always be referenced by the callback, the
listener
> > references the controls that references HTMLMediaElement, that references
the
> > Document.
> > 
> > Maybe there's some other event when we can remove the callback to avoid
leaking
> > the document but I don't know about it.
> 
> Thanks, that makes sense. We could probably add a comment about why it needs
to be weak.

Thanks, will do in a follow up change.