Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement custom dlopen to load libraries from apk

RESOLVED FIXED

Status

()

Core
Widget: Android
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

(Blocks: 1 bug)

Trunk
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(6 attachments, 11 obsolete attachments)

135.27 KB, patch
Details | Diff | Splinter Review
43.91 KB, patch
Details | Diff | Splinter Review
4.31 KB, patch
Details | Diff | Splinter Review
3.82 KB, patch
Details | Diff | Splinter Review
2.33 KB, patch
Details | Diff | Splinter Review
1.75 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

7 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b2+
(Assignee)

Updated

7 years ago
Assignee: nobody → mwu
(Assignee)

Updated

7 years ago
Depends on: 595989
(Assignee)

Comment 1

7 years ago
Created attachment 478935 [details] [diff] [review]
0. Add bionic linker files to other-licenses/android

From http://android.git.kernel.org/?p=platform/bionic.git;a=commit;h=312be567a03aaf851707a268807ee666b12f8c74
(Assignee)

Comment 2

7 years ago
Created attachment 478944 [details] [diff] [review]
1. Add support for reading libraries out of the apk

This patch adds a APKOpen.cpp to read libraries to read APKs, extract them, load them, and then forward the functions called by the java wrapper to libxul. It also hacks the bionic linker to read libraries out of memory without an associated fd.
Attachment #478944 - Flags: review?(tglek)
(Assignee)

Comment 3

7 years ago
Created attachment 478945 [details] [diff] [review]
2. Make the Java wrapper use the new library loader
Attachment #478945 - Flags: review?(blassey.bugs)
(Assignee)

Comment 4

7 years ago
Created attachment 478946 [details] [diff] [review]
3. Add support for the custom library loader to plugin-container

This patch is pretty terrible. Should I fork this file into an Android specific version?
Attachment #478946 - Flags: feedback?(jones.chris.g)
(Assignee)

Comment 5

7 years ago
Created attachment 478947 [details] [diff] [review]
4. Wrap the dl* functions with our own version

Pretty ugly. Guess we'll need our own wrapper variables? Or maybe turn the wrapper vars into something more generic.
Attachment #478947 - Flags: feedback?(ted.mielczarek)
Attachment #478947 - Flags: feedback?(khuey)
(Assignee)

Comment 6

7 years ago
For anyone testing these patches, the nspr configure needs to be regenerated after applying these patches.
(Assignee)

Updated

7 years ago
Attachment #478935 - Flags: review?(lvilla)
Comment on attachment 478944 [details] [diff] [review]
1. Add support for reading libraries out of the apk

you should use hg rename for moving these files
Comment on attachment 478945 [details] [diff] [review]
2. Make the Java wrapper use the new library loader

>-libs/armeabi/%: $(DIST)/lib/%
>-	@$(NSINSTALL) -D libs/armeabi
>+lib/%: $(DIST)/lib/%
>+	@$(NSINSTALL) -D lib/armeabi
why do you need to create the armeabi folder here?

seems like you create it here again:
>+	$(NSINSTALL) -D lib/armeabi
>+	mv lib/libmozutils.so lib/armeabi
(Assignee)

Comment 9

