In bug 1291424 we want to disable the on-demand decompression linker by extracting and caching shared libraries persistently. In bug 1294736 we look into LZMA-compression of the shared libraries to reduce APK size. However, LZMA-decoding is time consuming, which increases the initial (first startup after APK update) startup duration. We can avoid this by extracting the shared libraries in background after an APK update. This should allow for Fennec to load the extracted cached libs on its first startup. Also see https://bugzilla.mozilla.org/show_bug.cgi?id=1294736#c6

With this patch, we listen to ACTION_MY_PACKAGE_REPLACED intents which are dispatched when the APK has been updated. The receiver then extracts libxul to allow it to be loaded from cache on startup. I had to refactor some linker code to make parts of it reusable, otherwise we would end up with a lot of duplicated code.

Comment on attachment 8784909 [details] [diff] [review] 0001-Bug-1298090-1.1-Extract-and-cache-libxul.so-on-APK-u.patch Review of attachment 8784909 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with nits ::: mozglue/android/APKOpen.cpp @@ +207,5 @@ > + > +static mozglueresult > +extractGeckoLibs(const char *apkName) > +{ > + UniquePtr<char[]> filePath(getAPKAssetsFilePath(apkName, "libxul.so")); Do we want to do more than libxul.so? It's certainly the largest, but maybe we should put the other libs here too. @@ +208,5 @@ > +static mozglueresult > +extractGeckoLibs(const char *apkName) > +{ > + UniquePtr<char[]> filePath(getAPKAssetsFilePath(apkName, "libxul.so")); > + ElfLoader::ExtractFromPath(filePath.get()); We if this method is going to return some kind of success/error, we should actually do that. ::: mozglue/linker/ElfLoader.cpp @@ +459,5 @@ > + if (!zip || !zip->GetStream(subPath, &stream)) { > + return; > + } > + const char* name = LeafName(path); > + RefPtr<Mappable> mappable = MappableExtractFile::Create(name, zip, &stream); This can fail, right, so maybe return bool?

Comment on attachment 8784909 [details] [diff] [review] 0001-Bug-1298090-1.1-Extract-and-cache-libxul.so-on-APK-u.patch Review of attachment 8784909 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/android/APKOpen.cpp @@ -191,5 @@ > info->offset = offset; > } > > -static void* > -dlopenAPKLibrary(const char* apkName, const char* libraryName) It'd be way simpler to keep the dlopen as it is. It won't execute code besides static initializers, and will pull dependencies automatically. You may, however, want to dlclose libxul.so once extracted.

Moved refactoring out into a new patch.

GetMappableFromPath is difficult to read due to its deep if-nesting and mixed responsibilities and it leaks memory (zip_path). The patch is small and relevant, we can piggyback it with this bug.

