Remove library extraction from APKOpen

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
7 years ago
6 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox13 affected, blocking-fennec1.0 -)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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
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");
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
(Assignee)

Comment 3

7 years ago
Created attachment 602064 [details] [diff] [review]
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.
Assignee: nobody → wjohnston
Attachment #602064 - Flags: review?(mh+mozilla)
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.
Attachment #602064 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 5

7 years ago
Created attachment 602448 [details] [diff] [review]
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).
Attachment #602064 - Attachment is obsolete: true
(Assignee)

Comment 6

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

Updated

7 years ago
Attachment #602448 - Flags: feedback?(mh+mozilla)
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.
Attachment #602448 - Flags: feedback?(mh+mozilla) → feedback+
(Assignee)

Comment 8

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

Comment 9

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

Comment 11

7 years ago
Created attachment 603314 [details] [diff] [review]
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.
Attachment #603314 - Flags: review?(mh+mozilla)
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.
Attachment #603314 - Flags: review?(mh+mozilla)
Attachment #603314 - Flags: review?(blassey.bugs)
Attachment #603314 - Flags: review+
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
Attachment #603314 - Flags: review?(blassey.bugs) → review+
Backed out on inbound to investigate XUL Fennec crashes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5940fe8d1af9
Created attachment 604452 [details] [diff] [review]
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.
Attachment #604452 - Flags: review?(wjohnston)
(Assignee)

Comment 17

7 years ago
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.
Attachment #604452 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/b159cd73c215
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Backed out again temporarily because of broken Android builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a536d22f312c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 13 → ---
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()
https://hg.mozilla.org/mozilla-central/rev/bf4b1d3c624e
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Target Milestone: --- → Firefox 13
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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."
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.
status-firefox13: --- → wontfix
"affected" might be better
status-firefox13: wontfix → affected
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → -
No longer depends on: 807110
(Assignee)

Comment 31

6 years ago
This was fixed somewhere else I think.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.