7 years ago
(In reply to comment #8)
> Comment on attachment 478945 [details] [diff] [review]
> 2. Make the Java wrapper use the new library loader
> 
> >-libs/armeabi/%: $(DIST)/lib/%
> >-	@$(NSINSTALL) -D libs/armeabi
> >+lib/%: $(DIST)/lib/%
> >+	@$(NSINSTALL) -D lib/armeabi
> why do you need to create the armeabi folder here?
> 
> seems like you create it here again:
> >+	$(NSINSTALL) -D lib/armeabi
> >+	mv lib/libmozutils.so lib/armeabi

I'll remove one of them.
Comment on attachment 478935 [details] [diff] [review]
0. Add bionic linker files to other-licenses/android

r+ generally (to the extent I can really r anything :), but note that you should put this in about:license as well. Might want to file a separate bug for that.
Attachment #478935 - Flags: review?(lvilla) → review+
Comment on attachment 478945 [details] [diff] [review]
2. Make the Java wrapper use the new library loader

>-libs/armeabi/%: $(DIST)/lib/%
>-	@$(NSINSTALL) -D libs/armeabi
>+lib/%: $(DIST)/lib/%
>+	@$(NSINSTALL) -D lib/armeabi
no need to create the armeabi dir here

> 	@cp -L -v $< $@
> 	@$(DO_STRIP) $@
> 
> # Bug 567873 - Android packaging should use standard packaging code
>-dist: FORCE
>+dist: FORCE $(FULL_LIBS)
as I've understand it, if you have FORCE here, adding anything else is superfluous
 

>+dist.zip: dist
>+	rm -f dist.zip
>+	cd dist && zip -r8D ../dist.zip .
Does this pull armeabi/libgeckoutils.so into the zip? I'd expect that to be unneeded
Attachment #478945 - Flags: review?(blassey.bugs) → review+

Comment 12

7 years ago
Comment on attachment 478944 [details] [diff] [review]
1. Add support for reading libraries out of the apk

r-
* for commented out code
* using raw numbers instead of constants

r+ will come with the agreement that the extra memcpy will be removed in a future enhancement.
Attachment #478944 - Flags: review?(tglek) → review-
Comment on attachment 478946 [details] [diff] [review]
3. Add support for the custom library loader to plugin-container

Yeah, let's have a different file for this.
Attachment #478946 - Flags: feedback?(jones.chris.g) → feedback-
(Assignee)

Comment 14

7 years ago
Created attachment 479976 [details] [diff] [review]
4a. Wrap the dl* functions with our own version
Attachment #478947 - Attachment is obsolete: true
Attachment #479976 - Flags: review?(ted.mielczarek)
Attachment #478947 - Flags: feedback?(ted.mielczarek)
Attachment #478947 - Flags: feedback?(khuey)
(Assignee)

Comment 15

7 years ago
Created attachment 479977 [details] [diff] [review]
4b. Wrap the dl* functions with our own version (NSPR)
Attachment #479977 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 16

7 years ago
Created attachment 480732 [details] [diff] [review]
4a. Wrap the dl* functions with our own version, v2

System zlib required to build.
Attachment #479976 - Attachment is obsolete: true
Attachment #480732 - Flags: review?(ted.mielczarek)
Attachment #479976 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 17

7 years ago
Created attachment 480748 [details] [diff] [review]
3. Add support for the custom library loader to plugin-container, v2
Attachment #478946 - Attachment is obsolete: true
Attachment #480748 - Flags: review?(jones.chris.g)
Comment on attachment 480748 [details] [diff] [review]
3. Add support for the custom library loader to plugin-container, v2

>--- /dev/null
>+++ b/ipc/app/MozillaRuntimeMainAndroid.cpp
>+int
>+main(int argc, char* argv[])
>+{
>+    // Check for the absolute minimum number of args we need to move
>+    // forward here. We expect the last arg to be the child process type.
>+    if (argc < 1)

ITYM < 2 here.

>+    void *mozloader_handle = dlopen("libmozutils.so", RTLD_LAZY);
>+    if (!mozloader_handle)
>+        __android_log_print(ANDROID_LOG_ERROR, "GeckoChildLoad",
>+                            "Couldn't load mozloader because %s", dlerror());
>+

exit on error.

>+    typedef int (*ChildProcessInit_t)(int, char**);
>+    ChildProcessInit_t fChildProcessInit =
>+        (ChildProcessInit_t)dlsym(mozloader_handle, "ChildProcessInit");
>+    if (!fChildProcessInit)
>+        __android_log_print(ANDROID_LOG_ERROR, "GeckoChildLoad",
>+                            "Couldn't load cpi_t because %s", dlerror());
>+

exit on error.
Attachment #480748 - Flags: review?(jones.chris.g)
(Assignee)

Comment 19

7 years ago
(In reply to comment #18)
> Comment on attachment 480748 [details] [diff] [review]
> 3. Add support for the custom library loader to plugin-container, v2
> 
> >--- /dev/null
> >+++ b/ipc/app/MozillaRuntimeMainAndroid.cpp
> >+int
> >+main(int argc, char* argv[])
> >+{
> >+    // Check for the absolute minimum number of args we need to move
> >+    // forward here. We expect the last arg to be the child process type.
> >+    if (argc < 1)
> 
> ITYM < 2 here.
> 
This was code copied from MozillaRuntimeMain.cpp. Should it be 2 there too?
(Assignee)

Comment 20

7 years ago
Created attachment 481369 [details] [diff] [review]
1a. Fix review comments, add passthrough mode

This fixes review comments and adds a new mode for debug builds which extracts the libraries and loads them through the system linker.
Attachment #481369 - Flags: review?(tglek)
(Assignee)

Comment 21

7 years ago
Created attachment 481370 [details] [diff] [review]
3. Add support for the custom library loader to plugin-container, v3
Attachment #480748 - Attachment is obsolete: true
Attachment #481370 - Flags: review?(jones.chris.g)
Attachment #481370 - Flags: review?(jones.chris.g) → review+
Comment on attachment 479977 [details] [diff] [review]
4b. Wrap the dl* functions with our own version (NSPR)

># HG changeset patch
># Parent 0bc7433dc518b4d49f47f2356774406a4422b018
>
>diff --git a/nsprpub/configure.in b/nsprpub/configure.in
>--- a/nsprpub/configure.in
>+++ b/nsprpub/configure.in
>@@ -194,6 +194,8 @@ if test "$target" = "arm-android-eabi" ;
>         HOST_LDFLAGS=" "
>     fi
> 
>+    WRAP_MALLOC_CFLAGS="-Wl,--wrap -Wl,dlopen -Wl,--wrap -Wl,dlclose -Wl,--wrap -Wl,dlerror -Wl,--wrap -Wl,dlsym -Wl,--wrap -Wl,dladdr"

Nitpicky, but is it possible to write this as:
WRAP_MALLOC_CFLAGS='-Wl,"--wrap dlopen --wrap dlclose ..."'
?

Won't work as easily for the hunk below because you want variable interpolation.
Attachment #479977 - Flags: review?(ted.mielczarek) → review+

Comment 23

7 years ago
Comment on attachment 481369 [details] [diff] [review]
1a. Fix review comments, add passthrough mode

>+static void
>+extractFile(const char * path, const struct cdir_entry *entry, void * data)

As discussed on irc, should just mmap the file for writing, instead of C convulsions.
Low priority.

>+static void
>+extractLib(const struct cdir_entry *entry, void * data, void * buf)
>+{
buf is a bad name, say dest or something.
Attachment #481369 - Flags: review?(tglek) → review+
Blocks: 603592
Attachment #480732 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 24

7 years ago
Created attachment 483300 [details] [diff] [review]
0. Add bionic linker files to other-licenses/android (hg patch ready)
Attachment #478935 - Attachment is obsolete: true
(Assignee)

Comment 25

7 years ago
Created attachment 483302 [details] [diff] [review]
1. Add support for reading libraries out of the apk, v2 (hg patch ready)
(Assignee)

Updated

7 years ago
Attachment #483302 - Attachment description: 1. Add support for reading libraries out of the apk → 1. Add support for reading libraries out of the apk, v2 (hg patch ready)
Attachment #483302 - Attachment is patch: true
Attachment #483302 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 26

7 years ago
Created attachment 483305 [details] [diff] [review]
2. Make the Java wrapper use the new library loader, v2 (hg patch ready)
Attachment #478944 - Attachment is obsolete: true
Attachment #478945 - Attachment is obsolete: true
Attachment #481369 - Attachment is obsolete: true
(Assignee)

Comment 27

7 years ago
Created attachment 483306 [details] [diff] [review]
3. Add support for the custom library loader to plugin-container, v3 (hg patch ready)
Attachment #481370 - Attachment is obsolete: true
(Assignee)

Comment 28

7 years ago
Created attachment 483308 [details] [diff] [review]
4a. Wrap the dl* functions with our own version, v3 (hg patch ready)
Attachment #480732 - Attachment is obsolete: true
(Assignee)

Comment 29

7 years ago
Created attachment 483310 [details] [diff] [review]
4b. Wrap the dl* functions with our own version (NSPR) (hg patch ready)
Attachment #479977 - Attachment is obsolete: true
Comment on attachment 483310 [details] [diff] [review]
4b. Wrap the dl* functions with our own version (NSPR) (hg patch ready)

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.289; previous revision: 1.288
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.292; previous revision: 1.291
done
(Assignee)

Comment 31

7 years ago
http://hg.mozilla.org/mozilla-central/rev/6277ca1ac576 Patch 0
http://hg.mozilla.org/mozilla-central/rev/266535ecad79 Patch 1
http://hg.mozilla.org/mozilla-central/rev/a6b6ebdd29a2 Patch 2
http://hg.mozilla.org/mozilla-central/rev/884b917c7628 Patch 3
http://hg.mozilla.org/mozilla-central/rev/e3cdf1619342 Patch 4a
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Blocks: 607534
Blocks: 608593
Depends on: 647288
You need to log in before you can comment on or make changes to this bug.