923173 - Trying to set audio.volume.master or audio.volume.bt_sco via mozSettings crashes device

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Bob Silverberg [:bsilverberg]]

Attachment: logcat when the crash occurs

On today's build [1], when running our Python test suite we found that the phone was crashing during our setup routine. 

We narrowed it down to the step where we attempt to silence the phone by setting the volume using mozSettings. We set the following to 0:

audio.volume...

'master', 'content', 'notification', 'alarm', 'telephony', 'bt_sco'

When setting either 'master' or 'bt_sco' the phone crashes.

A crash report can be found at [2], and the logcat from the run is attached.


[1] Gecko  http://hg.mozilla.org/mozilla-central/rev/e3c84e9f2490
Gaia   73a64d360f0ed135fcca205c6292e9219e085413
BuildID 20131002040206
Version 27.0a1

[2] https://crash-stats.mozilla.com/report/index/5a4ef136-b38b-455a-9651-440822131002

Comment 1

[Bob Silverberg [:bsilverberg]]

The Javascript error we're seeing in the logcat is:

E/GeckoConsole(  460): Content JS LOG at dummy file:499 in GaiaDataLayer.setSetting: setting audio.volume.master to 0
E/GeckoConsole(  500): [JavaScript Error: "NS_ERROR_INVALID_POINTER: Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIMessageSender.sendAsyncMessage]" {file: "chrome://global/content/BrowserElementChildPreload.js" line: 41}]
E/GeckoConsole(  500): [JavaScript Error: "NS_ERROR_INVALID_POINTER: Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIContentFrameMessageManager.content]" {file: "chrome://global/content/BrowserElementChildPreload.js" line: 718}]

I'm not sure if this is actually related to the attempt to set the audio channel setting, but removing that code removes the error and stops the phone from crashing.

James, can you cc in whomever would be the proper person to look into this?

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

We might be affected by bug 874508, and bug 923776; I've commented in the latter, asking if it could be causing/related to this bug, just FYI.

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

What's the need to set master volume to 0?
Also, bt_sco is written by BT earphone, overwrite the value using mozSettings may not be meaningful.

Comment 4

[Zac C (:zac)]

(In reply to Alive Kuo [:alive] from comment #3)
> What's the need to set master volume to 0?

The people in MV office where our phones are located were annoyed by the dialer and camera tests playing noises throughout the office :)

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Zac C (:zac) from comment #4)
> (In reply to Alive Kuo [:alive] from comment #3)
> > What's the need to set master volume to 0?
> 
> The people in MV office where our phones are located were annoyed by the
> dialer and camera tests playing noises throughout the office :)

Let's try to set audio.volume.notifications and audio.volume.publicnotification to 0?
I'm not sure why it crashes but maybe relevant to BT.

Comment 6

[Bob Silverberg [:bsilverberg]]

I believe the channels we use came from https://wiki.mozilla.org/WebAPI/AudioChannels#Volume_control, but I'm not sure where 'master' came from, tbh. We can certainly change the list of channels we are silencing to be a more appropriate list, but the fact remains that this worked a few days ago and now causes a crash, which seems to suggest that there is a problem somewhere that is worth investigating.

:Alive: Do you have a recommendation for a different set of channels that would ensure that the phones don't make any noise during device testing?

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Currently silent: alarm, notification, publicnotification, content should be O.K.
Master Volume is to control all the channels together by a factor.
Set BT_SCO is meaningless because it's for earphone; that means it doesn't make noise.

If this still crashes then we should find some audio guys to see what happens.

BTW I'm not sure about publicnotificaion channel is implemented or not now.

Comment 8

[Bob Silverberg [:bsilverberg]]

Attempting to set publicnotification also causes the phone to crash, but using the other three mentioned above seems to do the trick. This unblocks the gaia-ui-tests (once bug 924412 is fixed), but the error is probably still worth investigating.

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> Attempting to set publicnotification also causes the phone to crash, but
> using the other three mentioned above seems to do the trick. This unblocks
> the gaia-ui-tests (once bug 924412 is fixed), but the error is probably
> still worth investigating.

