If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

crash in `anonymous namespace''::unregister_notification_client

RESOLVED FIXED

Status

()

Core
Audio/Video: cubeb
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kbrosnan, Assigned: padenot)

Tracking

({crash})

Trunk
All
Windows
crash
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox46 affected, firefox47 affected, firefox48 ?)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
jesup suggested you know this code, Paul would you have a look at this signature.
Flags: needinfo?(padenot)
(Assignee)

Comment 2

2 years ago
My plan was to take this one.
Assignee: nobody → padenot
Flags: needinfo?(padenot)
(Assignee)

Comment 3

2 years ago
Created attachment 8741706 [details]
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/
Attachment #8741706 - Flags: review?(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.
Flags: needinfo?(padenot)
(Assignee)

Comment 5

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=87fc481e7f86
(Assignee)

Comment 6

2 years ago
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
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/
Attachment #8742339 - Flags: review?(kinetik)
(Assignee)

Comment 7

2 years ago
(clearing NI)
Flags: needinfo?(padenot)
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
Attachment #8741706 - Flags: review?(kinetik)
Attachment #8742339 - Flags: review?(kinetik) → review+
(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).
(Assignee)

Updated

2 years ago
Depends on: 1265692
(Assignee)

Comment 10

2 years ago
Pushed as part of https://hg.mozilla.org/integration/mozilla-inbound/rev/2f3a6b7cd4b3.
(Assignee)

Comment 11

2 years ago
We'll have to look at the crash rates now, let's open a new bug if this continues.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.