Closed Bug 990130 Opened 10 years ago Closed 10 years ago

Investigate UnsatisfiedLinkError caused on trying to load native libraries on some Android devices

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: mfinkle, Assigned: rnewman)

References

Details

Attachments

(1 file, 3 obsolete files)

Blocks: 989787
OS: Linux → Android
Hardware: x86_64 → All
Blacklisting is all or nothing for all APK's in production. So this is cant-fix.
Summary: Blacklist devices that seemingly get the x86 APK but really want ARMv7 APK → Investigate why Google Play offering x86 APK to some ARMv7 devices
http://stackoverflow.com/questions/18111739/why-do-some-android-phones-cause-our-app-to-throw-an-java-lang-unsatisfiedlinker

https://code.google.com/p/android/issues/detail?id=35962
Summary: Investigate why Google Play offering x86 APK to some ARMv7 devices → Investigate UnsatisfiedLinkError caused on trying to load native libraries on some Android devices
11:46:28 < AaronMT> "For whatever reason the System.loadLibrary function tries to look up the native library from the partial update (where it doesn't exist). It's both a flaw in Android and in Google
                    Play."

11:49:08 < rnewman> that does imply that we can work around it
11:49:15 < rnewman> loadLibrary can also take full paths
11:49:26 < rnewman> so if the first try fails, try the full non-dashed path
Patch for discussion. This attempts two tiers of fallbacks. Note that we don't bother with the ApplicationInfo.nativeLibraryPath method -- I would expect that to return the -2 path, not the old path, so it wouldn't help.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Comment on attachment 8400862 [details] [diff] [review]
Fall back to full path of library on failure.

Note that the NativeCrypto stuff duplicates code 'cos that'll need uplift to a-s.
Now with .in file.
Attachment #8400862 - Attachment is obsolete: true
Comment on attachment 8400864 [details] [diff] [review]
Fall back to full path of library on failure. v2

Oops, there's an error in here. The lib string should be prefixed with "lib" in the two fallback cases. Principle still stands.
Attachment #8400864 - Attachment is obsolete: true
Attachment #8400868 - Flags: feedback?(mark.finkle)
Is it possible to create an environment where this is testable in theory?
(In reply to Aaron Train [:aaronmt] from comment #9)
> Is it possible to create an environment where this is testable in theory?

We have not yet been able to reproduce the issue.

If we wanted to move ahead with this idea, we'd need to ship it and watch for the results on Beta.

I think we need blassey and glandium to take a look at the idea and see what gotchas we need to watch.
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(blassey.bugs)
(In reply to Mark Finkle (:mfinkle) from comment #0)
> I have a long list captured here:
> http://people.mozilla.org/~mfinkle/fennec/fennec-release-crashes-43-linker.
> png

https://bugzilla.mozilla.org/show_bug.cgi?id=991055#c1 has a similar list, but in text form.

https://code.google.com/p/android/issues/detail?id=35962#c15 seems to indicate that that issue isn't our issue.

Do we we have any information that leads us to believe that these extra search paths will find the libraries? In the very least, once we get to that last catch statement I'd like to throw an error that contains useful information. We won't get those bug reports until beta, which is unfortunite but at least we'll be in a better place than we are now.
Flags: needinfo?(blassey.bugs)
Is it a case of the system linker looking in a new directory after a system update, but android leaving the files in the old location?
Flags: needinfo?(mh+mozilla)
It's what the going theory is: http://stackoverflow.com/a/18130212
(In reply to Mike Hommey [:glandium] from comment #12)
> Is it a case of the system linker looking in a new directory after a system
> update, but android leaving the files in the old location?

Pretty much. It uses tiers of unpacked APK directories, and doesn't look in the old one when it should (or doesn't put the files in the new one).

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #11)

> Do we we have any information that leads us to believe that these extra
> search paths will find the libraries?

Not tested, but research suggests that those paths will be the unpacked directory. We can verify (some paths, and to a point) by making the earlier clauses deliberately fail. I didn't spend that time yet -- wanted f+ on the general approach.

> In the very least, once we get to that
> last catch statement I'd like to throw an error that contains useful
> information. We won't get those bug reports until beta, which is unfortunite
> but at least we'll be in a better place than we are now.

Yeah, makes sense. No worse than what we currently have.
Comment on attachment 8400868 [details] [diff] [review]
Fall back to full path of library on failure. v3

I am OK with trying a fix like this. We need to be sure that this is no worse than our current situation, which I thinkis the case. We only try fallbacks if the normal flow fails. Currently, failing means crashing.
Attachment #8400868 - Flags: feedback?(mark.finkle) → feedback+
Severity: normal → critical
I'm testing this now. If we handle a deliberate failure correctly, I'll go ahead and hit this on fx-team, assuming that

08:26:36 <@mfinkle> rnewman, reminder for landing the fallback path library loader changes

counts as f+ == r+.
Cleaned up and tested.
Attachment #8407636 - Flags: review?(mark.finkle)
Attachment #8400868 - Attachment is obsolete: true
Comment on attachment 8407636 [details] [diff] [review]
Fall back to full path of library on failure. v2


>diff --git a/mobile/android/base/background/nativecode/NativeCrypto.java b/mobile/android/base/background/nativecode/NativeCrypto.java

I assume we can't use GeckoLoader.doLoadLibrary here


>diff --git a/mobile/android/base/mozglue/GeckoLoader.java.in b/mobile/android/base/mozglue/GeckoLoader.java.in
>     }
> 
>+    
>+    public static void doLoadLibrary(final String lib) {

Drop the extra line break
Attachment #8407636 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/integration/fx-team/rev/be14df51e362

Yes, the duplication is to avoid importing GeckoLoader into ABS.

ni on me for uplift, in the anticipation of wanting to get this into Aurora before the merge. Aaron, mfinkle, please feel free to make a case for pushing this into Beta if you want to.
Here's the exciting and reassuring log output if you try to run an x86 build on an ARM device:

04-16 18:48:10.736 F/GeckoLoader(25608): Couldn't load mozglue. Trying /data/app-lib path.
04-16 18:48:10.746 F/GeckoLoader(25608): Couldn't load mozglue: java.lang.UnsatisfiedLinkError: dlopen failed: library "/data/app-lib/org.mozilla.fennec_rnewman/libmozglue.so" not found. Trying /data/data path.
04-16 18:48:10.746 F/GeckoLoader(25608): Failed every attempt to load mozglue. Giving up.
https://hg.mozilla.org/mozilla-central/rev/be14df51e362
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Blocks: 1042935
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.