Closed Bug 1298090 Opened 8 years ago Closed 8 years ago

Extract shared libraries on APK update

Categories

(Firefox for Android Graveyard :: General, defect)

51 Branch
All
Android
defect
Not set
normal

Tracking

(firefox51 wontfix, firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
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-
Moved refactoring out into a new patch.
Attachment #8784909 - Attachment is obsolete: true
Attachment #8785289 - Flags: review?(mh+mozilla)
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.
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)
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)
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+
(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.
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+
Throw and log error in Java.
Attachment #8798024 - Attachment is obsolete: true
Attachment #8799528 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/0470b254efe4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1309708
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: