Closed
Bug 1255628
Opened 8 years ago
Closed 8 years ago
illegal access to system library libui.so
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 wontfix, firefox49 wontfix, fennec51+, firefox50 fixed, firefox51 fixed)
RESOLVED
FIXED
Firefox 51
People
(Reporter: kbrosnan, Assigned: snorp)
References
Details
Attachments
(3 files, 1 obsolete file)
1.87 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
37.74 KB,
patch
|
jchen
:
review+
rbarker
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
24.10 KB,
patch
|
padenot
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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: ? → +
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8756505 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8756509 -
Flags: review?(rbarker)
Attachment #8756509 -
Flags: review?(nchen)
Assignee | ||
Comment 4•8 years ago
|
||
With these patches I think we get rid of a lot of the dlopen, but certainly not all of it.
Updated•8 years ago
|
Attachment #8756509 -
Flags: review?(rbarker) → review+
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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)
Comment 8•8 years ago
|
||
Bug 1155801 and Bug 1237342 dropped < API 15 support from Firefox for Android in 46.
Comment 9•8 years ago
|
||
(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).
Assignee | ||
Comment 10•8 years ago
|
||
(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 11•8 years ago
|
||
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+
Updated•8 years ago
|
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
Renoming per shitstorm described in comment #12
tracking-fennec: + → ?
Assignee | ||
Comment 14•8 years ago
|
||
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+
Assignee | ||
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8775142 -
Flags: review?(nchen) → review+
Assignee | ||
Comment 17•8 years ago
|
||
(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 18•8 years ago
|
||
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+
Assignee | ||
Comment 20•8 years ago
|
||
Instead, use JNI in Gecko to obtain latency and preferred sample rate, and pass those to cubeb.
Attachment #8776181 -
Flags: review?(padenot)
Assignee | ||
Updated•8 years ago
|
Attachment #8775142 -
Attachment is obsolete: true
Assignee | ||
Comment 21•8 years ago
|
||
Paul I just want to make sure I didn't screw anything up with the review fixups. Thanks.
Comment 22•8 years ago
|
||
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•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7a7aa2512bf7 https://hg.mozilla.org/mozilla-central/rev/4a3775a4a1ab
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•8 years ago
|
Assignee: nobody → snorp
Comment 25•8 years ago
|
||
Snorp, are you going to fill an uplift request for this to aurora & beta? Thanks
Assignee | ||
Comment 26•8 years ago
|
||
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•8 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)
Comment 30•8 years ago
|
||
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)
Comment 31•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/72984b90a353 https://hg.mozilla.org/releases/mozilla-aurora/rev/3c133d122ea1
Assignee | ||
Comment 33•8 years ago
|
||
Actually, I'm going to file a different bug for the cubeb issue with libmedia.so
Comment 34•8 years ago
|
||
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)
Assignee | ||
Comment 36•8 years ago
|
||
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)
Updated•8 years ago
|
Comment 37•8 years ago
|
||
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-
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•