1263514 - crash in `anonymous namespace''::unregister_notification_client

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

This bug was filed from the Socorro interface and is 
report bp-462d46d0-949e-4172-af7c-8bb9f2160410.
=============================================================

Crash when playing back media. URLs are mostly Facebook, Skype and Youtube. With a smattering of other sites

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

jesup suggested you know this code, Paul would you have a look at this signature.

Comment 2

[Paul Adenot (:padenot)]

My plan was to take this one.

Comment 3

[Paul Adenot (:padenot)]

Attachment: MozReview Request: Bug 1263514 - Handle notification registering failure in cubeb_wasapi.cpp. r?kinetik

Review commit: https://reviewboard.mozilla.org/r/46699/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46699/

Comment 4

[Matthew Gregan [:kinetik]]

Can you please explain why we're crashing and why this fixes it?  I can't understand why the patch might help, but it's midnight and I'm falling asleep so maybe I'm being dense.

Comment 5

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87fc481e7f86

Comment 6

[Paul Adenot (:padenot)]

Attachment: Handle notification registering failure in cubeb_wasapi.cpp

Hrm I think I initially thought we were not addrefing/releasing properly, but I
see that the refcount starts at 1 in MS's COM, so that's not it, I removed the
AddRef/Release pair. Supposdely the IMMDeviceNotification does not
Addref/Release the object, but then I read [0] that implies that it does:

> The root cause of the problem is that the
> IMMDeviceEnumerator::RegisterEndpointNotificationCallback takes a reference to
> the IMMNotificationClient object passed in.  This shouldn’t be a surprise,
> because the Counted Pointer design pattern requires that every time you save a
> pointer to an object, you take a reference to that object (and every interface
> that derives from IUnknown implements the Counted Pointer design pattern).
> Since the RegisterEndpointNotificationCallback saves it’s input pointer for
> later consumption (when it generates the notification), it has to take a
> reference to the object.

Maybe we should still AddRef/Release to make sure, it's really unclear.
Locally, it does not call our AddRef, but disassembling the code, and looking at
function names, it looks like it's registering something internaly.

This can happen with two stacks: either when an AudioStream shuts down normally
because it's probably ended or something, or during a seek.

Anyway, we're clearly calling `Release()` on a invalid pointer. My only
thought was that when we fail to unregister the notification client, the pointer
becomes bad or maybe that it's released by something. During the life time of a
cubeb_stream, we create it once, and release it at the end, so it should not be
affected by device changes. I'm thinking maybe a double call on
cubeb_stream_destroy, since this is the first thing we do in the function ?

This patch tries to release things early and avoid the crashing path, as a
mitigation mechanism, but I'm still looking at the code.

[0]:
https://blogs.msdn.microsoft.com/larryosterman/2007/10/24/memory-leak-when-using-the-vista-audio-api-notification-routines/

Comment 7

[Paul Adenot (:padenot)]

(clearing NI)

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8741706 [details]
MozReview Request: Bug 1263514 - Handle notification registering failure in cubeb_wasapi.cpp.  r?kinetik

https://reviewboard.mozilla.org/r/46699/#review43991

Comment 9

[Matthew Gregan [:kinetik]]

(In reply to Paul Adenot (:padenot) from comment #6)
> Created attachment 8742339 [details] [diff] [review]
> Handle notification registering failure in cubeb_wasapi.cpp
> 
> Hrm I think I initially thought we were not addrefing/releasing properly,
> but I
> see that the refcount starts at 1 in MS's COM, so that's not it, I removed
> the
> AddRef/Release pair. Supposdely the IMMDeviceNotification does not

Right, that's why I was confused about the first patch.  r+ the second since that looks like a good change even if it turns out not to be the final fix.

Looking at this code did make me wonder if we should exclusively manage this object via refcounts rather than assuming our reference is the last and destroying it manually.  I don't know if there's a situation where it matters (and I don't think it's the root cause of this particular crash).

Comment 10

[Paul Adenot (:padenot)]

Pushed as part of https://hg.mozilla.org/integration/mozilla-inbound/rev/2f3a6b7cd4b3.

Comment 11

[Paul Adenot (:padenot)]

We'll have to look at the crash rates now, let's open a new bug if this continues.