Yes, the phone shouldn't just crash when we change a settings...
^Marco?

Comment 10

[James Lal [:lightsofapollo]]

I think alive means Marco Chen... setting needinfo.

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

Given tomorrow's build, this /should/ be resolved by these two landings:

bug 923776
bug 923704

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Marco, given the following commit in a local tree, is it correct?

https://github.com/stephendonner/gaia/commit/2eaf0b2876c49241b6f84ac116a14d068c7469e0

Which channels should we be setting?

Comment 13

[Marco Chen [:mchen]]

(In reply to Stephen Donner [:stephend] from comment #12)
> Marco, given the following commit in a local tree, is it correct?
> 
> https://github.com/stephendonner/gaia/commit/
> 2eaf0b2876c49241b6f84ac116a14d068c7469e0
> 
> Which channels should we be setting?

Yes, these 4 channels are the ones we handled in AudioChannelService.cpp.
For crash issue by others name, I will check it. Thanks for reporting the issue.

Comment 15

[Marco Chen [:mchen]]

The crash point is 
http://hg.mozilla.org/mozilla-central/file/64b497e6f593/dom/audiochannel/AudioChannelService.cpp#l712

I think that web content can set any name of key through mozSettings web api then the code there cause web content easy to crash the B2G.

Comment 16

[Marco Chen [:mchen]]

Attachment: Patch v1: Change MOZ_ASSUME_UNREACHABLE to MOZ_WARNING

Hi,

Please refer to comment 15 to the reason for this patch. Thanks.

Comment 17

[Marco Chen [:mchen]]

Attachment: bug-923173-audio-volume-unreachPatch v1: Change MOZ_ASSUME_UNREACHABLE to MOZ_WARNING

Comment 18

[(no longer active)]

Comment on attachment 814783 [details] [diff] [review]
bug-923173-audio-volume-unreachPatch v1: Change MOZ_ASSUME_UNREACHABLE to MOZ_WARNING

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

This looks good.  Sorry for stepping on your toes, but this shows that the original MOZ_ASSERT there was semantically incorrect!  :-)

Comment 19

[Marco Chen [:mchen]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #18)
> Comment on attachment 814783 [details] [diff] [review]
> bug-923173-audio-volume-unreachPatch v1: Change MOZ_ASSUME_UNREACHABLE to
> MOZ_WARNING
> 
> Review of attachment 814783 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good.  Sorry for stepping on your toes, but this shows that the
> original MOZ_ASSERT there was semantically incorrect!  :-)

Yes, you are right. The original MOZ_ASSERT was still a wrong thing there.
Thanks for your help on it.

Comment 20

[Marco Chen [:mchen]]

Attachment: Patch : Checkin Version

https://tbpl.mozilla.org/?tree=Try&rev=58fb5e2bfb00

Comment 21

[Marco Chen [:mchen]]

Without this patch, any web content with mozsettings permission can be easy to crash the b2g by adding setting name.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4a5442007b43

Comment 23

[Jason Smith [:jsmith]]

(In reply to Marco Chen [:mchen] (Summit 10/03 ~10/08) from comment #21)
> Without this patch, any web content with mozsettings permission can be easy
> to crash the b2g by adding setting name.

Have we confirmed this bug would reproduce on 1.2? That will help figure out if we need this on 1.2.

Comment 24

[Zac C (:zac)]

This replicates on 1.2 aswell.
We were just seeing it on today's Inari/mozilla-aurora nightly build. I can check for Hamachi too but I think I know the answer already :)

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4a5442007b43

Comment 26

[Jason Smith [:jsmith]]

(In reply to Zac C (:zac) from comment #24)
> This replicates on 1.2 aswell.
> We were just seeing it on today's Inari/mozilla-aurora nightly build. I can
> check for Hamachi too but I think I know the answer already :)

Okay. Blocking+ then.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f72c378bc3e8