399727 - Deflate stream converter produces raw deflate data rather than zlib compressed data

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Mime Čuvalo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007101104 Minefield/3.0a9pre eMusic DLM/4.0_1.0.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007101104 Minefield/3.0a9pre

I've been testing with the latest and greatest contribution by Dave Townsend (from bug 379633) of using an nsIStreamConverter to compress data.  It works fine on first glance.  Internally, nsIStreamConverter is able to compress data and then uncompress the data with another converter (using @mozilla.org/streamconv;1?from=deflate&to=uncompressed).

But if I compress the data and send it off to another program to decompress the other programs complain of zlib errors.  I've tested this 2 programs so far that behave this way: the Filezilla and CrossFTP servers (in MODE Z).

Now maybe they're both at fault - I'm not sure but I think something's up in nsIZipWriter.  I'll attach the test files I've been using shortly.  Please let me know if there's anything you would like me to test or if you need more information.  This would be a fantastic for users of FireFTP if I could only get it to play nicely with others.

Reproducible: Always

Comment 1

[Mime Čuvalo]

Attachment: Original file (uncompressed)

Comment 2

[Mime Čuvalo]

Attachment: Mozilla compressed version of the file

Comment 3

[Mime Čuvalo]

Attachment: Filezilla compressed version of the file

Comment 4

[Mime Čuvalo]

Attachment: CrossFTP compressed version of the file

Comment 5

[Mime Čuvalo]

By the way, a way to test this:

1.) Install, setup the Filezilla server (http://filezilla-project.org/)
2.) Get an FTP client (e.g. FireFTP http://fireftp.mozdev.org/)
3.) Open a connection to the server.
4.) Do a custom command:  MODE Z
5.) Upload one of the test files.  The Mozilla version will fail, the other ones will be successful.

Comment 6

[Dave Townsend [:mossop]]

If you ignore the first 2 bytes and last 4 of the CrossFTP example the compressed data generated by the deflate stream converter is identical.

The data is missing the CMF and FLG markers and the following ADLER32 checksum. These are things that could be added by the streamconverter's caller but it may be worthwhile to make the streamconverter do it itself. Need to do some investigation on how widespread the usage is.

Comment 7

[Dave Townsend [:mossop]]

What MODE Z uses isn't a raw deflate stream, it is the "ZLIB Compressed Data Format", usually with a compression method of deflate. IANA in their wisdom have allocated this compression format a Content-Encoding value of "deflate".

PNG uses the zlib compressed data format as is. HTTP content encoding and obviously MODE Z as well. GZIP uses different headers and checksums only retaining the raw deflated data. ZIP also uses the raw deflated data with it's own headers.

Given that IANA have allocate deflate to describe the zlib format it would probably be a good idea to make the stream converter match that, but I think it would be sensible to maybe also make it respond to something like from=uncompressed&to=rawdeflate such that it is still relatively easy to get the raw data for other uses.

The from=deflate&to=uncompressed succeeds in inflating the data we currently generate because it has some workarounds for broken webservers that don't send the zlib headers and it looks like it ignores the trailing checksum if it is there as well.

Comment 8

[Mime Čuvalo]

So should I just add those bits manually in the data or...?

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 1

zlib can (unsurprisingly) generate the zlib format header and footer itself so this is a fairly simple patch, just registers the stream converter for a few extra contracts and configures zlib accordingly.

Adds a testcase which uses the source html and crossftp generated version as sources for comparison.

Comment 10

[Mime Čuvalo]

Would this be blocking 1.9 as it deals with fixing a bug in the implementation of a new feature?

Comment 11

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

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

+    else if (!PL_strncasecmp(aFromType, GZIP_TYPE, sizeof(GZIP_TYPE)-1) ||
+             !PL_strncasecmp(aFromType, X_GZIP_TYPE, sizeof(X_GZIP_TYPE)-1))

spaces around the minus, please

+typedef enum {
+    WRAP_ZLIB,
+    WRAP_GZIP,
+    WRAP_NONE
+} WrapMode;

this is C++, just use:

enum WrapMode { ... };

Also, declare that inside the class declaration, probably in the private section.

Comment 12

[Dave Townsend [:mossop]]

Note to self, from IRC also change from strncasecmp to strcasecmp.

Comment 13

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Addressed comments carrying over review.

Requesting approval to land for 1.9. This is a fairly simple fix that makes the implementation of this component meet the IETF spec, without it the component cannot be used for what was one of it's intended use, sending deflated content data to http and ftp servers.

The component is compiled into Firefox but not actually used by the app at all so there is no risk to the general use of the application only a benefit of a working component to other applications and extensions using the platform.

Comment 14

[Dave Townsend [:mossop]]

Checking in src/ZipWriterModule.cpp;
/cvsroot/mozilla/modules/libjar/zipwriter/src/ZipWriterModule.cpp,v  <--  ZipWriterModule.cpp
new revision: 1.2; previous revision: 1.1
done
Checking in src/nsDeflateConverter.cpp;
/cvsroot/mozilla/modules/libjar/zipwriter/src/nsDeflateConverter.cpp,v  <--  nsDeflateConverter.cpp
new revision: 1.2; previous revision: 1.1
done
Checking in src/nsDeflateConverter.h;
/cvsroot/mozilla/modules/libjar/zipwriter/src/nsDeflateConverter.h,v  <--  nsDeflateConverter.h
new revision: 1.2; previous revision: 1.1
done
Checking in src/nsZipDataStream.cpp;
/cvsroot/mozilla/modules/libjar/zipwriter/src/nsZipDataStream.cpp,v  <--  nsZipDataStream.cpp
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/modules/libjar/zipwriter/test/unit/test_bug399727.js,v
done
Checking in test/unit/test_bug399727.js;
/cvsroot/mozilla/modules/libjar/zipwriter/test/unit/test_bug399727.js,v  <--  test_bug399727.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/modules/libjar/zipwriter/test/unit/data/test_bug399727.html,v
done
Checking in test/unit/data/test_bug399727.html;
/cvsroot/mozilla/modules/libjar/zipwriter/test/unit/data/test_bug399727.html,v  <--  test_bug399727.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/modules/libjar/zipwriter/test/unit/data/test_bug399727.zlib,v
done
Checking in test/unit/data/test_bug399727.zlib;
/cvsroot/mozilla/modules/libjar/zipwriter/test/unit/data/test_bug399727.zlib,v  <--  test_bug399727.zlib
initial revision: 1.1
done

Comment 15

[Mime Čuvalo]

Thanks, Dave!  It works great now!  Much obliged.