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

RESOLVED FIXED in mozilla1.9beta2

Status

()

Core
General
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Mime Čuvalo, Assigned: mossop)

Tracking

Trunk
mozilla1.9beta2
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
Created attachment 284792 [details]
Original file (uncompressed)
(Reporter)

Comment 2

10 years ago
Created attachment 284793 [details]
Mozilla compressed version of the file
(Reporter)

Comment 3

10 years ago
Created attachment 284794 [details]
Filezilla compressed version of the file
(Reporter)

Comment 4

10 years ago
Created attachment 284795 [details]
CrossFTP compressed version of the file
(Reporter)

Comment 5

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

Comment 6

10 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 7

10 years ago
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.
Summary: Deflate stream converter produces data unreadable by other programs → Deflate stream converter produces raw deflate data rather than zlib compressed data
(Reporter)

Comment 8

10 years ago
So should I just add those bits manually in the data or...?
(Assignee)

Updated

10 years ago
Assignee: nobody → dtownsend
(Assignee)

Comment 9

10 years ago
Created attachment 286147 [details] [diff] [review]
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.
Attachment #286147 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #286147 - Flags: review? → review?(cbiesinger)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch]
(Reporter)

Comment 10

10 years ago
Would this be blocking 1.9 as it deals with fixing a bug in the implementation of a new feature?
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.
Attachment #286147 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 12

10 years ago
Note to self, from IRC also change from strncasecmp to strcasecmp.
(Assignee)

Comment 13

10 years ago
Created attachment 289653 [details] [diff] [review]
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.
Attachment #289653 - Flags: review+
Attachment #289653 - Flags: approval1.9?

Updated

10 years ago
Attachment #289653 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 14

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla1.9 M10
Version: unspecified → Trunk
(Assignee)

Updated

10 years ago
Flags: in-testsuite+
(Reporter)

Comment 15

10 years ago
Thanks, Dave!  It works great now!  Much obliged.
You need to log in before you can comment on or make changes to this bug.