269813 - getZip() returns open nsIZipReader

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Per Persson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; sv-SE; rv:1.7.5) Gecko/20041108 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; sv-SE; rv:1.7.5) Gecko/20041108 Firefox/1.0

The documentation for nsIZipReaderCache doesn't say that getZip() returns an
*open* nsIZipReader.

Calling open() for the already opened zipreader makes every entry appear twice
in the enumerator returned by findEntries and the file isn't released (unless
the zipreader is closed twice, I suppose).

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Martijn Wargers (dead)]

So I guess something has to be documented here:
http://lxr.mozilla.org/seamonkey/source/modules/libjar/nsIZipReader.idl#128

Comment 2

[Johnny Accot]

Could somebody add this quick "property" of nsIZipReader to the IDL file?  It took me a long time to understand why I got the entries twice!  Wouldn't it be better, when the Zip is already open, to just return successfully?  Something like:

RCS file: /cvsroot/mozilla/modules/libjar/nsJAR.cpp,v
retrieving revision 1.120
diff -u -r1.120 nsJAR.cpp
--- modules/libjar/nsJAR.cpp    29 Mar 2006 22:10:37 -0000      1.120
+++ modules/libjar/nsJAR.cpp    5 Apr 2006 15:39:19 -0000
@@ -213,6 +213,10 @@
 NS_IMETHODIMP
 nsJAR::Open()
 {
+  /* if already open, nothing to do */
+  if (mFd)
+    return NS_OK;
+
   nsresult rv;
   nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(mZipFile, &rv);
   if (NS_FAILED(rv)) return rv;

Comment 3

[Nickolay_Ponomarev]

Attachment: document the fact that getZip returns an open zipreader

Yes, the current behavior makes little sense, but we should document that the returned zip reader is already open, even if we plan to fix the double-open() issue.

Comment 4

[Nickolay_Ponomarev]

Attachment: throw NS_ERROR_ALREADY_INITIALIZED when someone tries to open the reader twice

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 217841 [details] [diff] [review]
document the fact that getZip returns an open zipreader

sr=bzbarsky, but could we maybe make this behave sanely?  What consumers of  this do we have in our code?  Where do we use the sharing?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 217842 [details] [diff] [review]
throw NS_ERROR_ALREADY_INITIALIZED when someone tries to open the reader twice

Is it better to do this, or to just be a no-op?

Comment 7

[Nickolay_Ponomarev]

(In reply to comment #5)
> (From update of attachment 217841 [details] [diff] [review] [edit])
> sr=bzbarsky, but could we maybe make this behave sanely?  What consumers of 
> this do we have in our code?  Where do we use the sharing?
> 
I think we use sharing only in nsJARChannel, nsXPTZipLoader, and nsChromeRegistry (rdf/chrome version). I think this should be treated as a "private" interface with  all consumers being careful about shared zipreaders. (Is there a way to enforce the "privateness" of the interface?)

> Is it better to do this, or to just be a no-op?
I made it throw for because I don't think it's useful to be able to open the reader twice, it is impossible currently, and the problem of opening a zipreader twice, closing it once, then trying to use it will be easier to catch. Did I miss something and it's better to make this return NS_OK?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> (Is there a way to enforce the "privateness" of the interface?)

Make it noscript?

That said, people are clearly using it; which part do you feel should be private?

> and the problem of opening a zipreader twice, closing it once, then trying to
> use it will be easier to catch.

Ah, good point.  Ok, that makes the throw solution better.   ;)

Comment 9

[Darin Fisher]

I think we should fix nsIZipReaderCache to return some proxy object that does the right thing when open or close are called.  It should not be possible to call getZip and get a closed nsIZipReader instance.

Comment 10

[Nickolay_Ponomarev]

(In reply to comment #8)
> That said, people are clearly using it; which part do you feel should be
> private?

Oops, I didn't think of the people who use it already. It's just that the whole nsIZipReaderCache thing seemed to me an optimization of our chrome-loading mechanism (JAR channels in particular). Is it really useful as a part of the platform? I don't think so.

This is also the reason I don't think additional complexity should be added to this code. If you feel this should be a robust public component, I can file a separate bug for Darin's suggestion in comment 9.

Comment 11

[Alfred Kayser]

The whole jar nsZipReaderCache/jarCache thing is not really working optimally.
nsChromeRegistry creates its own instance just to test for existance of entries (using 'GetZip / GetEntry') nsJARChannel uses its own jarCache to cache all channel requests for a jar entry or for a directory entry.
This results in two separate caches.

See also bug 333202 where there is also some suspicion on the working of the cache.

Why not define a new servie (say 'JarStreamFactory') which can be queried with: 'ContainsEntry(jarname, entryname)', 'GetJarInputStream(jarname, entryname)', 'GetJarDirStream(jarname, entryname)'.
This allows that service to manage a cache of 'ZipReader' at will (on demand, as we say it... ;-)), without exposing these 'internal) ZipReaders to others to misuse. It also allows for more flexible cache mechanism's, for example to use a timer to close any zipreader not used for more than a certain period (see nsRecyclingAllocator...)

Bij keeping the existing nsIZipReader, one can still directly manipulate zipfile directly.

This is all more work and more impact than this bug intended, so possibly something for a new bug, and close this one as 'fixed' with the extra comment and check?

Comment 12

[Darin Fisher]

Alfred: I like your suggestion.  If you'd be willing to help spearhead that (after your current libjar patch lands), that'd be awesome.

Comment 13

[Alfred Kayser]

Two more notes:
1. Currently the libjar code assumes that the ZipReader remains open as long as the JarInputStream is open. So closing the first before the second is finished WILL crash! See bug 214672!

2. The new setup will also allow to forcely clear the jarcache (whilst allowing the streams to continue, see above) in case of memory shortage or in case of switching themes or so (as long as the old file remains existing, renamed, then reading will not fail).

Comment 14

[Darin Fisher]

Comment on attachment 217842 [details] [diff] [review]
throw NS_ERROR_ALREADY_INITIALIZED when someone tries to open the reader twice

NS_ENSURE_TRUE(!mFD, NS_ERROR_ALREADY_INITIALIZED) would be even better.

Comment 15

[Nickolay_Ponomarev]

Attachment: use NS_ENSURE_TRUE

Right. Carrying reviews over.

Comment 16

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

Both attachment 217841 [details] [diff] [review] and attachment 218064 [details] [diff] [review] need to be checked in, right?

Comment 17

[Nickolay_Ponomarev]

> Both attachment 217841 [details] [diff] [review] [edit] and attachment 218064 [details] [diff] [review] [edit] need to be checked in, right?

Yes, please.

Comment 18

[Asaf Romano (gone)]

Checking in nsIZipReader.idl;
/cvsroot/mozilla/modules/libjar/nsIZipReader.idl,v  <--  nsIZipReader.idl
new revision: 1.19; previous revision: 1.18
done

Checking in modules/libjar/nsJAR.cpp;
/cvsroot/mozilla/modules/libjar/nsJAR.cpp,v  <--  nsJAR.cpp
new revision: 1.121; previous revision: 1.120
done