1265621 - Omnijar is opened twice on Android

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

On Android, we open the omnijar twice, once through Omnijar::Init, and once through nsJAR::Open. We do have code in nsJAR::Open to reuse existing zip archive from Omnijar if possible [1], but Omnijar::GetReader doesn't support Android-style nested jars, so we end up not reusing the existing omnijar. The result is the omnijar being reopened and remapped, which is a concern given our trouble with virtual memory exhaustion (bug 1164027).

[1] https://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsJAR.cpp?rev=d53b301a14e1#139

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Use StaticRefPtr in Omnijar.cpp (v1)

Use StaticRefPtr instead of raw pointers in Omnijar.cpp for convenience
and safety.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Expose outer zip readers in Omnijar::GetReader (v1)

Previously, Omnijar::GetReader(nsIFile*) returned nullptr when using
nested jars. This patch makes it return the outer jar reader in case
of nested jars, so the caller can reuse the existing zip archive.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8742639 [details] [diff] [review]
Expose outer zip readers in Omnijar::GetReader (v1)

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

What is this "Android-style nested jars" of which you speak?

I believe this change makes sense, but the GetReader documentation now needs updating.  r=me with that change.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8742639 [details] [diff] [review]
> Expose outer zip readers in Omnijar::GetReader (v1)
> 
> Review of attachment 8742639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is this "Android-style nested jars" of which you speak?

On Android the omnijar is a jar file (omni.ja) within a jar file (the app apk file), so we have some special handling for that.

> I believe this change makes sense, but the GetReader documentation now needs
> updating.  r=me with that change.

Ok, I'll change the doc.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add flag to track omnijar handle in nsJAR (v1)

Add a bool flag in nsJAR to track whether the zip handle came from
omnijar.  The old code compared pointers instead, but other patches in
this bug made it no longer possible to do that.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8744974 [details] [diff] [review]
Add flag to track omnijar handle in nsJAR (v1)

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

r=me once corrections are made.

::: modules/libjar/nsJAR.cpp
@@ +214,5 @@
>    mTotalItemsInManifest = 0;
>  
> +  if (mOpenedOmnijar) {
> +    // Reset state, but don't close the omnijar because we did not open it.
> +    mOpenedOmnijar = false;

You still need the line:
mZip = new nsZipArchive();

in this if block, otherwise this nsJAR will continue to hold a reference to the cached omnijar nsZipArchive after Close() has been called.

::: modules/libjar/nsJAR.h
@@ +112,5 @@
>      mozilla::Mutex           mLock;
>      int64_t                  mMtime;
>      int32_t                  mTotalItemsInManifest;
>      bool                     mOpened;
> +    bool                     mOpenedOmnijar;

Please rename this to mIsOmnijar - I think that is a bit more clear.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/499b7bf811b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/370e94b2ac05
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4aa1cda7cd7

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add flag to track omnijar handle in nsJAR (v1.1)

Updated patch

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/499b7bf811b3
https://hg.mozilla.org/mozilla-central/rev/370e94b2ac05
https://hg.mozilla.org/mozilla-central/rev/c4aa1cda7cd7