Now that we have mmemory mapped jar files(see bug 504864), it is possible to do less memcpy's for compressed jars. Attached patch removed 'ReadBuf' as it is (because of the MMAPing of bug 504864) no longer needed to 'read' data into a 'readbuffer'). This saves memory usage and copying all data. Also elements 'dataOffset' and 'hasDataOffset' are no longer needed, as they are now determined by the GetItemData method of nsZipHandle directly. This saves 4 bytes for every jar item.

Comment on attachment 394779 [details] [diff] [review] V1: Remove readbuf and its simulated 'read' for compressed jar IO Thank you for this nice patch, it almost gets jar io stuff into acceptable state. Found a few minor issues with it. r? me on the next one and we'll sr bsmedberg. I think the only major thing remaining after this is cleaning up the directory support in nsjarinputstream(it's a mess). > /*------------------------------------------------------------------------- > * Class nsJARInputStream declaration. This class defines the type of the > * object returned by calls to nsJAR::GetInputStream(filename) for the > * purpose of reading a file item out of a JAR file. > *------------------------------------------------------------------------*/ > class nsJARInputStream : public nsIInputStream > { > public: >- nsJARInputStream() : >- mInSize(0), mCurPos(0), mInflate(nsnull), mDirectory(0), mClosed(PR_FALSE) >- { } >- >- ~nsJARInputStream() { >- Close(); >- } >+ nsJARInputStream() : mClosed(PR_TRUE) { } Please put all of the static initializers back. It's fragile to completely reply on things getting correctly initialized elsewhere. >+ ~nsJARInputStream() { Close(); } > > NS_DECL_ISUPPORTS > NS_DECL_NSIINPUTSTREAM > > // takes ownership of |fd|, even on failure > nsresult InitFile(nsZipHandle *aFd, nsZipItem *item); > > nsresult InitDirectory(nsJAR *aJar, > const nsACString& aJarDirSpec, > const char* aDir); > > private: >- PRUint32 mInSize; // Size in original file > PRUint32 mCurPos; // Current position in input >- >- struct InflateStruct { >- PRUint32 mOutSize; // inflated size >- PRUint32 mInCrc; // CRC as provided by the zipentry >- PRUint32 mOutCrc; // CRC as calculated by me >- z_stream mZs; // zip data structure >- unsigned char mReadBuf[ZIP_BUFLEN]; // Readbuffer to inflate from >- }; >- struct InflateStruct * mInflate; >+ PRUint32 mOutSize; // inflated size >+ PRUint32 mInCrc; // CRC as provided by the zipentry >+ PRUint32 mOutCrc; // CRC as calculated by me >+ z_stream mZs; // zip data structure > > /* For directory reading */ > nsRefPtr<nsJAR> mJar; // string reference to zipreader > PRUint32 mNameLen; // length of dirname >- nsCAutoString mBuffer; // storage for generated text of stream >+ nsCString mBuffer; // storage for generated text of stream > PRUint32 mArrPos; // current position within mArray > nsTArray<nsCString> mArray; // array of names in (zip) directory >- PRPackedBool mDirectory; // is this a directory? >+ PRPackedBool mCompressed; // is this compressed? >+ PRPackedBool mDirectory; // is this a directory? > PRPackedBool mClosed; // Whether the stream is closed lets change these PRBools to "bool" types and move them to bottom. Same with nsZipItem's prbools >@@ -637,24 +624,22 @@ nsresult nsZipArchive::BuildFileList() > if (namelen > BR_BUF_SIZE || extralen > BR_BUF_SIZE || commentlen > 2*BR_BUF_SIZE) > return ZIP_ERR_CORRUPT; > > nsZipItem* item = CreateZipItem(namelen); > if (!item) > return ZIP_ERR_MEMORY; > > item->headerOffset = xtolong(central->localhdr_offset); >- item->dataOffset = 0; > item->size = xtolong(central->size); > item->realsize = xtolong(central->orglen); > item->crc32 = xtolong(central->crc32); > item->time = xtoint(central->time); > item->date = xtoint(central->date); > item->isSynthetic = PR_FALSE; >- item->hasDataOffset = PR_FALSE; > > PRUint16 compression = xtoint(central->method); > item->compression = (compression < UNSUPPORTED) ? (PRUint8)compression > : UNSUPPORTED; > > item->mode = ExtractMode(central->external_attributes); I think we should make zipcentral a member of nsZipLocal and add accessor methods to do xtoint/etc ondemand. That should cut down some more on memory usage and will help with overhead of building up file lists for big jars(it's measurable on mobile). But this can be done in a separate bug. > > nsZipHandle* nsZipArchive::GetFD(nsZipItem* aItem) > { >- if (!mFd || !MaybeReadItem(aItem)) >+ if (!mFd) > return NULL; > return mFd.get(); > } Should get rid of this. Also get rid of mFD in nsJARInputStream, holding on to nsJAR is good enough(we already have to do that for directories). I guess with that you also don't need any more refcounting in the file descriptor emulation either. > > //--------------------------------------------- >-// nsZipArchive::MaybeReadItem >+// nsZipHandle::GetItemData > //--------------------------------------------- >-bool nsZipArchive::MaybeReadItem(nsZipItem* aItem) >+PRUint8* nsZipHandle::GetItemData(nsZipItem* aItem) I see where you are going with this, but I think this should be nsZipItem::GetData(). I'm also not sure it is best to stop caching dataOffset. We certainly get rid of hasDataOffset and use dataOffset == NULL to see if we have enough info. Reason I'm concerned about this is that calling getitemdata may cause extra stuff to be paged in.

New patch with the following changes: 1. Fixed/reverted the static initialization of nsJarInputStream. 2. Replaced PRPackedBools and PRBools in classes with 'bool'. 3. Got rid of GetFD() 4. Renamed GetItemData to GetData. 5. Removed all nsZipHandle, saving another malloc, and reducing code. 6. I have kept the 'lazy initialisation' of DataOffset. reason: GetData is only called directly before the actual data itself is read, and reading the ZipLocal is just a few bytes before the real data, so paging that part in is not that bad. Also GetData is really only once per item that is decoded/copied, so caching the result won't benefit.

Note, the xtoint, etc of the zipcentral fields on demands is indeed something to be addressed in a separate bug. I would also like to not copy the entryNames, but nsWildcard unfortunately requires zero-terminated strings and the name in the zipcentral record isn't zero-terminated.

(In reply to comment #2) > 5. Removed all nsZipHandle, saving another malloc, and reducing code. Sorry, but this isn't that easy. Currently we need the ziphandle to able able to access the jar entries AFTER the jar is closed to stay compatible with old behavior. Ie if you get a JARInputStream, call nsJAR.Close(), you can continue reading from JARInputStream::Read

I added a testcase with mmap bug that tests this behavior

Grr..., so, I need to resurrect nsZipHandle? P.s., the same problem is also true when reading directories? One could init a nsJARInputStream to a directory listing, and during the reading close nsJar...

(In reply to comment #6) > Grr..., so, I need to resurrect nsZipHandle? > > P.s., the same problem is also true when reading directories? > One could init a nsJARInputStream to a directory listing, and during the > reading close nsJar... yup, except in old code you'd end up referencing free()ed memory :) So it's not a backwards compatibility issue. My plan for directories is to malloc a big buffer in InitDirectory() and not keep any references around. It would let us get rid of all of the stupid little nsJARInputStream members that are only used for directories.

Comment on attachment 395385 [details] [diff] [review] V3: resurrect nsZipHandle Hopefully this is the last r-, these are pretty minor issues. Just fix these and i'll r+ the next patch. Also can you rebase this patch on top of bug 510247, since that's probably going to land first. >- // note that sometimes, we will close mFd before we've finished >- // deflating - this is because zlib buffers the input >- // So, don't free the ReadBuf/InflateStruct yet. >- // It is ok to close the fd multiple times (also in nsJARInputStream::Close()) >- if (mCurPos >= mInSize) { >- mFd.Close(); >- } >+ PRUint32 count = PR_MIN(aCount, mOutSize - mCurPos); >+ memcpy(aBuffer, mZs.next_in + mCurPos, count); >+ mCurPos += count; >+ *aBytesRead = count; Should preserve old behavior here too. Set mFd = null, and then if (!mClosed && !mFD) *bytesRead = 0; >- > nsZipHandle::~nsZipHandle() > { >+ if (mFileData) { >+ PR_MemUnmap(mFileData, mLen); >+ mFileData = nsnull; >+ } >+ if (mMap) { >+ PR_CloseFileMap(mMap); >+ mMap = nsnull; >+ } This is overly defensive. I'm not sure why you need to break up the two. Can always assert (!mFileData && !mMap)|| ... > if (mFd) { >- PR_MemUnmap(mFileData, mLen); >- PR_CloseFileMap(mMap); > PR_Close(mFd); > mFd = 0; should fix that to be NULL; >+ memset(mFiles, 0, sizeof mFiles); use () in sizeof() >+ mBuiltSynthetics = false; > return ZIP_OK; >- PRPackedBool hasDataOffset : 1; >- PRPackedBool isDirectory : 1; >- PRPackedBool isSynthetic : 1; /* whether item is an actual zip entry or was >+ bool isDirectory : 1; >+ bool isSynthetic : 1; /* whether item is an actual zip entry or was > generated as part of a real entry's path, > e.g. foo/ in a zip containing only foo/a.txt > and no foo/ entry is synthetic */ > #if defined(XP_UNIX) || defined(XP_BEOS) >- PRPackedBool isSymlink : 1; >+ bool isSymlink : 1; > #endif don't use bitfields for bools.

Comment on attachment 396967 [details] [diff] [review] V5: Addressed comments of Taras and rebased after bug 510247 >- rv = jis->InitFile(mZip.GetFD(item), item); >+ rv = jis->InitFile(this, item); >diff --git a/modules/libjar/nsJARInputStream.cpp b/modules/libjar/nsJARInputStream.cpp > > NS_IMETHODIMP > nsJARInputStream::Read(char* aBuffer, PRUint32 aCount, PRUint32 *aBytesRead) > { > NS_ENSURE_ARG_POINTER(aBuffer); > NS_ENSURE_ARG_POINTER(aBytesRead); > > *aBytesRead = 0; This is a bug. We should only write outparams when returning NS_OK. >- *aBytesRead = (mInflate->mZs.total_out - oldTotalOut); >+ *aBytesRead = (mZs.total_out - oldTotalOut); Same sort of issue here. >+ nsRefPtr<nsZipHandle> mFd; // handle for reading >+ PRUint32 mCurPos; // Current position in input >+ PRUint32 mOutSize; // inflated size >+ PRUint32 mInCrc; // CRC as provided by the zipentry >+ PRUint32 mOutCrc; // CRC as calculated by me >+ z_stream mZs; // zip data structure >+ >+ bool mCompressed; // is this compressed? >+ bool mDirectory; // is this a directory? >+ bool mClosed; // Whether the stream is closed the //s should align. > PRUint32 size; /* size in original file */ > PRUint32 realsize; /* inflated size */ > PRUint32 crc32; > > /* > * Keep small items together, to avoid overhead. > */ > PRUint16 time; > PRUint16 date; > PRUint16 mode; > PRUint8 compression; >- PRPackedBool hasDataOffset : 1; >- PRPackedBool isDirectory : 1; >- PRPackedBool isSynthetic : 1; /* whether item is an actual zip entry or was >- generated as part of a real entry's path, >- e.g. foo/ in a zip containing only foo/a.txt >- and no foo/ entry is synthetic */ >+ bool isDirectory; >+ bool isSynthetic; /* whether item is an actual zip entry or was >+ generated as part of a real entry's path */ > #if defined(XP_UNIX) || defined(XP_BEOS) >- PRPackedBool isSymlink : 1; >+ bool isSymlink; > #endif > > char name[1]; // actually, bigger than 1 > }; align comments above, maybe make them use same /**/ style instead of mixing in //? These are pretty minor, so r+ with outparam + comment issues corrected

i'm also going to give this a run on the try server.

Note, I didn't change the '*aBytesRead' thing as: 1. it changes the behavior of the interface (even if it is 'wrong'). 2. Firefox crashes when this is changed, so someone/somewhere one is expected aBytesRead to be changed, even in 'error' situations.

(In reply to comment #13) > Created an attachment (id=397267) [details] > V6: Addressed final nits of Tarak > > Note, I didn't change the '*aBytesRead' thing as: > 1. it changes the behavior of the interface (even if it is 'wrong'). > 2. Firefox crashes when this is changed, so someone/somewhere one is expected > aBytesRead to be changed, even in 'error' situations. It's Taras, not Tarak. I checked with valgrind, and you are right, a bunch of code screws up. Also you only need Benjamin's review if you want an extra pair of eyes on the code, my review is enough to land.

Comment on attachment 397267 [details] [diff] [review] V6: Addressed final nits of Taras Sorry about your name, Taras. And let's land this as soon as possible.

(In reply to comment #13) > Created an attachment (id=397267) [details] > V6: Addressed final nits of Tarak > > Note, I didn't change the '*aBytesRead' thing as: > 1. it changes the behavior of the interface (even if it is 'wrong'). > 2. Firefox crashes when this is changed, so someone/somewhere one is expected > aBytesRead to be changed, even in 'error' situations. Is this a de-facto standard, or should we try to fix the callers who check *aBytesRead even on error? Or possibly both? If either, please file bugs. /be

It is 'de-facto' as this behaviour (of setting aBytesCount even in case of an error) was there already for a long time. It would be good to seek any misbehaving callers that use bytesCount even on error. Created bug 513589 for this. Meanwhile, let's get this checked in without changing the current behavior.

85769 INFO Running /tests/modules/libjar/test/mochitest/test_bug403331.html... 85770 ERROR TEST-UNEXPECTED-FAIL | /tests/modules/libjar/test/mochitest/test_bug403331.html | Test timed out. Backing out...

Backed out http://hg.mozilla.org/mozilla-central/rev/ac90123f11e9.

Mm, that's strange. This test passed in both my opt and debug builds without any problems. Could this test failure be caused by one of the other bug patches that went in around the same time? bug 513999: mochitest-plain hanging during shutdown bug 512327: Update liboggz (merge for backout bug 513999) bug 488270: New APIs for precise time measurement of net requests. bug 513342: crash while browsing to and from a geolocation page The bug of the failed testcase is about redirecting channels which is not related to the actual zip reading itself...

Especially bug 513999 with the quote "Starting at 2009/08/30 19:12:43 the mochitest-plain suite is exhibiting a new kind of "random" failure"... Bug 488270 intercepts nsHTTPTtransaction, and could also be a trigger for this test failure.

It seems that bug 506038 is related or has the same problem?

Taras said this passed that same test on the try-server, as well as locally. I think we should re-land this on a quiet tree. I'll poke someone on Euro-time to do that.

i can try to push this again tomorrow morning Europe time, and look at the tree. Btw, notice it happened quite often that tests were passing locally and on Try-servers, but not on production tinderboxes. Still, i hope this is not the case.

pushed right now. http://hg.mozilla.org/mozilla-central/rev/3cab7a0c2c3d

Sorry the test timed out again, so it's most likely this patch is the cause. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1251975887.1251978183.701.gz WINNT 5.2 mozilla-central test mochitests on 2009/09/03 04:04:47 85592 ERROR TEST-UNEXPECTED-FAIL | /tests/modules/libjar/test/mochitest/test_bug403331.html | Test timed out. this failed only on Win. i backed out. http://hg.mozilla.org/mozilla-central/rev/b1ccd18e73dd

Marco, thanks for trying it again. My apologies for the backout... For all: How can we solve a 'intermittent' problem that only happens in a production server, but not in a tryserver or local build? The only thing that I can see is that JARInputStream may be failing in some obscure ways, possibly some race condition or so? The item under test is about reading a compressed entry from the test zipfile, which is probably where we need to start looking. I can try to make the code more robust, but there is seemingly no way to ensure that this failure will be fixed.

btw, the patch probably hit nightlies, and some user experimented this crash that sounds related to JARChannel http://crash-stats.mozilla.com/report/index/bp-7be186fd-e1f8-411a-8162-36a172090903 dunno if related, but sounds like it is.

or it's just bug 270042, and completely unrelated...

There is something definitive sensitive about streams and jarchannels which seems to be triggered in some builds/configs somehow.

Suggest valgrinding -- intermittent problems/non-determinism => deterministic (heap-ordering exploit) memory safety bug is a nasty progression we've seen over and over. /be

I don't have valgrind, but there is one thing that is interesting, which can cause this intermittent behavior. It seems that users of nsIInputStream not always check error codes of Read() and Available(), but just use the out param always: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#4700 4700 stream->Available((PRUint32 *) &contentLength); http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.cpp#177 This seems also related to bug 270042 and the crash report, where nsJARChannel is acting up, probably because of Available returning strange values. Another measure is to also clear more members of nsJARInputStream to prevent misuse and intermittent behavior. 176 // ask the JarStream for the content length 177 mJarStream->Available((PRUint32 *) &mContentLength); Others: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLDataTransfer.cpp#1435 http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLDataTransfer.cpp#1435 http://mxr.mozilla.org/mozilla-central/source/dom/src/json/nsJSON.cpp#548 http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptinfo/src/xptiZipLoader.cpp#54 http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7733 And even the documentation of nsIInputStream is not clear on Available: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIInputStream.idl#98 Working on a patch to fix this issues with libjar. For the callers not checking return status of Available, another bug should be created.

Handle Available error code in nsJARchannel. Let Available always set a sensible value in _retval. Do more initializing in nsJARInputStream(). And ensure better detection of end of file/stream situation.

Re comments #29 and #30: I think this is an issue within nsXULDocument::OnStreamComplete, for there is a 'wallpaper' patch in bug 270042. For the many crashreports mentioned overthere, in only one nsJARinputStream is visible, all others are all about nsXULDocument::OnStreamComplete crashing on a null mCurrentScriptProto. So, I would request to first fix bug 270042, which is still causing issues, and see if we can retest this patch on a tinderbox build somehow.

(In reply to comment #36) > So, I would request to first fix bug 270042, which is still causing issues, and > see if we can retest this patch on a tinderbox build somehow. i can land patches on the Places branch for testing if you need.

Thanks Dietrich, it would probably help me a lot if you can apply the patch from this bug and from bug 270042 to the Places branch, so that we can verify if mochitest then doesn't fail anymore on this.

Test-landing this on the Places branch is on my list for the upcoming week.

I have managed to do a tryserver build myself with patch v7. The mochitest still fails in Winnt, so at least it is now 'reproducable'. Now I only need to find the root cause of this failure.

This patch survives multiple goes in the tryserver unit tests, for which v7 (and older) were failing, and even crashing on Mac. I found the culprit: With my patch the initial state of nsJARinputStream was 'mClosed=true', while before it was false before it opened. So, when for example JARchannel/thunk does an Available or Read before it is really opened, it would get NS_BASE_STREAM_CLOSED instead of NS_OK, so it would close the channel before really opening it. So, I have replaced the three booleans representing the state with one enum, including a state for notinited, so to always return a good return to the caller. This should be OK to commit to the tree.

What makes me uneasy about all this is that we have certain files that are opened on one thread and closed on another, which seems like a recipe for disaster after we switched to mmap io. Example: content/browser/browser.xul. Before mmaped patch, the OS provided natural locking due to blocking io, but I'm wondering if we should do locks while doing mmaped read()/open()/close()

alfred, any idea if comment #42 is a concern? what else is blocking this from landing?

(In reply to comment #43) > alfred, any idea if comment #42 is a concern? what else is blocking this from > landing? It's not a concern to this particular bug

There is nothing left that blocks the landing of the last patch. The issue that caused mochitest failures has been found and fixed in the last version of the patch.

Note, I have done a another tryserver run to be really sure, and the mochitests are all fine.

Note, I have done a another tryserver run to be really sure, and the mochitests are all fine.

Thanks!

Comment on attachment 402424 [details] [diff] [review] v8: differentiate between not initialized and closed a=me per discussion w/ beltzner et al.