Bug 1291424 affects the startup timings which results in intermittent crashes when trying to access unmapped memory. I think this is due to incorrect handling of the native Zip resources in NativeZip. 1. We allow for 1-to-N relationships between a NativeZip and InputStreams via NativeZip::getInputStream, but each returned InputStream releases the parent NativeZip when closed. 2. We allow for NativeZip construction based on an a given InputStream for children jar files without keeping the original NativeZip referenced which holds the memory mapping. 3. ZipCollection is not thread-safe. 4. GeckoJarReader::getStream may return null, we don't check for it. 5. lib_mapping may be accessed before initialization, handled in bug 1301665.

Don't allow manual closing of ByteBufferInputStream and NativeZip as the referenced native resources may be shared across streams. If we want to allow for manual closing, we would need to implement custom ref count management (a la https://hg.mozilla.org/try/rev/acd6a97dab4c9325c31b9214782110aeeb9ed28f).

The named zips handle the memory mapping and need to be referenced by the children zips to prevent gc.

Mutex-locking in ZipCollection.

Check for null return from GeckoJarReader::getStream.

This is interesting, apparently the patches above fix the crash tests [1] but not the rest of the tests [2]. For comparison, the custom ref count implementation (on top of the patches) passes the tests [3]. I must have missed something. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=852827c4af7c [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1494af2fb22 [3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=33c1a5d24a0e

Comment on attachment 8790424 [details] [diff] [review] 0003-Bug-1302189-3.1-Add-mutex-locking-to-ZipCollection-z.patch Review of attachment 8790424 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/linker/Zip.cpp @@ +188,5 @@ > already_AddRefed<Zip> > ZipCollection::GetZip(const char *path) > { > + { > + AutoLock lock(&sZipCollectionMutex); Hopefully, this doesn't cause deadlocks on fork... ::: mozglue/linker/Zip.h @@ +15,4 @@ > #include "mozilla/RefCounted.h" > #include "mozilla/RefPtr.h" > > +class AutoLock { Please put this in Utils.h instead.

Please find another reviewer for the Java code.

(that is, I don't know who a suitable reviewer is, I'm not)

Comment on attachment 8790424 [details] [diff] [review] 0003-Bug-1302189-3.1-Add-mutex-locking-to-ZipCollection-z.patch Moving patch 3 to bug 1302516.

To mimic the behavior of the custom ref counting [1], we need to keep the named zip reference around in ByteBufferInputStream to prevent it from getting gc'ed. kats, looks like you've been the original reviewer, can you take the reviews of these patches? [1] https://hg.mozilla.org/try/rev/acd6a97dab4c9325c31b9214782110aeeb9ed28f [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0218fd871b0c

Comment on attachment 8790420 [details] [diff] [review] 0001-Bug-1302189-1.1-Release-native-zip-only-on-finalize-.patch Review of attachment 8790420 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/NativeZip.java @@ +42,1 @@ > } I think it's better to just get rid of this function entirely and the two callsites of it in GeckoJarReader. We shouldn't have an API that misleads users into thinking it does something.

Comment on attachment 8790910 [details] [diff] [review] 0002-Bug-1302189-2.2-Keep-named-zip-referenced-in-its-chi.patch Review of attachment 8790910 [details] [diff] [review]: ----------------------------------------------------------------- Not sure I fully understand why this is needed. My impression is that this scenario occurs when: - Somebody creates a NativeZip - Then creates a ByteBufferInputStream to read a file from that zip - Then creates another NativeZip for the file being read (which is a nested zip) - Then creates another ByteBufferInputStream to read a file from the nested zip And somehow the root zip gets released. However, the nested ByteBufferInputStream holds a ref to the nested zip, and the nested zip holds a ref to the root ByteBufferInputStream (in NativeZip::mInput) which also holds a ref to the root zip. So if the NativeZip is now only released in finalize(), because of the part 1 patch, this chain should remain valid and nothing should get released until the nested ByteBufferInputStream is GC'd. Am I missing something?

Addressed comments.

Further inspection has revealed that ProGuard is optimizing out NatizeZip::mInput which breaks the reference chain to the root zip. Adding the @JNITarget annotation will prevent this and implicitly document that we need the reference to manage the native resource lifecycle. I think this is a clearer solution than adding this specific case to proguard.cfg, which usually holds general exclusion patterns otherwise.

Removed redundant import.

Comment on attachment 8791409 [details] [diff] [review] 0002-Bug-1302189-2.3-Prevent-ProGuard-from-optimizing-out.patch Review of attachment 8791409 [details] [diff] [review]: ----------------------------------------------------------------- Sure

Pushed by esawin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/df4352625311 [1.2] Release native zip only on finalize to prevent premature unmapping of referenced memory. r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/0e08d54ad9f6 [2.3] Prevent ProGuard from optimizing out native resource references. r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/48cca0d90707 [4.1] Check for null when retrieving input streams from GeckoJarReader. r=kats

I wish we would've not used @JNITarget, since it's not a JNI target. We could consider adding a new annotation like @NoStrip or something for stuff like this. As it is, if you wanted to look at JNI entry points via annotations, you'd have stuff like this showing up too.

https://hg.mozilla.org/mozilla-central/rev/df4352625311 https://hg.mozilla.org/mozilla-central/rev/0e08d54ad9f6 https://hg.mozilla.org/mozilla-central/rev/48cca0d90707

Comment on attachment 8791398 [details] [diff] [review] 0001-Bug-1302189-1.2-Release-native-zip-only-on-finalize-.patch Approval Request Comment [Feature/regressing bug #]: None [User impact if declined]: Intermittent crashes on startup [Describe test coverage new/current, TreeHerder]: Nightly, crashtests [Risks and why]: Low, it just prevents objects from being released too early [String/UUID change made/needed]: None

Comment on attachment 8791398 [details] [diff] [review] 0001-Bug-1302189-1.2-Release-native-zip-only-on-finalize-.patch Crash fix, Beta50+ (already on Aurora51)

Needs rebasing for Beta.

Beta rebase.

Beta rebase.

Beta rebase.

https://hg.mozilla.org/releases/mozilla-beta/rev/ea1cd458ad5e https://hg.mozilla.org/releases/mozilla-beta/rev/893bfa57607e https://hg.mozilla.org/releases/mozilla-beta/rev/c793b35169b7