203627 - Make nsIZipEntry redundant, and remove it.

Status: RESOLVED INVALID

(Core :: Networking: JAR, defect)

Description

[Alfred Kayser]

As a followup of bug 203344, this bug is about removing nsIZipEntry.
It is (after bug 203344) only used in :
http://lxr.mozilla.org/seamonkey/source/rdf/chrome/src/nsChromeRegistry.cpp#822
and only to test whether the entry exists in the jar/zipfile.

So may be in ZipReader interface replace getEntry with existsEntry, to
just return a bool, to indicate existance?

Just like 203344, this saves an object creation and immediate deletion, just
for a simple value retrieval.

Comment 1

[Alfred Kayser]

*** Bug 203626 has been marked as a duplicate of this bug. ***

Comment 2

[Darin Fisher]

sounds good to me.  alecf: what do you say?

Comment 3

[Alec Flett]

this sounds great!

Comment 4

[Alfred Kayser]

Ok, a summary of things to do:

nsIZipReader.idl:
1. change nsISimpleEnumerator/*<nsIZipEntry>*/ findEntries(in string aPattern);
   to return an enumeration of strings (just the entryname).
2. change getEntry to containsEntry returning a boolean
3. remove nsIZipEntry

Ad 1. Fix callers of findEntries:
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsInstall.cpp#2741
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsSoftwareUpdateRun.cpp#105
http://lxr.mozilla.org/seamonkey/source/modules/libjar/nsXPTZipLoader.cpp#68
http://lxr.mozilla.org/seamonkey/source/modules/libjar/nsJAR.cpp#507
http://lxr.mozilla.org/seamonkey/source/modules/libjar/nsJAR.cpp#542
All five use the ZipEntry returned by findEntries to get the name of the entry,
so patch is simple, and even addressing performance/footprint.
Cleanup of FindEntries, etc in nsJAR/nsZipArchive.

Ad 2. Fix callers of GetEntry
http://lxr.mozilla.org/seamonkey/source/rdf/chrome/src/nsChromeRegistry.cpp#823
(and bug 203344 of course)
nsJAR.cpp: Change GetEntry to containsEntry...

Ad 3. Remove the nsIZipEntry from idl (and .cpp/.h)

Comment 5

[Alec Flett]

note that there is now nsIStringEnumerator if you need it..

Comment 6

[Alfred Kayser]

Which looks perfect for this situation. 
However I'm not to sure with (A)(C)Strings yet...

Comment 7

[Alec Flett]

see http://www.mozilla.org/projects/xpcom/string-guide.html for starters

Comment 8

[Alfred Kayser]

Attachment: Patch for modules\libjar

Removing zipEntry from IDL, replacing GetEntry with ContainsEntry, etc.

Some test info:
		 New	     Old
    jar50.dll	94.268	    98.364
    jar.xpt	   814	       941
Savings: about 4K of dllsize.	 

Timings(5 starts):  3.95     3.96, so measureable difference

Comment 9

[Alfred Kayser]

Attachment: Patch for rdf\chrome\src

Second part of patch, to be applied in rdf\chrome\src.

Comment 10

[Darin Fisher]

dveditz: thought this bug might interest you...

Comment 11

[Alfred Kayser]

Attachment: Patch for xpinstall\src

Third part: for xpinstall.
Changes:
 nsInstall.cpp: 
    replaced ExtractDirEntries with UTF8StringEnumerator
    removed DeleteVector (redundant now)
 nsSoftwareUpdateRun.cpp:
    replaced SimpleEnum with ZipEntry's to UTF8StringEnumerator

Comment 12

[Alec Flett]

this is great stuff. I look forward to reviewing it later... :)

Comment 13

[Alfred Kayser]

Further investigation showed that Open and Close are also obsolete. 
Open is always called directly after Init, so Init and Open can be
combined into one function. Close is nowhere used, except
in the destructur of nsJAR itself. 
So, IMHO Open and Close can be removed from the nsIZipReader.idl.

Init/Open:
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#2307
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsSoftwareUpdateRun.cpp#239

