446708 - zlib/zipwriter dies when zipping this directory

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[David Ascher (:davida)]

Attachment: problematic keynote bundle (zipped for attachment)

I'll attach a keynote file which Tb/trunk can't attach because the directory of the bundle is +x.  chmod -x makes it attachable.

Comment 1

[Håkan Waara]

Attachment: Simplified testcase bundle (zipped)

Here's a bare bones bundle which still fails. It contains only one folder with one file (a TIFF file). Its structure is like this:

OSCON_unattachable_simplified_testcase.key/
  thumbs/
    st14-1.tiff

For some reason, zipwriter fails when it's just done adding the file and tries to close the stream. 

This is the stack:

#0	0x14b9a5e0 in nsZipDataStream::OnStopRequest at nsZipDataStream.cpp:137
#1	0x14b9aaf5 in nsZipDataStream::ReadStream at nsZipDataStream.cpp:229
#2	0x14b9e5d2 in nsZipWriter::AddEntryStream at nsZipWriter.cpp:517
#3	0x14b9ef0d in nsZipWriter::AddEntryFile at nsZipWriter.cpp:420
#4	0x136d6167 in nsSimpleZipper::AddToZip at nsMsgAttachmentHandler.cpp:120

The first call is the one that is failing, and returning NS_BASE_STREAM_CLOSED.

Comment 2

[Håkan Waara]

Mossop, this might be interesting for you. Turns out it is zlib/zipwriter which is dying. 

I have an extremely minimal testcase to zipwriter to reproduce it:  it's just a folder structure with one file, and then a script to try to zip it.

Here is the information I've gathered:

* The testcase data that cannot be zipped looks like this:

  testcase.key/     (drwxr-xr-x@)
    thumbs/         (drwxr-xr-x@)
      st14-1.tiff   (-rw-r--r--@)

* If you chmod -x testcase.key/ it works.
* The piece of code that fails is this: http://mxr.mozilla.org/mozilla/source/modules/libjar/zipwriter/src/nsDeflateConverter.cpp?mark=196#196
* deflate() is returning a non-zero error code (1) and then we're screwed.

The testcase data + xpcshell test is coming up. Hopefully the hg patch still preserves all file permissions, otherwise you can see them above.

Comment 3

[Håkan Waara]

Attachment: testcase.key (zipped)

hg didn't want to include the testcase.key/ so I'm zipping it and putting here.

To test this bug:

Unzip this into modules/libjar/zipwriter/test/unit/data/ and run the upcoming xpcshell testcase.

Comment 4

[Håkan Waara]

Attachment: xpcshell testcase for this bug

Here's a patch with the actual xpcshell test that fails. 

BTW, the failure is this:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80470002: file /Users/hakan/Programmering/mozilla/mozilla-central/modules/libjar/zipwriter/src/nsDeflateConverter.cpp, line 198
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80470002: file /Users/hakan/Programmering/mozilla/mozilla-central/modules/libjar/zipwriter/src/nsZipWriter.cpp, line 424

[Exception... "Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIZipWriter.addEntryFile]"  nsresult: "0x80470002 (NS_BASE_STREAM_CLOSED)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_zipwriter//unit/test_bug446708.js :: AddToZip :: line 17"  data: no]

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 1

We are catching an edge case where the final call to deflate realises it is out of data and completes, zipwriter then attempts to flush that 0 data out to the stream which is a bad thing.

Made a couple of minor tweaks to the testcase to make it work, also renamed the directory for identification, I verified that with these changes it still fails without the patch.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 332519 [details] [diff] [review]
patch rev 1

actually shouldn't you still do the "now set the state for 'deflate'" part?

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #6)
> (From update of attachment 332519 [details] [diff] [review])
> actually shouldn't you still do the "now set the state for 'deflate'" part?

No, we're catching the case where zlib hasn't added anything to the output buffer, so it already has the full buffer available to it which is all that code does.

Comment 8

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/index.cgi/mozilla-central/rev/132495a0e3c0

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

David, does it fix your problem? Can we mark it as verified?

Comment 10

[Håkan Waara]

I was surprised to reproduce this bug when trying to attach the file with bug 452206's patch applied.   

David, do you think you could try to repro/verify this bug on a current nightly, and then perhaps see if bug 452206's attachment regresses it?

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> I was surprised to reproduce this bug when trying to attach the file with bug
> 452206's patch applied.   
> 
> David, do you think you could try to repro/verify this bug on a current
> nightly, and then perhaps see if bug 452206's attachment regresses it?
> 

I don't really know how to reproduce this in a build except using the testcase. Are you saying that the testcase it has is failing in some way? Do you have a crash report of it crashing in a nightly?

I can't see any reason that that attachment would affect this.

Comment 12

[Håkan Waara]

(In reply to comment #11)
> (In reply to comment #10)
> > I was surprised to reproduce this bug when trying to attach the file with bug
> > 452206's patch applied.   
> > 
> > David, do you think you could try to repro/verify this bug on a current
> > nightly, and then perhaps see if bug 452206's attachment regresses it?
> > 
> 
> I don't really know how to reproduce this in a build except using the testcase.
> Are you saying that the testcase it has is failing in some way? Do you have a
> crash report of it crashing in a nightly?
> 
> I can't see any reason that that attachment would affect this.

Sorry Mossop, that was targetted to David Ascher. I shouldn't use first name here. :( 

Anyhow: the bug never showed up as a crash, but as zipwriter returning NS_BASE_STREAM_CLOSED_ERROR.

I'm also very surprised/confused why it would suddenly regress or be related to the other bug. 

I've just started investigating what could be wrong (possibly with my tree), but haven't yet found anything, which is worrying.  So when continuing on this it would be very useful if the (Thunderbird-)bug is either not really fixed on trunk with your patch, or my patch regresses it, or if I am crazy. :-)

Comment 13

[David Ascher (:davida)]

I just sent myself the bundle in the first attachment (note to history: bundle is zipped in attachment, needs to be unzipped to test).

AFAICT, this is VERIFIED.

(note: it's really, really slow to send, but that's a different bug)