Bug 170771 - Deadlock in CoreAudioCaptureSource
Summary: Deadlock in CoreAudioCaptureSource
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-12 07:59 PDT by Jeremy Jones
Modified: 2017-04-18 16:14 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.02 KB, patch)
2017-04-12 10:33 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (3.05 KB, patch)
2017-04-13 12:58 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (4.54 KB, patch)
2017-04-17 12:09 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (4.19 KB, patch)
2017-04-17 13:26 PDT, Jeremy Jones
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing. (4.17 KB, patch)
2017-04-17 15:02 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (898.04 KB, application/zip)
2017-04-17 16:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (11.74 MB, application/zip)
2017-04-17 17:32 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2017-04-12 07:59:07 PDT
Deadlock in CoreAudioCaptureSource
Comment 1 Jeremy Jones 2017-04-12 07:59:57 PDT
rdar://problem/31578919
Comment 2 Jeremy Jones 2017-04-12 10:33:11 PDT
Created attachment 306920 [details]
Patch
Comment 3 youenn fablet 2017-04-12 10:41:58 PDT
Comment on attachment 306920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306920&action=review

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:418
> +            err = setupAudioUnits();

If err is not null, I would guess m_ioUnit to no longer be null.
If err is null, is m_ioUnit null? Should we clean it?
How about setupAudioUnits be returning a m_ioUnit instead of an err?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:423
> +        err = AudioOutputUnitStart(m_ioUnit);

If err, do we need to do some clean-up related to m_ioUnit?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:-428
> -    m_ioUnitStarted = true;

Should we do ASSERT(m_ioUnit) here?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:438
> +    if (m_ioUnit) {

If there is no m_ioUnit, do we still want to call setMuted(true)?
If not, we could return early if !m_ioUnit.
Comment 4 Jeremy Jones 2017-04-13 12:10:53 PDT
Comment on attachment 306920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306920&action=review

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:418
>> +            err = setupAudioUnits();
> 
> If err is not null, I would guess m_ioUnit to no longer be null.
> If err is null, is m_ioUnit null? Should we clean it?
> How about setupAudioUnits be returning a m_ioUnit instead of an err?

Sounds good, but that is outside the scope of this change.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:423
>> +        err = AudioOutputUnitStart(m_ioUnit);
> 
> If err, do we need to do some clean-up related to m_ioUnit?

Maybe. The unit will be cleaned-up in the destructor. In the mean-time no audio will be produced.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:-428
>> -    m_ioUnitStarted = true;
> 
> Should we do ASSERT(m_ioUnit) here?

I can add it even earlier, right after setupAudioUnits succeeds.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:438
>> +    if (m_ioUnit) {
> 
> If there is no m_ioUnit, do we still want to call setMuted(true)?
> If not, we could return early if !m_ioUnit.

Yes, we still want to call setMuted even if there is no audio unit so that the state is still correct when the unit is later created.
Comment 5 Jeremy Jones 2017-04-13 12:58:31 PDT
Created attachment 307015 [details]
Patch
Comment 6 youenn fablet 2017-04-13 13:13:30 PDT
(In reply to Jeremy Jones from comment #4)
> Comment on attachment 306920 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306920&action=review
> 
> >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:418
> >> +            err = setupAudioUnits();
> > 
> > If err is not null, I would guess m_ioUnit to no longer be null.
> > If err is null, is m_ioUnit null? Should we clean it?

It is probably error prone to have an error but have a non null m_ioUnit.
Can we clean it here or ensure it is null if there is an error?

> > How about setupAudioUnits be returning a m_ioUnit instead of an err?
> 
> Sounds good, but that is outside the scope of this change.

OK

> >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:423
> >> +        err = AudioOutputUnitStart(m_ioUnit);
> > 
> > If err, do we need to do some clean-up related to m_ioUnit?
> 
> Maybe. The unit will be cleaned-up in the destructor. In the mean-time no
> audio will be produced.

It seems error-prone to leave the class in an unstable state (muted, m_ioUnit not null, error occurred). Can we do the clean-up in this patch if that is not too difficult?

> > If there is no m_ioUnit, do we still want to call setMuted(true)?
> > If not, we could return early if !m_ioUnit.
> 
> Yes, we still want to call setMuted even if there is no audio unit so that
> the state is still correct when the unit is later created.

If there is no m_ioUnit, muted should already be false and setMuted will be a no-op, right?
Comment 7 Jeremy Jones 2017-04-17 12:08:56 PDT
(In reply to youenn fablet from comment #6)
> (In reply to Jeremy Jones from comment #4)
> > Comment on attachment 306920 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=306920&action=review
> > 
> > >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:418
> > >> +            err = setupAudioUnits();
> > > 
> > > If err is not null, I would guess m_ioUnit to no longer be null.
> > > If err is null, is m_ioUnit null? Should we clean it?
> 
> It is probably error prone to have an error but have a non null m_ioUnit.
> Can we clean it here or ensure it is null if there is an error?

I now clean up everything in case of an error here.

> 
> > > How about setupAudioUnits be returning a m_ioUnit instead of an err?
> > 
> > Sounds good, but that is outside the scope of this change.
> 
> OK
> 
> > >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:423
> > >> +        err = AudioOutputUnitStart(m_ioUnit);
> > > 
> > > If err, do we need to do some clean-up related to m_ioUnit?
> > 
> > Maybe. The unit will be cleaned-up in the destructor. In the mean-time no
> > audio will be produced.
> 
> It seems error-prone to leave the class in an unstable state (muted,
> m_ioUnit not null, error occurred). Can we do the clean-up in this patch if
> that is not too difficult?

Added cleanupAudioUnits() after setupAudioUnits() fails.

> 
> > > If there is no m_ioUnit, do we still want to call setMuted(true)?
> > > If not, we could return early if !m_ioUnit.
> > 
> > Yes, we still want to call setMuted even if there is no audio unit so that
> > the state is still correct when the unit is later created.
> 
> If there is no m_ioUnit, muted should already be false and setMuted will be
> a no-op, right?

Setting m_muted directly in the constructor instead of calling setMuted().
Comment 8 Jeremy Jones 2017-04-17 12:09:14 PDT
Created attachment 307287 [details]
Patch
Comment 9 Jeremy Jones 2017-04-17 13:26:53 PDT
Created attachment 307291 [details]
Patch
Comment 10 Jeremy Jones 2017-04-17 15:02:13 PDT
Created attachment 307303 [details]
Patch for landing.
Comment 11 Build Bot 2017-04-17 16:20:09 PDT
Comment on attachment 307303 [details]
Patch for landing.

Attachment 307303 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3553540

New failing tests:
webrtc/multi-video.html
Comment 12 Build Bot 2017-04-17 16:20:10 PDT
Created attachment 307315 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 13 Build Bot 2017-04-17 17:32:28 PDT
Comment on attachment 307303 [details]
Patch for landing.

Attachment 307303 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3553809

New failing tests:
webrtc/no-port-zero-in-upd-candidates.html
Comment 14 Build Bot 2017-04-17 17:32:30 PDT
Created attachment 307324 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 15 WebKit Commit Bot 2017-04-18 16:14:35 PDT
Comment on attachment 307303 [details]
Patch for landing.

Clearing flags on attachment: 307303

Committed r215485: <http://trac.webkit.org/changeset/215485>