Closed Bug 510844 Opened 15 years ago Closed 15 years ago

Remove memcpy()s for compressed jar reading

Categories

(Core :: Networking: JAR, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta5-fixed

People

(Reporter: alfredkayser, Assigned: alfredkayser)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, memory-footprint, perf, Whiteboard: [ts])

Attachments

(1 file, 8 obsolete files)

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)
Blocks: 510611
Status: NEW → ASSIGNED
Attachment #394779 - Flags: review?(benjamin) → review?(tglek)
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-
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)
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)
Attachment #395128 - Attachment is obsolete: true
Attachment #395128 - Flags: review?(tglek)
(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
Whiteboard: [ts]
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.
Attached patch V3: resurrect nsZipHandle (obsolete) — Splinter Review
Attachment #395131 - Attachment is obsolete: true
Attachment #395385 - Flags: review?(tglek)
Attachment #395131 - Flags: review?(tglek)
Attachment #395385 - Attachment is patch: true
Attachment #395385 - Attachment mime type: application/octet-stream → text/plain
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-
Depends on: 510247
Blocks: 511736
Blocks: 511754
Attachment #395385 - Attachment is obsolete: true
Attachment #396967 - Flags: review?(tglek)
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.
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.
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)
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
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
Closed: 15 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 → ---
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.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 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 → ---
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.
Attachment #397267 - Attachment is obsolete: true
Attached patch Differences between v6 and v7. (obsolete) — Splinter Review
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.
Whiteboard: [ts] → [ts][needs test-landing dietrich]
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.
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
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.
http://hg.mozilla.org/mozilla-central/rev/44694b467f51
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Thanks!
Attachment #402424 - Flags: approval1.9.2?
Blocks: 510874
Blocks: 521227
Blocks: 525755
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+
Flags: blocking1.9.2+
Keywords: crash
You need to log in before you can comment on or make changes to this bug.