Other issues:
** is the nsIZipReader.idl include is still needed?
http://lxr.mozilla.org/seamonkey/source/caps/idl/nsICertificatePrincipal.idl#39
 39 /* Defines an interface for certificate principals. */
 40 #include "nsIPrincipal.idl"
 41 #include "nsIZipReader.idl"

** Here is a cache created for 32 entries, but only used to get one zip?
http://lxr.mozilla.org/seamonkey/source/rdf/chrome/src/nsChromeRegistry.cpp#873
873   //
874   // also cache the zip file itself
875   //
876   nsCOMPtr<nsIZipReaderCache> readerCache =
877     do_CreateInstance("@mozilla.org/libjar/zip-reader-cache;1", &rv);
878   if (NS_FAILED(rv)) return rv;
879 
880   rv = readerCache->Init(32);
881   
882   rv = readerCache->GetZip(overrideFile, getter_AddRefs(mOverrideJAR));

So, do I create separate bugs for these issues?

Comment 14

[Darin Fisher]

alfred: i don't have much opinion on the matter of separate bugs.  i'm fine if
you want to just add patches here for those other changes.

Comment 15

[Alfred Kayser]

I've got a patch to cleanup the nsIZipReader, as discussed, including the
ZipEntry removal, the redundant Open/Close, and lots of small fixes in libjar.
The patch is probably a bit bitrotted, as the tree progresses.
I'll upload a clean patch somewhere next week, depending on connectivity issues.

Comment 16

[Alfred Kayser]

Attachment: modules\libjar: Removal of ZipEntry, and corresponding cleanup

This is complete, but needs some more testing, and
I will provide some more documentation on the changes.

Comment 17

[Alfred Kayser]

Attachment: Updated patch for xpinstall\src

This also removes the redundant 'Open', and
uses nsnull as argument for FindEntries (which is same but faster than '*').

Comment 18

[Alfred Kayser]

Attachment: Updated patch for modules\libjar

+----------------------------------------------------------------------------
| Summary of changes
+----------------------------------------------------------------------------
1: Removal of ZipEntry from IDL, including corresponding nsJarItem and
nsZipItem
2: Removal of Open/Close from nsZipReader.idl: merged with Init and Destructor
3: Change errorcodes to nsresult 
4: FindEntries: use nsIUTF8StringEnumerator for finds, returning names.
5: ZipItem changed to struct (saves class overhead)
6: In xpinstall use 'nsnull' as find pattern instead of '"*"' (same but
faster):
   
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsSoftwareUpdateRun.cpp#134

    FindEntries("*", ...)
7: nsJar.cpp: Only allocate and init ObjectHashtable when needed:
    Made mManifestData a pointer to the ObjectHashtable, and allocate
    only when really needed. Reduces size of nsJAR from 1164 to 1104
    (saves 60 bytes per JAR), excluding the extra memory allocated by 
    ObjectHashtable itself.
    Also remove mParsedManifest (mManifestData indicates parsedManifest status)

8: nsJar.cpp: VerifyEntry: Call LoadEntry only when really needed.
9: class nsJARManifestItem has three (3) flags:
    Changed Enum into PRInt8 (to save bits), and grouped with other flags
    (sizeof nsJARManifestItem reduced from 24 to 20).
10: ZipReader: replace GetEntry with ContainsEntry: returns boolean instead of
ZipEntry
11: Removal of redundant nsJarItem

+----------------------------------------------------------------------------
| Footprint data
+----------------------------------------------------------------------------
- Structure/class size changes
	nsJar: 1168 to 1104
	nsJarEnumerator: 17 to 8
	nsJarManifestDataItem: 24 to 20
	nsJarItem: gone
- Mallocs: 
	FindEntries: returns names instead of ZipItem structures
	Only allocate ObjectHashtable when really needed
	VerifyEntries: only load (and allocate buffer) when really needed
	BuildFileList: nsZipItem allocate together with name (replace 2 mallocs
with 1 malloc in STANDALONE)
- Codesize (debug) jar50.dll: from 98.364 to 86.076 bytes (.dll filesize)      
 
	jar50_s.lib: from 57.624 to 49.062 
	(total about 12% reduction)

TODO:
** is the destructor of ManifestItem really called?
nsJARManifestItem::~nsJARManifestItem()
{
  // Delete digests if necessary
  PR_FREEIF(calculatedSectionDigest);
  PR_FREEIF(storedEntryDigest);
}
PR_STATIC_CALLBACK(PRBool)
DeleteManifestEntry(nsHashKey* aKey, void* aData, void* closure)
{
//-- deletes an entry in  mManifestData.
  PR_FREEIF(aData);
  return PR_TRUE;
}
Does the 'free' in DeleteManifestEntry calls the destructor??



+----------------------------------------------------------------------------
| Detailed description of changes (see patch.txt)
+----------------------------------------------------------------------------
Changes:
nsIZipReader.idl: 
* removal of ZipEntry
* ZipReader: removal of 'file' attribute
* ZipReader: removal of 'open' and 'close' members
* ZipReader: replace GetEntry with ContainsEntry: returns boolean instead of
ZipEntry
* ZipReader: attribute name consistency: aEntryName
* ZipReader: FindEntries: returns nsIUTF8StringEnumerator

nsJar.h:
* add nsJar:GetJarPath
* remove nsJarItem (=ZipEntry)
* nsJarEnumerator implements nsIUTF8StringEnumerator
* nsJarEnumerator: removal of mArchive and mIsCurrStale (size: 17 -> 8)

nsJar.cpp:
* removal of ziperr2nsresult
* removal of JAR_NULLFREE
* nsJAR: new member 'GetJarPath'
* nsJAR: replace GetEntry with ContainsEntry
* nsJAR: FindEntries: change to use nsIUTF8StringEnumerator
* replace .Length()==0 with .IsEmpty
* nsJAREnumerator: return nsIUTF8StringEnumerator
* nsJAREnumerator: remove mIsCurrStale (mCurr==NULL indicates 'stale')
* Replace archive->FindFree(mFind) with delete mFind
* removal of nsJarItem (ZipEntry)

nsInputStream.cpp:
* use nsresult errorcodes instead of ZIP_...

nsXPTZipLoader.cpp:
* use the new FindEntries with the nsIUTF8StringEnumerator

nsZipArchive.cpp:
* make ExtractMode and IsSymLink only for XP_UNIX
* change PRInt32 return codes to nsresult
* Zip_FindFree does just 'delete find' insteadof
'find->GetArchive()->FindFree(find)'
* ::Test: simplified
* ::GetItem return item as returnvalue directly
* consistency: attribute ZipEntry to aEntryName
* nsZipFind add member FindNext, to allow hiding of mArchive
* nsZipArchive::FindNext: also update mSlot and mItem when nothing found, so
that next find request
will not search whole table again.
* removal of ::FindFree, just use 'delete'
* BuildFileList: 
    - general cleanup, 
    - nsZipItem allocate together with name (replace 2 mallocs with 1 malloc in
STANDALONE)
    - malloc nsZipItem instead of new (new sets to zero, and other overhead).
    - extract 'external_attributes' only for XP_UNIX
    - removal of the 'debug' code (errors)
* ::TestItem: removal of 'goto' and 'bInflating'
* removal of nsZipItem
* removal of nsZipFind::GetArchive

nsZipArchive.h:
* nsZipItem changed to struct, to remove class overhead
* nsZipItem: name as last member, to alloc name buffer together with nsZipItem
* nsZipItem: 'mode' only for XP_UNIX
* nsZipFind: removal of FindFree, add of FindNext
* nsZipFind: move mSlot after mItem, for better byte alignment
* nsZipArchive::FindNext returns entryName instead of nsZipItem

zipfile.h:
* use nsresult codes

Comment 19

[Darin Fisher]

