Closed Bug 588607 Opened 14 years ago Closed 14 years ago

Implement custom dlopen to load libraries from apk

Categories

(Core Graveyard :: Widget: Android, defect)

All
Android
defect
Not set
normal

Tracking

(fennec2.0b2+)

RESOLVED FIXED
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: mwu, Assigned: mwu)

References

Details

Attachments

(6 files, 11 obsolete files)

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
      No description provided.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b2+
Assignee: nobody → mwu
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)
Attachment #478945 - Flags: review?(blassey.bugs)
This patch is pretty terrible. Should I fork this file into an Android specific version?
Attachment #478946 - Flags: feedback?(jones.chris.g)
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)
For anyone testing these patches, the nspr configure needs to be regenerated after applying these patches.
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
(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 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-
Attachment #478947 - Attachment is obsolete: true
Attachment #479976 - Flags: review?(ted.mielczarek)
Attachment #478947 - Flags: feedback?(ted.mielczarek)
Attachment #478947 - Flags: feedback?(khuey)
Attachment #479977 - Flags: review?(ted.mielczarek)
System zlib required to build.
Attachment #479976 - Attachment is obsolete: true
Attachment #480732 - Flags: review?(ted.mielczarek)
Attachment #479976 - Flags: review?(ted.mielczarek)
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)
(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?
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)
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 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+
Attachment #480732 - Flags: review?(ted.mielczarek) → review+
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
Attachment #478944 - Attachment is obsolete: true
Attachment #478945 - Attachment is obsolete: true
Attachment #481369 - 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
Blocks: 607534
Depends on: 647288
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.