732069 - Remove library extraction from APKOpen

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

With the new linker, we think we can remove all our library extraction stuff to make this code simpler. There are some caveats. Because I'll probably write them wrong, I'm going to paste an IRC conversation here:

09:33:13 AM - dougt: question - we are done w/ extracting libs now, right? we have some code that wesj is moving around. it deals with extracting libs if we use the debug intent
09:33:17 AM - dougt: i didn't think that was required any longer.
09:33:45 AM - glandium: dougt: so, there are essentially two answers to that.
09:34:22 AM - glandium: three, even
09:36:22 AM - glandium: so, first thing is that with the old linker (so, on xul builds) gcc is happy with debuggable builds (debuggable flag in AndroidManifest.xml) without extracting libs. (it's not on non-debuggable builds) ; the new linker makes gcc happy whether it's debuggable or not
09:36:56 AM - glandium: other tools, such as valgrind, require library extraction, so the lib extraction code is kept around for them
09:37:23 AM - glandium: then, the new linker takes care of library extraction independently, while the old linker has APKOpen.cpp do it
09:38:06 AM - glandium: so, until the old linker goes away, we need to maintain that code
09:41:54 AM - dougt: glandium: so, keep the code for the old linker?
09:42:02 AM - dougt: and that is only used for the XUL build?
09:42:10 AM - glandium: dougt: indeed
09:42:17 AM - dougt: glandium: can valgrind get around this?
09:42:48 AM - glandium: dougt: needs modifications to valgrind
09:43:00 AM - dougt: glandium: i know someone...
09:43:08 AM - dougt: so, i don't care about debugging xul.
09:43:19 AM - dougt: it is kinda tagged and bagged at this point.
09:43:55 AM - dougt: suppose that someone cared... they would just have to unzip the jar, move libraries to cache, and continue?
09:43:55 AM - glandium: dougt: feel free to file a bug to have lib extraction removed from APKOpen.cpp, then
09:44:05 AM - dougt: glandium: yes. wesj is doing that now
09:44:14 AM - dougt: glandium: i just want to be sure what we break by doing it
09:44:29 AM - glandium: dougt: nothing important :)
09:44:49 AM - glandium: dougt: one thing that *needs* to stay there, though, is extracting lib.id

Comment 1

[Mike Hommey [:glandium]]

I'll add that another thing related to library extraction that needs to remain in APKOpen.cpp is the conditional putenv("MOZ_LINKER_EXTRACT=1");

Comment 2

[Mike Hommey [:glandium]]

reading the code i realize i need to add another comment: extractFile is the one that needs to go, but not extractLib, as it decompresses libraries in memory for the old linker

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

This removes the bits I think we don't need. Built and ran both native and xul fennec fine with this. (Note, I yanked out the MOZ_LINKER_EXTRACT bits in bug 731341). If/When we land that I'll update any wiki docs I can find so that debuggers know to add it to their environment.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 602064 [details] [diff] [review]
Patch

Review of attachment 602064 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/android/APKOpen.cpp
@@ +318,1 @@
>  extern "C" int extractLibs = 0;

You need to remove this, the extractLibs assignment in Java_org_mozilla_gecko_GeckoAppShell_loadSQLiteLibsNative, and the extractLibs checks in other-licenses/android/dlfcn.c.

I think that's required if you don't want things to break with the debug intent on xul (which is still required for gdb, intependently of extracting libs or not)

While there, you can also remove the putenv("MOZ_LINKER_EXTRACT=1"); and subsequently, the jShouldExtract argument to Java_org_mozilla_gecko_GeckoAppShell_loadSQLiteLibsNative.
But that's optional.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2

> >  extern "C" int extractLibs = 0;
> 
> You need to remove this, the extractLibs assignment in
> Java_org_mozilla_gecko_GeckoAppShell_loadSQLiteLibsNative, and the
> extractLibs checks in other-licenses/android/dlfcn.c.

Removed the dlfcn.c stuff. I already yanked out the loadSQLiteLibsNative bits in bug 731341 (because I was already reorganizing GeckoAppShell there and in the process removed the jShouldExtract parameter to Java_org_mozilla_gecko_GeckoAppShell_loadSQLiteLibsNative as well).

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 602448 [details] [diff] [review]
Patch v2

