illegal access to system library libui.so

RESOLVED FIXED in Firefox 50

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kbrosnan, Assigned: snorp)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 51
All
Android
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, fennec51+, firefox50 fixed, firefox51 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
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.
tracking-fennec: ? → +
Created attachment 8756505 [details] [diff] [review]
Build against NDK platform 15
Attachment #8756505 - Flags: review?(mh+mozilla)
Created attachment 8756509 [details] [diff] [review]
Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents
Attachment #8756509 - Flags: review?(rbarker)
Attachment #8756509 - Flags: review?(nchen)
With these patches I think we get rid of a lot of the dlopen, but certainly not all of it.
Attachment #8756509 - Flags: review?(rbarker) → review+
Duplicate of this bug: 1271551
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!
Attachment #8756509 - Flags: review?(nchen) → review+
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 ?
Attachment #8756505 - Flags: review?(mh+mozilla)
Bug 1155801 and Bug 1237342 dropped < API 15 support from Firefox for Android in 46.
(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).
(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.
Flags: needinfo?(mh+mozilla)
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+
Attachment #8756505 - Flags: review+
Flags: needinfo?(mh+mozilla)
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
Renoming per shitstorm described in comment #12
tracking-fennec: + → ?
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.
tracking-fennec: ? → 51+
Created attachment 8775142 [details] [diff] [review]
Don't dlopen libmedia in cubeb

Instead, use JNI in Gecko to obtain latency and preferred sample rate, and pass those to cubeb.
Attachment #8775142 - Flags: review?(padenot)
Attachment #8775142 - Flags: review?(nchen)
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.
Attachment #8775142 - Flags: review?(padenot)
Attachment #8775142 - Flags: review?(nchen) → review+
(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?
Flags: needinfo?(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.
Attachment #8775142 - Flags: review+
r+ with the the comments fixed.
Flags: needinfo?(padenot)
Created attachment 8776181 [details] [diff] [review]
Don't dlopen libmedia in cubeb

Instead, use JNI in Gecko to obtain latency and preferred sample rate, and pass those to cubeb.
Attachment #8776181 - Flags: review?(padenot)
Attachment #8775142 - Attachment is obsolete: true
Paul I just want to make sure I didn't screw anything up with the review fixups. Thanks.
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.
Attachment #8776181 - Flags: review?(padenot) → review+

Comment 23

2 years ago
Pushed by jwillcox@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7a7aa2512bf7
https://hg.mozilla.org/mozilla-central/rev/4a3775a4a1ab
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Assignee: nobody → snorp
Snorp, are you going to fill an uplift request for this to aurora & beta? Thanks
status-firefox48: affected → wontfix
status-firefox49: --- → affected
status-firefox50: --- → affected
Flags: needinfo?(snorp)
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
Flags: needinfo?(snorp)
Attachment #8756509 - Flags: approval-mozilla-beta?
Attachment #8756509 - Flags: approval-mozilla-aurora?
Hey Kevin, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(kbrosnan)
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.
Attachment #8756509 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 29

2 years ago
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.
Flags: needinfo?(kbrosnan)
I see messages about libmedia.so when using my own (debuggable) build in Android 7. Not sure if the cubeb patch got upstreamed?
Flags: needinfo?(snorp)
Correct, the cubeb one is still pending.
Flags: needinfo?(snorp)
Actually, I'm going to file a different bug for the cubeb issue with libmedia.so
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.
Attachment #8756509 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'm hitting conflicts uplifting this to beta. Could we get a rebased patch?
Flags: needinfo?(snorp)
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.
Flags: needinfo?(snorp)
status-firefox49: affected → wontfix
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.
Attachment #8756509 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.