Comment on attachment 8785289 [details] [diff] [review] 0001-Bug-1298090-1.2-Extract-and-cache-libxul.so-on-APK-u.patch Review of attachment 8785289 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/android/APKOpen.cpp @@ +181,4 @@ > extern "C" void > report_mapping(char *name, void *base, uint32_t len, uint32_t offset) > { > + if (mapping_count >= MAX_MAPPING_INFO || !lib_mapping) { There's an interesting possible race condition here that would make libxul unknown to the crash reporter... I guess it's time to finish bug 689178 @@ +322,5 @@ > + const char* str = jenv->GetStringUTFChars(jApkName, nullptr); > + if (str == nullptr) { > + return; > + } > + int res = loadGeckoLibs(str); loadGeckoLibs does call XRE_StartupTimelineRecord. Might be better to avoid that. Do we care about the logging of how long it takes to load the libraries? then you could just use dlopenAPKLibrary(). That would also avoid a race condition with xul_handle, in case some other thread tries to actually start the browser at the same time.

Comment on attachment 8785292 [details] [diff] [review] 0002-Bug-1298090-2.1-Refactor-Elfloader-GetMappableFromPa.patch Review of attachment 8785292 [details] [diff] [review]: ----------------------------------------------------------------- This should be in two separate bugs. One for the memleak, one for the refactor. ::: mozglue/linker/ElfLoader.cpp @@ +419,5 @@ > return nullptr; > } > > +static already_AddRefed<Zip> > +GetZip(const char* path, const char** subPath) I feel it would be better to have a function that splits the path in the two parts.

(In reply to Mike Hommey [:glandium] from comment #6) > Comment on attachment 8785289 [details] [diff] [review] > 0001-Bug-1298090-1.2-Extract-and-cache-libxul.so-on-APK-u.patch > > Review of attachment 8785289 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mozglue/android/APKOpen.cpp > @@ +181,4 @@ > > extern "C" void > > report_mapping(char *name, void *base, uint32_t len, uint32_t offset) > > { > > + if (mapping_count >= MAX_MAPPING_INFO || !lib_mapping) { > > There's an interesting possible race condition here that would make libxul > unknown to the crash reporter... I guess it's time to finish bug 689178 Alternatively, we could ensure the java code never attempts to start gecko until it's done extracting it.

Addressed comments.

Comment on attachment 8795339 [details] [diff] [review] 0001-Bug-1298090-1.3-Extract-and-cache-native-libraries-o.patch Review of attachment 8795339 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/android/APKOpen.cpp @@ +314,5 @@ > extern "C" NS_EXPORT void MOZ_JNICALL > +Java_org_mozilla_gecko_mozglue_GeckoLoader_extractGeckoLibsNative( > + JNIEnv *jenv, jclass jGeckoAppShellClass, jstring jApkName) > +{ > + static const std::vector<const char*> kLibs = {"libxul.so", "libnss3.so"}; Why is libnss3.so in there? Especially after libxul.so, that seems unnecessary (libxul.so depends on libnss3, so loading the former will load the latter)

Don't explicitly extract libnss3.so. Do we still want to keep the vector and loop for easy expandability in future?

Comment on attachment 8797599 [details] [diff] [review] 0001-Bug-1298090-1.4-Extract-and-cache-native-libraries-o.patch Review of attachment 8797599 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Eugen Sawin [:esawin] from comment #11) > Created attachment 8797599 [details] [diff] [review] > 0001-Bug-1298090-1.4-Extract-and-cache-native-libraries-o.patch > > Don't explicitly extract libnss3.so. > Do we still want to keep the vector and loop for easy expandability in > future? no.

Addressed comments.

Comment on attachment 8798024 [details] [diff] [review] 0001-Bug-1298090-1.5-Extract-and-cache-native-libraries-o.patch Review of attachment 8798024 [details] [diff] [review]: ----------------------------------------------------------------- Please not this is r+ for the C++ side only. I haven't looked at the Java side. I assume snorp reviewed that already. ::: mozglue/android/APKOpen.cpp @@ +328,5 @@ > + "Extracted and cached libxul.so."); > + // We have extracted and cached the lib, we can close it now. > + __wrap_dlclose(handle); > + } else { > + // Don't JNI-throw on error here since we're doing this in background. Not that it matters much, but why not throw an error and catch it on the java side?

(In reply to Mike Hommey [:glandium] from comment #14) > Not that it matters much, but why not throw an error and catch it on the > java side? We already log the error in C++, there is nothing more we can or have to do here in Java.

Comment on attachment 8798024 [details] [diff] [review] 0001-Bug-1298090-1.5-Extract-and-cache-native-libraries-o.patch The Java parts haven't changed, the rest has been trimmed down so that most of your first comments no longer apply.

Comment on attachment 8798024 [details] [diff] [review] 0001-Bug-1298090-1.5-Extract-and-cache-native-libraries-o.patch Review of attachment 8798024 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/android/APKOpen.cpp @@ +328,5 @@ > + "Extracted and cached libxul.so."); > + // We have extracted and cached the lib, we can close it now. > + __wrap_dlclose(handle); > + } else { > + // Don't JNI-throw on error here since we're doing this in background. It probably is a little better to throw here and log the below message in Java, but I don't care that much.

Throw and log error in Java.

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=127a4482e0fd620ac2cbc0acb87f4cd7eb5b102c&selectedJob=28904000

Pushed by esawin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0470b254efe4 [1.6] Extract and cache native libraries on APK update. r=glandium,snorp