I pushed this to try on Friday with some other tests and noticed that something in the queue broke tests on XUL Fennec (probably this since nothing else should touch XUL Fennec).

https://tbpl.mozilla.org/?tree=Try&rev=e73b11c50c00

I'll try this alone on Monday, but I'm new to this code, so any hints as to what's going on would be helpful.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 602448 [details] [diff] [review]
Patch v2

Review of attachment 602448 [details] [diff] [review]:
-----------------------------------------------------------------

I'd expect this patch to work properly. Please note you can also remove apk_mtime.

Comment 8

[Wesley Johnston (:wesj)]

My second try push of that patch is looking happier: https://tbpl.mozilla.org/?tree=Try&rev=b93deb673ef7. If a few more things fill in ok, i'll request review again.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 602448 [details] [diff] [review]
Patch v2

No changes at all, but this try run looks much happier. Must be something else in that other patch queue angering XUL....

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 602448 [details] [diff] [review]
Patch v2

Review of attachment 602448 [details] [diff] [review]:
-----------------------------------------------------------------

Same comment as before, you can also remove apk_mtime.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch part 2

I pulled this out of bug 731341 because I thought it was causing problems with jimdb. But those are actually apparently from bug 732117.

I'm not exactly sure why we're iterating through this cache and deleting everything though, so I'd like to make sure thats ok to do.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 603314 [details] [diff] [review]
Patch part 2

Review of attachment 603314 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure about the GRE_HOME bit.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 603314 [details] [diff] [review]
Patch part 2

Review of attachment 603314 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoAppShell.java
@@ +248,5 @@
>          // libxul will depend on.  Not ideal.
>          GeckoProfile profile = GeckoProfile.get(context);
>  
>          File cacheFile = getCacheDir(context);
> +        File greDir = new File(context.getApplicationInfo().dataDir);

GeckoAppShell.getGREDir(context); // This is a recent addition to GeckoAppShell

Comment 14

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3689bb4199d8

Comment 15

[Matt Brubeck (:mbrubeck)]

Backed out on inbound to investigate XUL Fennec crashes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5940fe8d1af9

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fix startup crash with old linker

Latest maple built with the old linker crashes on startup. I assume m-c suffers from the same problem. This patch works for me on maple.

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 604452 [details] [diff] [review]
Fix startup crash with old linker

Review of attachment 604452 [details] [diff] [review]:
-----------------------------------------------------------------

Dropped this at some point during my patch wrangling.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b159cd73c215

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/b159cd73c215

Comment 20

[Wesley Johnston (:wesj)]

backed out and relanded
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d129062b2b

Comment 21

[Matt Brubeck (:mbrubeck)]

Backed out again temporarily because of broken Android builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a536d22f312c

Comment 22

[Wesley Johnston (:wesj)]

And landed again
https://hg.mozilla.org/integration/mozilla-inbound/

Comment 23

[Gian-Carlo Pascutto [:gcp]]

Profile Migration has code to clean up any old caches from the XUL build. I think you'll want to uncomment it in this bug? Look for cleanupXULLibCache()

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/bf4b1d3c624e

Comment 25

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a0eff95de19b - it looks (based on https://tbpl.mozilla.org/?tree=Try&fromchange=6820ae07bfaa&tochange=68dd518f6a5f) like this is the reason we went from around one failed to start up the browser (bug 722166, bug 690176, several more for talos) a day to two or five or seven per push.

Comment 26

[Phil Ringnalda (:philor)]

And https://tbpl.mozilla.org/?tree=Try&rev=721d921ef238 is with this in, but with the two instances of "putenv("MOZ_LINKER_EXTRACT=1")" in APKOpen.cpp removed, so it looks like glandium's feeling that those were the problem is correct.

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a0eff95de19b

Comment 28

[Phil Ringnalda (:philor)]

Though once my ten rounds on the comment 26 try push finished, there were two Talos "initialization timed out" failures, which feels like quite a lot for ten rounds - normal was "yeah, maybe two in one day, but then none for four or five days."

Comment 29

[Phil Ringnalda (:philor)]

Backed out of 13 in https://hg.mozilla.org/releases/mozilla-aurora/rev/1eb6d3a4018b - though not accurate, wontfix seems to be as close to defixed as we have for a status flag.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

"affected" might be better

Comment 31

[Wesley Johnston (:wesj)]

This was fixed somewhere else I think.