346088 - Replace costly zip->GetEntry()->isSynthetic with check for '/' at end of name

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Alfred Kayser]

In nsSoftwareUpdateRun.cpp, it says that it is pretty inefficient, and truly it is so:
143         // This is pretty inefficient, but maybe that's okay.
144         nsCOMPtr<nsIZipEntry> entry;
145         rv = hZip->GetEntry(name.get(), getter_AddRefs(entry));
146         if (NS_FAILED(rv)) return rv;
147 
148         PRBool isSynthetic;
149         rv = entry->GetIsSynthetic(&isSynthetic);
150         if (NS_FAILED(rv)) return rv;
151         if (isSynthetic)
152             continue;

This is the only location where nsIZipEntry is used, and only to check if the entry if a (synthetic) directory. This code is very inefficient, and prevents the deletion (decom!) of nsIZipEntry.
Furthermore, not just synthetic entries should be skipped from signature validation, also 'real' directory entries, as they also don't have anything to create a signature on.

So, the above code can just be replaced with:
if (name.Last() == '/') continue;

With this fixed, we can easily remove nsIZipEntry because it is then only used within libjar itself...

Comment 1

[Alfred Kayser]

Attachment: V1: Remove the zip->GetEntry()->isSynthetic() check

Note, also remove some redundant include's.

Comment 2

[Daniel Veditz [:dveditz]]

Comment on attachment 230938 [details] [diff] [review]
V1: Remove the zip->GetEntry()->isSynthetic() check

Please try this patch with the testcases found at http://www.mozilla.org/projects/xpinstall/signed/testcases/

>+        if (PL_strncasecmp("META-INF/", name.get(), 9) == 0)
>             continue;
> 
>         // we only count the entries which are not in the meta-inf
>         // directory and which are explicitly listed in the zip
>         entryCount++;

As long as you're here it would make a lot more sense with
those two comment lines above the PL_strncasecmp("META-INF/"
Or perhaps leave them and add

  // Don't count any of the files in the META-INF/ dir itself

(or similar) to the appropriate place.

Comment 3

[Alfred Kayser]

Attachment: V2: Fixed the comments

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 231085 [details] [diff] [review]
V2: Fixed the comments

r/sr=dveditz

Comment 5

[Alfred Kayser]

Brendan, can you do the checkin for me?
Thanks in advance, Alfred

Comment 6

[Nickolay_Ponomarev]

This was forgotten; Alfred you probably know can put [checkin needed] in the status whiteboard to put a patch on the to be checked in radar.

Comment 7

[Brendan Eich [:brendan]]

Alfred, you should file a bug asking for CVS commit access.  I'll vouch, and I'm sure darin and others will sr+.

Sorry I missed this request to check in.  Can someone nominate the patch and carry it through checkin to trunk and (if approved) 1.8 branch?  I'm swamped right now, so I would appreciate help from another committer.

/be

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: as checked in

mozilla/xpinstall/src/nsSoftwareUpdateRun.cpp 	1.108

Comment 9

[mmc]

(In reply to comment #0)

> With this fixed, we can easily remove nsIZipEntry because it is then only used
> within libjar itself...
 
is there a follow-up bug on this?

Comment 10

[Alfred Kayser]

See bug 203627