B2G hangs on the emulator x86 if microphone is muted during answered call

RESOLVED FIXED in FxOS-S5 (21Aug)

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: alexander.kazakevich.tv, Assigned: tzimmermann)

Tracking

unspecified
FxOS-S5 (21Aug)
x86
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

(Whiteboard: [red-tai])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150222232811

Steps to reproduce:

1. Call to the emulator x86 via telnet (gsm call 111333) or ddms.
2. Answer to the incoming call on the emulator x86.
3. Mute microphone.

1. Call to the emulator x86 via telnet (gsm call 111333) or ddms.
2. Answer to the incoming call on the emulator x86.
3. End call by telnet, ddms or the HangUp key.


Actual results:

UI hangs


Expected results:

UI should not hangs.
(Reporter)

Updated

4 years ago
Hardware: ARM → x86

Updated

4 years ago
Whiteboard: [red-tai]
B2G hangs after answered call is finished.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Root cause: Any operation with microphone is blocked beacause the write call is called periodically and mutex for audio_hw object is held constantly.
Proposed solution: Add mutex for the "microphone is muted" flag and use it to set or get this flag here:
B2G/device/generic/goldfish/audio/audio_hw.c
Hi Bobby,
Can you find someone from audio team to look at this bug?
blocking-b2g: --- → 2.2?
Flags: needinfo?(bchien)

Comment 4

4 years ago
Alaster, could you help to handle this? thanks.
Flags: needinfo?(bchien) → needinfo?(alwu)
Sure, I'll check it later.
Keep NI for tracking.
Hi, I think that it's not audio channel issue.
Maybe find someone familiar with RIL or call screen app to check it.
Component: AudioChannel → Emulator
Flags: needinfo?(alwu)
Created attachment 8628670 [details] [diff] [review]
B2G_device_generic_goldfish.patch
Dear Alastor,

We have investigated this issue and found that B2G is blocked in the "static int adev_get_mic_mute(const struct audio_hw_device *dev, bool *state)" function in file B2G/device/generic/goldfish/audio/audio_hw.c. (We added debug messages).
Muting and unmuting is handled by AudioManager on the emulator (not by RIL).
The same problem is observed when we try to start outgoing call from Dialer App, so it is not the "callscreen" issue.
I think the pthread_mutex_lock does not put waiting threads to a queue, so thread which calls the "write" method unlock and lock mutex in the short period and another thread which waits the same mutex is locked for long period.
The attached patch for "B2G/device/generic/goldfish" seems to have fixed the issue. Could you please check it and if you don’t have any objections, apply it to branch v2.2?

Thank you for attention,
Alexander.
Sorry, Alex,
Because I don't familiar with these code, I can't review this patch.
You may need other Gonk team guys to review it.
However, for my opinion, I think the double locking is not a good idea.
Maybe we can check the |adev->mic_mute| first, if it's true, we can ignore the |read(...)| and release the lock.

--

Hi, Michael,
Sorry to bother you,
Do you know who can review this patch?
Thanks!
Flags: needinfo?(mwu)
[Blocking Requested - why for this release]:
Continue fixing on next release.

Hi Michael,
Could you help to review the patch per comment 7? Thanks!
blocking-b2g: 2.2? → 2.5?
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
Comment on attachment 8628670 [details] [diff] [review]
B2G_device_generic_goldfish.patch

Forwarding to the emulator owner.
Flags: needinfo?(mwu)
Attachment #8628670 - Flags: review?(tzimmermann)
Comment on attachment 8628670 [details] [diff] [review]
B2G_device_generic_goldfish.patch

Review of attachment 8628670 [details] [diff] [review]:
-----------------------------------------------------------------

First of all, thanks for the patch!

::: audio/audio_hw.c
@@ +342,3 @@
>      pthread_mutex_lock(&adev->lock);
>      if (adev->fd >= 0)
>          bytes = read(adev->fd, buffer, bytes);

As far as I understand this bug, the problem is that this call to |read| blocks while we are still holding |adev->lock|, right? And there is also an implicit bug in |out_write| where |write| blocks while we're holding this lock.

Instead of introducing another lock, can we just open |adev->fd| with O_NONBLOCK near line 640? If |read| or |write| tells us that (errno == EWOULDBLOCK), we'd return 0 bytes.

Is the caller of this function able to handle this case?
I have checked proposal solution with the O_NONBLOCK flag. It does not resolve issue.
Comment on attachment 8628670 [details] [diff] [review]
B2G_device_generic_goldfish.patch

Review of attachment 8628670 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the feedback, Alexander.

Let's land the current patch. If the second lock turns out to be a problem, we can still fix it.
Attachment #8628670 - Flags: review?(tzimmermann) → review+
Hi Alex,

if you're actively working an a bug, please assign it to yourself.

If you have a patch with an r+, you're also responsible to landing to it. For Gecko patches, you can do that by setting [checkin-needed] in the Keywords field. For patches against Github repositories, like in this case, you need to create a pull request on Github, get your r+, and set [checkin-needed].

With [checkin-needed], people with commit rights see that the bug report's change is ready for landing and will commit the patch set. Once you fixed a few bugs and people know you, you can get commit rights and land patches yourself.
Assignee: nobody → alexander.kazakevich.tv
Status: NEW → ASSIGNED

Comment 16

3 years ago
I'm sorry, but Alex is no longer available. Mozilla guys, could you please take care of delivering this patch (preferably to v2.2r)? Thank you!
Status: ASSIGNED → NEW

Updated

3 years ago
Assignee: alexander.kazakevich.tv → nobody
I see. I'll take of landing the patch.
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Alexey,

Which emulator are you using? The bug report says emulator x86, but this doesn't have audio.c. Are you on emulator-x86-kk?
Flags: needinfo?(Alexey.Kistanov)

Comment 19

3 years ago
Thomas, yes, it's emulator-x86-kk.
Flags: needinfo?(Alexey.Kistanov)
Created attachment 8650914 [details] [review]
Github pull request

Thanks Alexey. Here's the pull request with the patch. Landing this now.
Attachment #8628670 - Attachment is obsolete: true
Attachment #8650914 - Flags: review+
Keywords: checkin-needed
b2g-4.4.2_r1: https://github.com/mozilla-b2g/device_generic_goldfish/commit/8d4018ebd33ac3f1a043b2d54bc578028656a659
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-v2.2: affected → fixed
status-b2g-v2.2r: --- → fixed
status-b2g-master: affected → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S5 (21Aug)
blocking-b2g: 2.5? → ---
You need to log in before you can comment on or make changes to this bug.