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

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

My plan was to take this one.

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

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.

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/

(clearing NI)

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

(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).

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

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