1167465 - Exposing Allowed Audio Channels in System App's Window

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Evan Tseng [:evanxd]]

Attachment: debug-bug-1167465.patch

Comment 2

[Evan Tseng [:evanxd]]

STR:
1. Install the below patches:
* For Gaia: https://github.com/evanxd/gaia/commit/9b56248655edcbf1e25fcf6a463b42270c4fc555
* For Gecko: The  debug-bug-1167465.patch attachment(https://bugzilla.mozilla.org/attachment.cgi?id=8609114)
2. Reboot FxOS.
3. Run `adb logcat | grep 1167465` to see the log.

Expected log:
I/GeckoConsole( 2043): Content JS LOG: Bug 1167465 - Send MozContentEvent to get audio channels 
I/GeckoDump( 2043): Bug 1167465 - Send audio channel list to System app
I/GeckoConsole( 2043): Content JS LOG: Bug 1167465 - Receive audioChannels: normal,notification,telephony

Actual log:
I/GeckoConsole( 2557): Content JS LOG: Bug 1167465 - Send MozContentEvent to get audio channels 
I/GeckoDump( 2557): Bug 1167465 - Send audio channel list to System app

The environment:
Build ID               20150521160241
Gaia Revision          1126d8bee559f7cde675df2fcc6c196da9cfeba1
Gaia Date              2015-05-21 21:23:56
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/3e737d30f842
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20141204.004231
Firmware Date          Thu Dec  4 00:42:42 EST 2014
Bootloader             L1TC00011880

Comment 3

[Evan Tseng [:evanxd]]

Hi Kan-Ru,

Could you take a look?

Thanks.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

I haven't looked into this yet, but could you try to send the event after document "load"?

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Can't reproduce. The output is correct

1087:I/GeckoConsole( 2316): Content JS LOG: Bug 1167465 - Send MozContentEvent to get audio channels 
1090:I/GeckoDump( 2316): Bug 1167465 - Send audio channel list to System app
1091:I/GeckoConsole( 2316): Content JS LOG: Bug 1167465 - Receive audioChannels: normal,notification,telephony 

Build ID               20150610181819
Gaia Revision          27e4c1af0be302150a5f6e428666e0e3daf2a4ea
Gaia Date              2015-06-10 09:46:07
Gecko Revision         n/a
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150427.192938
Firmware Date          Mon Apr 27 19:29:49 EDT 2015
Bootloader             L1TC00011880

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

gecko rev 516ac5b38c9ae90a5323b528ed9a09a4c87b1598

Comment 7

[Evan Tseng [:evanxd]]

Maybe the issues is about the implementation of Audio Channel Management APIs. So we need a specific gecko rev to reproduce.

The below are the steps to install the gecko in the device.
How to enable the AudioChannelManager.
1. To install the gecko module to enable the APIs:
* Unzip the b2g.tar.gz attachment[1].
* Run `adb shell mount -o remount,rw /system` to have capability to write file system if you don't have it.
* Run `adb push path/to/the/attachment/directory/b2g /system/b2g/`
2. Restart the device.
3. Done.

[1]: https://drive.google.com/open?id=0B3MxK4_nj3bpcnZwUFdNWlM3Nkk&authuser=1

Comment 8

[Evan Tseng [:evanxd]]

And I'll try to reproduce the bug again. If I can do it successfully, I'll do needinfo and let you know. Thanks for the help.

Comment 9

[Evan Tseng [:evanxd]]

New STR:

1. Add the gaia patch[1]
2. Reboot FxOS.
3. Run `adb logcat | grep 1167465` to see the log.

Expected result:
See the log: `I/GeckoConsole( 2191): Content JS LOG: Bug 1167465: get audio channels: normal,content,notification,ringer`

Actual result:
Cannot see the log: `I/GeckoConsole( 2191): Content JS LOG: Bug 1167465: get audio channels: normal,content,notification,ringer`

If we wait for a while and send the content event with the patch[2], then we can see the excepted log.

[1]: https://github.com/evanxd/gaia/commit/e0387eab1227da58e2b88731dbf973937ba3ff11
[2]: https://github.com/evanxd/gaia/commit/74d42719472533ec491df4281613c4111ab2e249

Comment 10

[Evan Tseng [:evanxd]]

The Comment 9 is based on the environment[1].

[1]:
Build ID               20150803150205
Gaia Revision          dbacf8364f4505d021b7d8fb9cabea325004dbcc
Gaia Date              2015-08-03 16:38:49
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/abc56d57f6e1
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150622.193834
Firmware Date          Mon Jun 22 19:38:45 EDT 2015
Bootloader             L1TC00011880

Comment 11

[Evan Tseng [:evanxd]]

Hi Kan-Ru,

We have a new STR in comment 9, and the reproduce-able environment is listed in comment 10. And this bug blocks bug 1180618.

Could you take a look? Thanks.

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

https://github.com/mozilla-b2g/gaia/blob/4df9592/apps/system/js/core.js#L59-L64

"Note that shell.js starts listen for the mozContentEvent event at
 mozbrowserloadstart, which sometimes does not happen till window.onload."

So this had happened before. I think the problem is mozbrowserloadstart is a async message so the mozContentEvent fired before shell.js received loadstart and attached event listeners was lost. We need a better way for shell.js to listen to the mozContentEvent events.

Comment 13

[Evan Tseng [:evanxd]]

Thanks for Kan-Ru's investigate.

baku, would you like to take a look?
This bug is blocking a gaia fix[1] for audio channel management.

Thanks.

[1]: http://bugzil.la/1180618

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

It's hard to avoid this races between shell.js and system app. shell.js can't notify system app because it doesn't know if system app is ready and vice versa. Hopefully a new API would solve this problem.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

My advices is, don't send mozContentEvent until you have received your first mozChromeEvent. Currently this is after the "load" event.

Comment 16

[Evan Tseng [:evanxd]]

Kan-Ru, thanks for the advice. I'll try that, hope it'll work.

Comment 17

[Andrea Marchesini [:baku]]

> baku, would you like to take a look?
> This bug is blocking a gaia fix[1] for audio channel management.

Can this be fixed by exposing allowedAudioChannels array to the system app directly?
If yes, I don't think this will take too much work.

Comment 18

[Evan Tseng [:evanxd]]

Do you mean that we can do something like
```
console.log(window.allowedAudioChannels[0].name);
```
 to print a audio channel name in System App's window?

If so, we can fix the gaia bug[1].

[1]: http://bugzil.la/1180618

Comment 19

[Andrea Marchesini [:baku]]

Yes, I was proposing something like this. If yes, and alwu agrees, I/we can go implementing it.

Comment 20

[Evan Tseng [:evanxd]]

Nice!

Just one more question, exposing allowedAudioChannels is only for System app, not for other apps, right?

It means that apps except System app still cannot access their allowedAudioChannels directly.

Comment 21

[Alastor Wu [:alwu]]

I have the same question as Evan's.
Can we expose the allowedAudioChannels only for system app? 
I agree if we can make sure this point.

Comment 22

[Andrea Marchesini [:baku]]

> Just one more question, exposing allowedAudioChannels is only for System
> app, not for other apps, right?

correct. How urgent is this?
We can have a custom permission just for this.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: foo.patch

This patch is not tested. But maybe you have time to give me a feedback.

Comment 24

[Andrea Marchesini [:baku]]

I don't see any tests for AudioChannelManager. Alastor, do you mind to review/test this patch next week in Taipei?

Comment 25

[Alastor Wu [:alwu]]

Sure!

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 8654940 [details] [diff] [review]
foo.patch

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

::: dom/apps/PermissionsTable.jsm
@@ +497,5 @@
>                                trusted: DENY_ACTION,
>                                privileged: ALLOW_ACTION,
>                                certified: ALLOW_ACTION
>                             },
> +                           "allowed-audio-channels-in-window": {

I want to rename this to allow-audio-channels-in-app

Comment 27

[Alastor Wu [:alwu]]

After offline discuss with Baku, we still need to modify the permission, 

If we use "certified: DENY_ACTION", the system app can not use the allowedaudiochannel.
If we use "certified: ALLOW_ACTION", all certified apps can use this attribute, that is not we want.

Comment 28

[[:fabrice] Fabrice Desré]

Comment on attachment 8654940 [details] [diff] [review]
foo.patch

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

Holding on review based on https://bugzilla.mozilla.org/show_bug.cgi?id=1167465#c27

Comment 29

[Andrea Marchesini [:baku]]

I propose to rename the permission system-app-only-audio-channel-in-app.

If you agree, can you take a look at the patch again?

Comment 30

[[:fabrice] Fabrice Desré]

Comment on attachment 8654940 [details] [diff] [review]
foo.patch

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

r=me with the permission name change

Comment 31

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a729df0cb8b4

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a729df0cb8b4