Remove memcpy()s for compressed jar reading

RESOLVED FIXED in mozilla1.9.3a1

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: alfredkayser, Assigned: alfredkayser)

Tracking

(Blocks: 3 bugs, {crash, memory-footprint, perf})

unspecified
mozilla1.9.3a1
crash, memory-footprint, perf
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta5-fixed)

Details

(Whiteboard: [ts])

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 394779 [details] [diff] [review]
V1: Remove readbuf and its simulated 'read' for compressed jar IO

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.
Attachment #394779 - Flags: review?(benjamin)
(Assignee)

Updated

9 years ago
Blocks: 510611
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED

Updated

9 years ago
Attachment #394779 - Flags: review?(benjamin) → review?(tglek)

Comment 1

9 years ago
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.
Attachment #394779 - Flags: review?(tglek) → review-
(Assignee)

Comment 2

9 years ago
Created attachment 395128 [details] [diff] [review]
V2: Addressed (most of) the review comments

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.
Attachment #394779 - Attachment is obsolete: true
Attachment #395128 - Flags: review?(tglek)
(Assignee)

Comment 3

9 years ago
Created attachment 395131 [details] [diff] [review]
V2b: Forgot to remove nsZipHandle from the include file

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.
Attachment #395131 - Flags: review?(tglek)
(Assignee)

Updated

9 years ago
Attachment #395128 - Attachment is obsolete: true
Attachment #395128 - Flags: review?(tglek)

Comment 4

9 years ago
(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

Comment 5

9 years ago
I added a testcase with mmap bug that tests this behavior

Updated

9 years ago
Whiteboard: [ts]
(Assignee)

Comment 6

9 years ago
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...

Comment 7

9 years ago
(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.
(Assignee)

Comment 8

9 years ago
Created attachment 395385 [details] [diff] [review]
V3: resurrect nsZipHandle
Attachment #395131 - Attachment is obsolete: true
Attachment #395385 - Flags: review?(tglek)
Attachment #395131 - Flags: review?(tglek)
(Assignee)

Updated

9 years ago
Attachment #395385 - Attachment is patch: true
Attachment #395385 - Attachment mime type: application/octet-stream → text/plain

Comment 9

9 years ago
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.
Attachment #395385 - Flags: review?(tglek) → review-

Updated

9 years ago
Depends on: 510247
(Assignee)

Updated

9 years ago
Blocks: 511736

Updated

9 years ago
Blocks: 511754
(Assignee)

Comment 10

9 years ago
Created attachment 396967 [details] [diff] [review]
V5: Addressed comments of Taras and rebased after bug 510247
Attachment #395385 - Attachment is obsolete: true
Attachment #396967 - Flags: review?(tglek)

Updated

9 years ago
Attachment #396967 - Flags: review?(tglek) → review+
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.
(Assignee)

Comment 13

9 years ago
Created attachment 397267 [details] [diff] [review]
V6: Addressed final nits of Taras

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.
Attachment #396967 - Attachment is obsolete: true
Attachment #397267 - Flags: superreview?(benjamin)
(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.
(Assignee)

Comment 15

9 years ago
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.
Attachment #397267 - Flags: superreview?(benjamin)
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(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
(Assignee)

Comment 17

9 years ago
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.
Attachment #397267 - Attachment description: V6: Addressed final nits of Tarak → V6: Addressed final nits of Taras
Summary: Remove memcpy's for compressed jar reading → Remove memcpy()s for compressed jar reading
http://hg.mozilla.org/mozilla-central/rev/3b1daf281ded
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

9 years ago
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...
(Assignee)

Comment 22

9 years ago
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.
(Assignee)

Comment 23

9 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

9 years ago
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...
(Assignee)

Comment 31

9 years ago
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
(Assignee)

Comment 33

9 years ago
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.
(Assignee)

Comment 34

9 years ago
Created attachment 398462 [details] [diff] [review]
Be more careful in resetting values to prevent intermittent behavior.
Attachment #397267 - Attachment is obsolete: true
(Assignee)

Comment 35

9 years ago
Created attachment 398465 [details] [diff] [review]
Differences between v6 and v7.

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.
(Assignee)

Comment 36

9 years ago
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.
(Assignee)

Comment 38

9 years ago
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.
Whiteboard: [ts] → [ts][needs test-landing dietrich]
(Assignee)

Comment 40

9 years ago
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.
(Assignee)

Comment 41

9 years ago
Created attachment 402424 [details] [diff] [review]
v8: differentiate between not initialized and closed

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.
Attachment #398462 - Attachment is obsolete: true
Attachment #398465 - Attachment is obsolete: true
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()
Whiteboard: [ts][needs test-landing dietrich] → [ts]
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
(Assignee)

Comment 45

9 years ago
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.
Keywords: checkin-needed
(Assignee)

Comment 46

9 years ago
Note, I have done a another tryserver run to be really sure, and the mochitests are all fine.
(Assignee)

Comment 47

9 years ago
Note, I have done a another tryserver run to be really sure, and the mochitests are all fine.
http://hg.mozilla.org/mozilla-central/rev/44694b467f51
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 49

9 years ago
Thanks!

Updated

9 years ago
Attachment #402424 - Flags: approval1.9.2?

Updated

9 years ago
Blocks: 510874

Updated

9 years ago
Blocks: 521227

Updated

9 years ago
Blocks: 525755

Updated

9 years ago
No longer blocks: 510874
Comment on attachment 402424 [details] [diff] [review]
v8: differentiate between not initialized and closed

a=me per discussion w/ beltzner et al.
Attachment #402424 - Flags: approval1.9.2? → approval1.9.2+

Updated

9 years ago
status1.9.2: --- → final-fixed

Updated

9 years ago
Duplicate of this bug: 536021
Flags: blocking1.9.2+

Updated

9 years ago
Keywords: crash
You need to log in before you can comment on or make changes to this bug.