sorry, i haven't had a chance to look over this patch yet... it may take me
another week or so to get to this patch :-(

Comment 20

[Alfred Kayser]

Attachment: patch for /netwerk/protocl/jar/src

Required patch for the IDL change: 
* remove calls to Open, Close (not needed anymore)

Comment 21

[Darin Fisher]

applying the patch from attachment 128162 [details] [diff] [review], i get a number of conflicts:

7 out of 25 hunks FAILED -- saving rejects to file nsJAR.cpp.rej
1 out of 6 hunks FAILED -- saving rejects to file nsJAR.h.rej
2 out of 42 hunks FAILED -- saving rejects to file nsZipArchive.cpp.rej
1 out of 16 hunks FAILED -- saving rejects to file nsZipArchive.h.rej

my appologies for not getting to this patch sooner... i tried digging in to fix
up the conflicts myself, but it looks like (at least some of) the conflicts are
actually with your previous patch for bug 201226 (or maybe i'm mistaken).  at
any rate, if you could update the patches (maybe put everything in one diff),
then i will review them promptly.

Comment 22

[Alfred Kayser]

Comment on attachment 122105 [details] [diff] [review]
Patch for rdf\chrome\src

See bug 214672 for the integrated patch

Comment 23

[Alfred Kayser]

Comment on attachment 128159 [details] [diff] [review]
Updated patch for xpinstall\src

See bug 214672 for the integrated patch

Comment 24

[Alfred Kayser]

Comment on attachment 128162 [details] [diff] [review]
Updated patch for modules\libjar

See bug 214672 for the integrated patch

Comment 25

[Alfred Kayser]

Comment on attachment 132044 [details] [diff] [review]
patch for /netwerk/protocl/jar/src

See bug 214672 for the integrated patch

Comment 26

[Alfred Kayser]

With bug 214672 fixed, this one becomes slightly simpler to solve, however JARInputDirectoryStream introduces a new user of nsZipEntry, which can be fixed by making that one calling directly nsZipArchive/nsZipItem.
One other caller is for checking 'IsSynthetic' in jarfile validation, but that should be checking for 'isDirectory' anyway (there is no way that a directory can be validated against a signature ;-) ).

nsIZipEntry users are:
/modules/libjar/nsJARDirectoryInputStream.cpp, line 98 -- nsCOMPtr<nsIZipEntry> ze;
/modules/libjar/nsJARDirectoryInputStream.cpp, line 208 -- nsCOMPtr<nsIZipEntry> ze;
/xpinstall/src/nsSoftwareUpdateRun.cpp, line 146 -- nsCOMPtr<nsIZipEntry> entry;
/xulrunner/setup/nsXULAppInstall.js, line 48 -- const nsIZipEntry = Components.interfaces.nsIZipEntry;
/xulrunner/setup/nsXULAppInstall.js, line 172 -- var entry = entries.getNext().QueryInterface(nsIZipEntry);

Comment 27

[Alfred Kayser]

bug 337561 makes it that nsJARDirectoryInputStream no longer needs nsIZipEntry.
So that only rdf/chrome is the last user of nsIZipEntry, to check for IsSynthetic, to prevent jarfile validation on synthetic entries, but actually all directory entries should not be checked for valid signature (as a directory as no data to base the signature on!), so a check for '/' at the end of the entryname is sufficient, instead of doing GetEntry->IsSynthethic (which is very costly).

Comment 28

[Alfred Kayser]

Attachment: Completely remove nsIZipEntry

Simple patch to remove nsIZipEntry.
Note, this also removes nsJARDirectoryInputStream.h (leftover from bug 337561).

Comment 29

[Darin Fisher]

Comment on attachment 242479 [details] [diff] [review]
Completely remove nsIZipEntry

ok, r=darin

Comment 30

[Alfred Kayser]

Christian Biesinger, any chance of sr'ing this patch?

Comment 31

[Reed Loden [:reed]]

Comment on attachment 242479 [details] [diff] [review]
Completely remove nsIZipEntry

Bug 379633 brought code that actually uses this back into existence, so this isn't going to work.

Comment 32

[Alfred Kayser]

Marking this one as 'INVALID' (see comment #31).

Created bug 412818 to cvs remove nsJARDirectoryInputStream.h