Extract shared libraries on APK update

RESOLVED FIXED in Firefox 52

Status

()

Firefox for Android
General
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

51 Branch
Firefox 52
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

a year ago
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
(Assignee)

Comment 1

a year ago
Created attachment 8784909 [details] [diff] [review]
0001-Bug-1298090-1.1-Extract-and-cache-libxul.so-on-APK-u.patch

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.
Attachment #8784909 - Flags: review?(snorp)
Attachment #8784909 - Flags: review?(mh+mozilla)
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?
Attachment #8784909 - Flags: review?(snorp) → review+
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.
Attachment #8784909 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 4

a year ago
Created attachment 8785289 [details] [diff] [review]
0001-Bug-1298090-1.2-Extract-and-cache-libxul.so-on-APK-u.patch

Moved refactoring out into a new patch.
Attachment #8784909 - Attachment is obsolete: true
Attachment #8785289 - Flags: review?(mh+mozilla)
(Assignee)

Comment 5

a year ago
Created attachment 8785292 [details] [diff] [review]
0002-Bug-1298090-2.1-Refactor-Elfloader-GetMappableFromPa.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.
Attachment #8785292 - Flags: review?(mh+mozilla)
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.
Attachment #8785289 - Flags: review?(mh+mozilla)
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.
Attachment #8785292 - Flags: review?(mh+mozilla)
(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.
(Assignee)

Comment 9

a year ago
Created attachment 8795339 [details] [diff] [review]
0001-Bug-1298090-1.3-Extract-and-cache-native-libraries-o.patch

Addressed comments.
Attachment #8785289 - Attachment is obsolete: true
Attachment #8795339 - Flags: review?(mh+mozilla)
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)
Attachment #8795339 - Flags: review?(mh+mozilla)
(Assignee)

Comment 11

a year ago
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?
Attachment #8795339 - Attachment is obsolete: true
Attachment #8797599 - Flags: review?(mh+mozilla)
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.
Attachment #8797599 - Flags: review?(mh+mozilla)
(Assignee)

Comment 13

a year ago
Created attachment 8798024 [details] [diff] [review]
0001-Bug-1298090-1.5-Extract-and-cache-native-libraries-o.patch

Addressed comments.
Attachment #8785292 - Attachment is obsolete: true
Attachment #8797599 - Attachment is obsolete: true
Attachment #8798024 - Flags: review?(mh+mozilla)
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?
Attachment #8798024 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 15

a year ago
(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.
(Assignee)

Comment 16

a year ago
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.
Attachment #8798024 - Flags: review?(snorp)
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.
Attachment #8798024 - Flags: review?(snorp) → review+
(Assignee)

Comment 18

a year ago
Created attachment 8799528 [details] [diff] [review]
0001-Bug-1298090-1.6-Extract-and-cache-native-libraries-o.patch

Throw and log error in Java.
Attachment #8798024 - Attachment is obsolete: true
Attachment #8799528 - Flags: review+
(Assignee)

Comment 19

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

Comment 20

a year ago
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

Comment 21

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0470b254efe4
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
(Assignee)

Updated

a year ago
Depends on: 1309708
status-firefox51: affected → wontfix
You need to log in before you can comment on or make changes to this bug.