1458020 - Make WebRTC work in GeckoView w/ E10s

Status: RESOLVED FIXED

(GeckoView Graveyard :: Sandboxing, enhancement, P1)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Right now a simple gUM test causes a crash in the child.

https://webrtc.github.io/samples/src/content/getusermedia/gum/

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Carrying over NI from https://bugzilla.mozilla.org/show_bug.cgi?id=1316241#c3

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: webrtc-e10s.diff

This patch makes it so we don't crash, but then we have the issue with using nsIPermissionManager in the child.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1458020 - 1a. Add waitForJS and waitForChromeJS;

Add waitForJS and waitForChromeJS methods that are similar to evaluateJS
and evaluateChromeJS. However, the wait methods count as wait events,
and will affect the behavior of forCallbacksDuringWait and
delegateDuringNextWait. The wait methods also wait for Promises to
resolve before returning their values.

Review commit: https://reviewboard.mozilla.org/r/243844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243844/

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1458020 - 1b. Add forceGarbageCollection;

Add a test for the permission delegate, including tests for Android and
content permission prompts.

Review commit: https://reviewboard.mozilla.org/r/243846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243846/

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1458020 - 2. Add PermissionDelegateTest;

We currently set the Android JavaVM pointer in MediaEngineWebRTC.
However, because of that, we end up setting the pointer in the child
process, even though we really want to set the pointer in the parent
process because that's where the camera will be accessed.

This patch makes us set JavaVM inside VideoEngine itself, where we
actually access the camera in the parent process.

Review commit: https://reviewboard.mozilla.org/r/243848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243848/

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1458020 - 3. Set JavaVM when actually using video capture;

We had a permission error because we were trying to add camera
permission in the child process. Move it to the parent process, where
the permission is actually checked.

Review commit: https://reviewboard.mozilla.org/r/243850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243850/

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1458020 - 4. Add camera permission in the parent process;

We apparently fail to get these properties when running in an emulator,
so we should just return default values.

Review commit: https://reviewboard.mozilla.org/r/243852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243852/

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8975598 [details]
Bug 1458020 - 2. Add PermissionDelegateTest;

https://reviewboard.mozilla.org/r/243848/#review249760

lgtm.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8975597 [details]
Bug 1458020 - 1b. Add forceGarbageCollection;

https://reviewboard.mozilla.org/r/243846/#review249788

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8975597 [details]
Bug 1458020 - 1b. Add forceGarbageCollection;

https://reviewboard.mozilla.org/r/243846/#review249790

I feel like it might be easier to understand a test page that does all of the stuff you're doing in the evaluateJs calls, but I guess there is no functional difference.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8975599 [details]
Bug 1458020 - 3. Set JavaVM when actually using video capture;

https://reviewboard.mozilla.org/r/243850/#review249792

Comment 12

[Eugen Sawin [:esawin]]

Comment on attachment 8975600 [details]
Bug 1458020 - 4. Add camera permission in the parent process;

https://reviewboard.mozilla.org/r/243852/#review250010

