1255628 - illegal access to system library libui.so

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

http://developer.android.com/preview/behavior-changes.html#ndk

I have seen this error in the N preview. Not crashing when I see the message. I tend to see this upon unlocking the phone.

Comment 1

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

I'm not sure we can avoid the private APIs we're currently using, so we might not have a good way forward here. Hopefully the full N release doesn't have these annoying toasts.

Comment 2

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

Attachment: Build against NDK platform 15

Comment 3

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

Attachment: Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents

Comment 4

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

With these patches I think we get rid of a lot of the dlopen, but certainly not all of it.

Comment 6

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

Comment on attachment 8756509 [details] [diff] [review]
Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents

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

Yay for removing code!

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8756505 [details] [diff] [review]
Build against NDK platform 15

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

::: build/autoconf/android.m4
@@ +10,5 @@
>                            use the specified C++ STL (stlport, libstdc++, libc++)],
>      android_cxx_stl=$withval,
>      android_cxx_stl=libc++)
>  
> +define([MIN_ANDROID_VERSION], [15])

Are we dropping support for Android < 4.0.3 ?

Comment 8

[Richard Newman [:rnewman]]

Bug 1155801 and Bug 1237342 dropped < API 15 support from Firefox for Android in 46.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Richard Newman [:rnewman] from comment #8)
> Bug 1155801 and Bug 1237342 dropped < API 15 support from Firefox for
> Android in 46.

O_o these bugs raise more questions...

- Why are --with-android-min-sdk and --with-android-version separate? IOW, what is the point of building with different versions there? Something built with --with-android-min-sdk=15 won't run on < 4.0.3, right, so why care about making the native binaries compatible with older versions?
- Why did https://reviewboard.mozilla.org/r/29721/diff/1#index_header *drop* the android version? Nothing in the context of those bugs explains it (or in the commit message).

Comment 10

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

(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Richard Newman [:rnewman] from comment #8)
> > Bug 1155801 and Bug 1237342 dropped < API 15 support from Firefox for
> > Android in 46.
> 
> O_o these bugs raise more questions...
> 
> - Why are --with-android-min-sdk and --with-android-version separate? IOW,
> what is the point of building with different versions there? Something built
> with --with-android-min-sdk=15 won't run on < 4.0.3, right, so why care
> about making the native binaries compatible with older versions?

I think there is some value to having these separate. You know better than I that newer NDK platform veresions sometimes break stuff (usually due to removed symbols). Pretty sure we don't build against 21 at all (or something close to that, I don't recall).

> - Why did https://reviewboard.mozilla.org/r/29721/diff/1#index_header *drop*
> the android version? Nothing in the context of those bugs explains it (or in
> the commit message).

That was only for b2gdroid. I think it had been incremented incorrectly, IIRC.

At any rate, building against 15 seems like a good idea if we can remove a bunch of code.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8756505 [details] [diff] [review]
Build against NDK platform 15

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

> I think there is some value to having these separate. You know better than I
> that newer NDK platform veresions sometimes break stuff (usually due to
> removed symbols). Pretty sure we don't build against 21 at all (or something
> close to that, I don't recall).

OTOH, recent NDKs have broken *old* API versions (e.g. API version 9 in NDK 11 is not the same as API version 9 in NDK 10)

Anyways, I don't care that much. r+

Comment 12

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

It looks like we could be pretty hosed when the final version of N rolls out according to this[0]. We use several libraries that are not in the temporarily approved list, so as soon as N final is out we will probably crash on startup.

I'm going to land these patches and see where we stand, there's likely more stuff to fix up.

[0] http://android-developers.blogspot.com/2016/06/improving-stability-with-private-cc.html

Comment 13

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

Renoming per shitstorm described in comment #12

Comment 14

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

We need to get this fixed before the final N release. Some folks are predicting that to be as early as mid-late August, so I think that means this needs fixed in 51.

Comment 15

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

Attachment: Don't dlopen libmedia in cubeb

Instead, use JNI in Gecko to obtain latency and preferred sample rate, and pass those to cubeb.

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8775142 [details] [diff] [review]
Don't dlopen libmedia in cubeb

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

You need to rebase this. Conveniently for you, cubeb now use frames for latency, so you can just pass the number directly.

Comment 17

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

