Closed Bug 1265621 Opened 4 years ago Closed 4 years ago

Omnijar is opened twice on Android

Categories

(Core :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(3 files, 1 obsolete file)

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
Use StaticRefPtr instead of raw pointers in Omnijar.cpp for convenience
and safety.
Attachment #8742638 - Flags: review?(nfroyd)
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.
Attachment #8742639 - Flags: review?(nfroyd)
Attachment #8742638 - Flags: review?(nfroyd) → review+
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.
Attachment #8742639 - Flags: review?(nfroyd) → review+
(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.
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.
Attachment #8744974 - Flags: review?(aklotz)
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.
Attachment #8744974 - Flags: review?(aklotz) → review+
Attachment #8744974 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/499b7bf811b3
https://hg.mozilla.org/mozilla-central/rev/370e94b2ac05
https://hg.mozilla.org/mozilla-central/rev/c4aa1cda7cd7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.