r+ with nits.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1855
(Diff revision 1)
>      }
>  
>      @WrapForJNI(calledFrom = "any")
>      public static int getAudioOutputFramesPerBuffer() {
>          if (SysInfo.getVersion() < 17) {
> -            return 0;
> +            return 512;

Make that a constant and log a warning.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1860
(Diff revision 1)
> -            return 0;
> +            return 512;
>          }
>          final AudioManager am = (AudioManager)getApplicationContext()
>                                  .getSystemService(Context.AUDIO_SERVICE);
>          if (am == null) {
> -            return 0;
> +            return 512;

Same.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1864
(Diff revision 1)
>          if (am == null) {
> -            return 0;
> +            return 512;
>          }
>          final String prop = am.getProperty(AudioManager.PROPERTY_OUTPUT_FRAMES_PER_BUFFER);
>          if (prop == null) {
> -            return 0;
> +            return 512;

Same.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1872
(Diff revision 1)
>      }
>  
>      @WrapForJNI(calledFrom = "any")
>      public static int getAudioOutputSampleRate() {
>          if (SysInfo.getVersion() < 17) {
> -            return 0;
> +            return 44100;

Same.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1877
(Diff revision 1)
> -            return 0;
> +            return 44100;
>          }
>          final AudioManager am = (AudioManager)getApplicationContext()
>                                  .getSystemService(Context.AUDIO_SERVICE);
>          if (am == null) {
> -            return 0;
> +            return 44100;

Same.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1881
(Diff revision 1)
>          if (am == null) {
> -            return 0;
> +            return 44100;
>          }
>          final String prop = am.getProperty(AudioManager.PROPERTY_OUTPUT_SAMPLE_RATE);
>          if (prop == null) {
> -            return 0;
> +            return 44100;

Same.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1458020 - 5. Return default sample rate / frames per buffer;

We apparently fail to get these properties when running in an emulator,
so we should just return default values.

Review commit: https://reviewboard.mozilla.org/r/244140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244140/

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1458020 - 6. Use separate emulator config for geckoview-junit tests;

Because of the automation VM's audio configuration, using the default
audio backend leads to crashes when testing media. Using the OSS backend
seems to make the crashes go away.

Review commit: https://reviewboard.mozilla.org/r/244142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244142/

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975596 [details]
Bug 1458020 - 1a. Add waitForJS and waitForChromeJS;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243844/diff/1-2/

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975597 [details]
Bug 1458020 - 1b. Add forceGarbageCollection;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243846/diff/1-2/

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975598 [details]
Bug 1458020 - 2. Add PermissionDelegateTest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243848/diff/1-2/

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975599 [details]
Bug 1458020 - 3. Set JavaVM when actually using video capture;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243850/diff/1-2/

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975600 [details]
Bug 1458020 - 4. Add camera permission in the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243852/diff/1-2/

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975596 [details]
Bug 1458020 - 1a. Add waitForJS and waitForChromeJS;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243844/diff/2-3/

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975597 [details]
Bug 1458020 - 1b. Add forceGarbageCollection;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243846/diff/2-3/

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975598 [details]
Bug 1458020 - 2. Add PermissionDelegateTest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243848/diff/2-3/

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975599 [details]
Bug 1458020 - 3. Set JavaVM when actually using video capture;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243850/diff/2-3/

Comment 24

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975600 [details]
Bug 1458020 - 4. Add camera permission in the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243852/diff/2-3/

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975940 [details]
Bug 1458020 - 5. Return default sample rate / frames per buffer;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244140/diff/1-2/

Comment 26

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975941 [details]
Bug 1458020 - 6. Use separate emulator config for geckoview-junit tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244142/diff/1-2/

Comment 27

[Geoff Brown [:gbrown]]

Comment on attachment 8975941 [details]
Bug 1458020 - 6. Use separate emulator config for geckoview-junit tests;

https://reviewboard.mozilla.org/r/244142/#review250106

Interesting.

'try -b do -p android-api-16 -u mochitests,geckoview-junit --rebuild 3' recommended.

Comment 28

[(inactive) Jim Chen [:jchen] [:darchons]]

u(In reply to Geoff Brown [:gbrown] from comment #27)
> Comment on attachment 8975941 [details]
> Bug 1458020 - 6. Use OSS audio backend for ARM 4.3 emulator;
> 
> https://reviewboard.mozilla.org/r/244142/#review250106
> 
> Interesting.
> 
> 'try -b do -p android-api-16 -u mochitests,geckoview-junit --rebuild 3'
> recommended.

Good idea. This caused mochitests to mass-fail. I'm pretty sure "-audio oss" is the right approach for geckoview-junit, but fixing mochitests would take a while. Would you be okay with using a different "androidarm_4_3.py" for geckoview-junit, so we only specify "-audio oss" for the geckoview-junit emulators?

Comment 29

[Geoff Brown [:gbrown]]

Yes, I think that would be okay.

Comment 30

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975596 [details]
Bug 1458020 - 1a. Add waitForJS and waitForChromeJS;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243844/diff/3-4/

Comment 31

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975597 [details]
Bug 1458020 - 1b. Add forceGarbageCollection;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243846/diff/3-4/

Comment 32

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975598 [details]
Bug 1458020 - 2. Add PermissionDelegateTest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243848/diff/3-4/

Comment 33

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975599 [details]
Bug 1458020 - 3. Set JavaVM when actually using video capture;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243850/diff/3-4/

Comment 34

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975600 [details]
Bug 1458020 - 4. Add camera permission in the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243852/diff/3-4/

Comment 35

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975940 [details]
Bug 1458020 - 5. Return default sample rate / frames per buffer;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244140/diff/2-3/

Comment 36

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8975941 [details]
Bug 1458020 - 6. Use separate emulator config for geckoview-junit tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244142/diff/2-3/

Comment 37

[Geoff Brown [:gbrown]]

Comment on attachment 8975941 [details]
Bug 1458020 - 6. Use separate emulator config for geckoview-junit tests;

https://reviewboard.mozilla.org/r/244142/#review250892

That looks fine. Thanks.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d8c5d68082
1a. Add waitForJS and waitForChromeJS; r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7da1af75d9
1b. Add forceGarbageCollection; r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd57487eee34
2. Add PermissionDelegateTest; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/30cb8d1f3fd3
3. Set JavaVM when actually using video capture; r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b61f6598d1
4. Add camera permission in the parent process; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb09eefb0606
5. Return default sample rate / frames per buffer; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/056515942855
6. Use separate emulator config for geckoview-junit tests; r=gbrown

Comment 39

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d9d8c5d68082
https://hg.mozilla.org/mozilla-central/rev/5a7da1af75d9
https://hg.mozilla.org/mozilla-central/rev/cd57487eee34
https://hg.mozilla.org/mozilla-central/rev/30cb8d1f3fd3
https://hg.mozilla.org/mozilla-central/rev/c3b61f6598d1
https://hg.mozilla.org/mozilla-central/rev/bb09eefb0606
https://hg.mozilla.org/mozilla-central/rev/056515942855

Comment 40

[Chris Peterson [:cpeterson]]

Moving some e10s bugs to the new GeckoView::Sandboxing component.