(In reply to Paul Adenot (:padenot) from comment #16)
> Comment on attachment 8775142 [details] [diff] [review]
> Don't dlopen libmedia in cubeb
> 
> Review of attachment 8775142 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You need to rebase this. Conveniently for you, cubeb now use frames for
> latency, so you can just pass the number directly.

I already did rebase it to use frames. Did I miss a spot?

Comment 18

[Paul Adenot (:padenot)]

Comment on attachment 8775142 [details] [diff] [review]
Don't dlopen libmedia in cubeb

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

Can't vouch for the java and binding bits, but the C++ bits looks correct.

Remember you need to uplift cubeb_opensl.c to https://github.com/kinetiknz/cubeb, and then use `./update.sh` in `media/libcube/` to get your patch. This might pull in a bit more that what you want. It's all been reviewed by either achronop or myself, feel free to land.

::: dom/media/CubebUtils.cpp
@@ +106,5 @@
> +      if (jni::IsAvailable()) {
> +        value = java::GeckoAppShell::GetAudioOutputLatencyFrames();
> +      } else {
> +        // Need to have something here, go with 10 frames?
> +        value = 10;

10 frames is very very small. More like 256.

::: media/libcubeb/src/cubeb_opensl.c
@@ +413,5 @@
>    stm->lastCompensativePosition = -1;
>  
> +  // If no latency value is passed, use 10ms
> +  if (stm->latency <= 0) {
> +    stm->latency = 10;

This is supposed to be in frame. Use 256, which I just invented, but should be alright.

@@ +451,5 @@
>      res = (*ctx->eng)->CreateAudioPlayer(ctx->eng, &stm->playerObj, &source,
>                                           &sink, NELEMS(ids), ids, req);
>    }
>  
> +  // Sample rate not supported? Try again with 44k?

44.1k. There are also very esoteric audio systems with 44k, and that caused us issues in the past.

@@ +634,5 @@
>    int64_t maximum_position = stm->written * (int64_t)stm->inputrate / stm->outputrate;
>    pthread_mutex_unlock(&stm->mutex);
>    assert(maximum_position >= 0);
>  
> +  if (msec > stm->latency) {

stm->latency is in frames, your comparing it to milliseconds.

Comment 19

[Paul Adenot (:padenot)]

r+ with the the comments fixed.

Comment 20

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

Attachment: Don't dlopen libmedia in cubeb

Instead, use JNI in Gecko to obtain latency and preferred sample rate, and pass those to cubeb.

Comment 21

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

Paul I just want to make sure I didn't screw anything up with the review fixups. Thanks.

Comment 22

[Paul Adenot (:padenot)]

Comment on attachment 8776181 [details] [diff] [review]
Don't dlopen libmedia in cubeb

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

snorp, remember to land the cubeb part via the upstream cubeb repo. achronop or kinetik can help you do that, I'm going on PTO in approximately 30 seconds.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7aa2512bf7
Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3775a4a1ab
Remove AndroidNativeWindow, as we can use the NDK functions directly now r=rbarker

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7a7aa2512bf7
https://hg.mozilla.org/mozilla-central/rev/4a3775a4a1ab

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Snorp, are you going to fill an uplift request for this to aurora & beta? Thanks

Comment 26

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

Comment on attachment 8756509 [details] [diff] [review]
Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents

Approval Request Comment
[Feature/regressing bug #]: Android N release
[User impact if declined]: *possibly* bad behavior on N (though non reported yet)
[Describe test coverage new/current, TreeHerder]: Nightly
[Risks and why]: low, just uses NDK calls for private calls we had before
[String/UUID change made/needed]: none

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hey Kevin, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8756509 [details] [diff] [review]
Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents

Fixes needed for Android N compatibility, let's uplift to Aurora50.

Comment 29

[Kevin Brosnan [Ex-Mozilla]]

I cannot. Google removed the alert dialogs from the last couple Android 7.0 preview releases. The alerts are not in the RTM Android 7.0 builds.

Comment 30

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

I see messages about libmedia.so when using my own (debuggable) build in Android 7. Not sure if the cubeb patch got upstreamed?

Comment 31

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/72984b90a353
https://hg.mozilla.org/releases/mozilla-aurora/rev/3c133d122ea1

Comment 32

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

Correct, the cubeb one is still pending.

Comment 33

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

Actually, I'm going to file a different bug for the cubeb issue with libmedia.so

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8756509 [details] [diff] [review]
Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents

Let's uplift this now and it should be in the beta 10 mobile build on Monday. 
We don't want Android N users to hit this issue which potentially could be a bad startup crash.

Comment 35

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

I'm hitting conflicts uplifting this to beta. Could we get a rebased patch?

Comment 36

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

I think we'll just let this one slide for 49 since it's so late now. Things seem to be working alright on N as it is anyway.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8756509 [details] [diff] [review]
Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents

Clearing the beta approval since we are wontfixing for 49.