Closed Bug 105292 (deflate) Opened 23 years ago Closed 21 years ago

Accept-encoding: deflate unsupported, but requested

Categories

(Core :: Networking: HTTP, defect, P3)

Product:
Core
Component:
Networking: HTTP
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Future

People

(Reporter: m, Assigned: ddick)

References

(
URL
)

Details

(Keywords: topembed+, Whiteboard: [adt2 RTM] [eapp] sun_621 [ETA 08/08])

Attachments

(7 files, 16 obsolete files)

http log for the dflate tranasactions
316.78 KB, text/plain
Details
deflate compressed file (without http headers)
62.12 KB, application/octet-stream
Details
Trace file (w/ raw data) of deflate attachment request
63.30 KB, text/plain
Details
while deflating retry with larger output buffer as long as we get Z_BUF_ERROR
4.52 KB, patch
Details | Diff | Splinter Review
text.gif from a normal apache server install in icons dir
229 bytes, image/gif
Details
cvs diff against tip
6.93 KB, patch
darin.moz
: review-
Details | Diff | Splinter Review
v2 patch : revised formating
23.84 KB, patch
dougt
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review 
I've made some tests with compressed pages (static and on the fly compressed)
and noticed that Mozilla (0.9.4 and 0.9.5) was not able to display the
"deflate"- nor the "compress"-version. When I tried to view the source I only
got "<html><body></body></html>" the rendered page stood empty.
Tried with w3m everything worked fine. Mozilla worked only right with the gzip
compressed version.
You can see the problem on my testpage: http://future.matthias-wimmer.de/. I
have there a sample page in uncompressed (identity), gzip and compress encoding.
Only the first two work with Mozilla.
we should either stop sending deflate and compress in the Accept-Encoding, or
fix the problem. --> 0.9.7

Btw: i've seen this too.
Severity: normal → major
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → mozilla0.9.7
We have code to support it.
very true... my comments were meant to suggest that unless this can be fixed by
the next milestone, advertising support for the feature should simply be disabled.
ack!  i lost track of this bug somehow... will make sure it gets some lovin soon.
Priority: P3 → P1
Here's a patch that enables the "deflate" compression method - basically, all
that was missing was the update of uLen when the initial call to uncompress
returned Z_BUF_ERROR.

I've also disabled HTTP_COMPRESS_COMPRESS, since (as far as I know) there is
nothing in zlib that allows decompression of compress(1) content. The only
support for this that I could find was in gzip's unlzh.c module, but that only
offers a filedesc-to-filedesc interface instead of the memory-to-memory
interface that would be needed here.
gereon: thx for figuring out the problem.

your patch appears to be backwards... the +'s should be -'s and vice versa.

besides that, i thought zlib did support compress, but maybe i'm mistaken.  if
it doesn't support compress, then you are right to disable support for it, but
your patch does not do so correctly.  let's just make this patch fix the deflate
problem, and file a separate bug to clean up compress... there will be changes
that need to be made to the HTTP Accept-Encoding header, for example.
Sorry for creating any confusion, but I'll have to take my original patch back.
it was tested against a buggy HTTP server ... :-(

I've now investigated this further and have come up with the attached diffs for
nsHTTPCompressConv.cpp/.h (hopefully this time in the right direction).

I've also undone my admittedly half-harted attempt at disabling "compress".
Keywords: patch
Whiteboard: [patch needs r/sr=]
-> 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
-> vinay
Assignee: darin → badami
Status: ASSIGNED → NEW
This patch does not seem to work. I'am unable to view the "compress" version of
the image on http://future.matthias-wimmer.de/ with this.
Can u make sure that this patch is working ?


My patch wasn't intended to make "compress" work - it only enables "deflate". 
In fact, I think supporting compress is not easily possible to do, as I've 
indicated in a previous comment - I wasn't able to find anything that handles 
compressed data except for the compress(1) source code. Unfortunately, zlib 
doesn't do compress.
I'd recommend to remove support for compress and leave only gzip and deflate
Is there an  URL that I could test deflate against ?
This patch is required to suppres the accept-encoding header from sending
compress by default.
Sorry, I'm not aware of any public web servers that support deflate. What I did 
for my tests was install an Apache with mod_deflate. Unfortunately, that server 
is not available over the internet, but it's very easy to setup.


this is a blocker for sun/siebel->eapp, nsbeta1
Blocks: 125136
Keywords: nsbeta1
Whiteboard: [patch needs r/sr=] → [patch needs r/sr=][eapp]
Gereon
I'am unable to find a mod_deflate. Can u help me with the specifics ?

mod_deflate is available at ftp://ftp.lexa.ru/pub/apache-rus/contrib/.
Gereon

Thanks. Got it.
However the README seems to be in russian.
Can u tell me what u had to do to get it compiled as part of an apache build and
what directives u had to use to configure it ?

Vinay
I'm pretty busy right now, so apologies for the terseness:

# build Apache with mod_deflate:

gtar zxvf mod_deflate.1.0.11.tgz
cd mod_deflate.1.0.11 
configure --with-apache=<apache-dir> 
make  
cd <apache-dir> 
configure --activate-module=src/modules/extra/mod_deflate.o 
make 
make install

# edit httpd.conf, add to global settings:

DeflateEnable on
DeflateOrder deflate
DeflateMinLength 1
DeflateCompLevel 1
DeflateHTTP 1.0
DeflateTypes text/html text/plain  

# restart apache

# show that it works
telnet localhost 8080
GET / HTTP/1.1
Host: localhost
Accept-Encoding: deflate

prints deflated gibberish on my system

Hth!
Attached patch cleaned up patch (obsolete) — — Splinter Review 
same as earlier with attachment id=59668.
Earlier patch had some control characters in it.

Seems to work fine.
Testing a bit more.
In this http log grep for 
Content-encoding: deflate
Darin 
can u please review patches
1. to supress compress in accept encoding
2. cleaned up patch

Comment on attachment 70069 [details] [diff] [review]
cleaned up patch

sr=darin
Attachment #70069 - Flags: superreview+
Gagan, Rick
Can one of u r= this one ?
Vinay, I am helping Siebel to test this patch and see if it will fix their
deflate problem (for which the esculation was about). 
After I applied the supress-compress patch (in all.js), what do I need to do?
Just recompile libpref, or I have to recompile the whole browser? Thanks in
advance.

I think libpref should suffice.
The patch does not seem to fix Siebel's problem. After applying the patchs
(supress compression and cleaned up patch) to Netscape6.2.1 on their Solaris 8
machine, when trying to load the login page of Siebel app, inflate() (inside
do_deflate() in cleaned up patch) returned -5.
Any ideas? 

This deflate problem currently is Siebel's top bug, preventing them from
releasing their app with Netscape6.2.1 on Solaris. Timely help will be greatly
appreciated.

int
nsHTTPCompressConv::do_deflate 
{
    ...
    err = inflate(&stream, Z_FINISH);
    ...
}

The stream value was:

stream = {
   z_stream_s::next_in = 0x7afb78 ""  
   z_stream_s::avail_in = 0 
   z_stream_s::total_in = 1192U 
   z_stream_s::next_out = 0x2c32c7 ""  
   z_stream_s::avail_out = 409U 
   z_stream_s::total_out = 3167U 
   z_stream_s::msg = (nil)  
   z_stream_s::state = 0x7d3e70 
   z_stream_s::zalloc = 0xfdad7048 = &zcalloc 
   z_stream_s::zfree = 0xfdad705c = &zcfree 
   z_stream_s::opaque = (nil) 
   z_stream_s::data_type = 1192 
   z_stream_s::adler = 1U 
   z_stream_s::reserved = 4290700296U
}

z_stream_s::state was:
*`libnecko.so`nsHTTPCompressConv.cpp`nsHTTPCompressConv::do_deflate`stream.state
 = {
   internal_state::dummy = 7
}
Is there some URL that i can debug this against ?
If u applied the "clened up patch" u would have had to provide necko.dll also.
Did u do that or di u only provide libprefs ?
Vinay, we are working to get access to the application for you.
Answers to Vinay's questions:

Is there a URL you can access from Netscape?
No. Currently there is no external access to their test environment yet, and
Siebel is working on it. Is it possible that you can go to Siebel with me? I
have the debug environment all set up.Or you can remotely join me debugging.

Did you apply neco.dll besides libpref?
This problem is on their Solaris machine. After I applied the
supress-compression patch, I recompiled libpref, and libneco.so was updated.
New info: 
If a fix takes some time, Siebel will accept a temparery work around that simply
disable defalte, by telling the web sever that the browser can't handle
compressed files, so the web server will not send compress files to the browser. 
Any idea how to do that? If somebody can tell me how to do it, I can apply the
changes to the Sun branch. 
To disbale defalte temporarily, u need to do the same as what  I have done for
compress in the "to supress compress in accept encoding" patch.
ie +pref("network.http.accept-encoding" ,"gzip;q=0.9");

Sorry, I cannot go to Siebel with u since I work from India. (-:
Keywords: mozilla0.9.9
Error -5 from inflate() is Z_BUF_ERROR which means that the output buffer for
decompression of the deflated date was not large enough. Can you try increasing
the output buffer size and see if that helps? (e.g. change mOutBufferLen*=3 to
*=10 or something)

Alternatively, even if there is no outside access to the Siebel test site, could
you grab a copy of the raw compressed data (including http headers) using wget
or curl and make that available publicly?
Keywords: topembed
Adding a Siebel deflate compressed files (one without http headers, and one
with).
The compressed file should be a PDF file (schwab.PDF).
This contains the HTTP trace for deflate attachment request.  Includes HTTP
header info. and raw compressed data.
Some new finding after further debugging of the Siebel deflate problem:
Without the patch, Netscape 6.2.1 on Solaris (or Linux) was able to deflated a 
compressed login html file and the page was displayed OK (enve though inflate() 
still returned -5, expending mOutBuffer size from *3 to *10 did not help); but 
the next compressed PDF (schwab.PDF) file did not got deflated since it did not 
go through the same deflate method (OnDataAvailable(), where the patch applied) 
as the login html file did.
Should a compressed PDF file (to be deflated)also go through the same deflate 
method as a compressed html file? If so, where should it be deflated? 
Any ideas?
Has the patch been tested on Linux? 
In Siebel's case, there is a Siebel app server that compress files and then send 
them through the web server to the browser. 
Can u just give us the file that is being used for this test in an uncompressed
format ? 
I just ran some tests on the compressed data. The format is correct, and
inflate() decompresses it OK in one go *if* it is given a sufficiently large
buffer - slightly more than 82K in this case. All buffer sizes below this limit
result in return code -5, everything large give 0, just like it should.

My bet is that somehow the buffer size calculation (or size adjustment) failed.
Vinay, 
We'll post the uncompressed file.

Gereon, 
In our test at Siebel, the PDF file did not go through inflate() in the patch 
at all. In Siebel's case, their application server sent the compressed file to 
the web server and then to the browser (as an attachment I believe). Would that 
make a difference? How was your test setup? Did you apply the patch, and the 
PDF file just ran through the patch?
I'll enlarge the buffer size, and try it again. Last time I enlarged the output 
buffer from x3 to x10, and inflate() still returned -5. Should I make it bigger?
Comment on attachment 65978 [details] [diff] [review]
to supress compress in accept encoding

sr=darin
Attachment #65978 - Flags: superreview+
cc'ing bz...

bz: FYI the siebel server expects "Content-Encoding: deflate" to be applied to
saved content or content fed to a helper app.
Adding AOLTW
Whiteboard: [patch needs r/sr=][eapp] → [patch needs r/sr=][eapp],AOLTW
Darin, do you mean that they expect that the content will be inflated before
saving or passing to a helper?
Keywords: nsbeta1+
*** Bug 109279 has been marked as a duplicate of this bug. ***
OS: Linux → All
Could somebody please explain in detail what the problem with Siebel really is?
I mean, what is this (non-standard, I think) Attachment: header supposed to
mean, and how do they expect the browser to handle is? Display the file? Start
Acrobat? Save the file?

My tests were run on Linux, and I basically used the code I submitted in the
deflate patch to verify that it actually can decompress the data that Siebels's
server spits out.


gereon: thanks for testing that... it turns out that siebel expects the data to
be inflated before being sent to acroread.  currently, mozilla does not inflate
when saving to disk or streaming to a helper app.  this part of the bug is fixed
by bz's patch in bug 128199.
Keywords: nsbeta1
I landed Darin's 094 patch onto the AOLTW branch (minus the NS_NOTREACHED macro).
Plussing for AOLTW
Whiteboard: [patch needs r/sr=][eapp],AOLTW → [patch needs r/sr=][eapp],AOLTW+
The patch for 0.9.4 (provided by Darin Fisher) has been checked into the Sun
6.2.1 branch.
Whiteboard: [patch needs r/sr=][eapp],AOLTW+ → [patch needs r/sr=][eapp],AOLTW+, sun_621
I verified Darin's fix (for 0.9.4) in Siebel today with the new release build.

Rick can u review this one for the trunk please .
Keywords: topembed → topembed+
Whiteboard: [patch needs r/sr=][eapp],AOLTW+, sun_621 → [patch needs r/sr=][eapp],AOLTW+, sun_621 [adt2]
zero'ing the mozilla milestone. we still need this in the mozilla 1.0 branch and
trunk though.
Target Milestone: mozilla0.9.8 → ---
Geron
Shouldnt the code that does the do_deflate when we get a Z_BUF_ERROR be in a
while until we succeed on uncompressing the file or get an error that is not
Z_BUF_ERROR? Attaching a patch that does it ? Can u please take a look. 

Further I'am running into problems with the following gif file. Seems to hog
memory and not display it. Can u take a look ?

Vinay
Geron 
This is the same as the cleaned up version of ur patch except that I loop until
the   do_deflate succeeds with a non-Z_BUF_ERROR.
Can u take a look please ? Am I missing something obvious here ?
Geron
With this text.gif that comes as part of the normal apache distribution, I seem
to be running into problems where the do_delate keeps returning Z_BUF_ERROR
even if we loop increasing the output buf size. Unable to figure root cause for
this. Any ideas ?
Vinay,

the new patch looks OK to me, although I would prefer having some hard upper
limit of the decompression buffer. In theory, unlimited reallocation calls might
be a risk that can be exploited in a DoS attack.

As for the text.gif problem: I've just successfully downloaded this image from
my mod_deflate-enabled Apache (with compression for image/gif enabled, although
this is rather pointless in practice). 

Are you sure the tests you ran really used a deflated .gif?

Gereon
*** Bug 137244 has been marked as a duplicate of this bug. ***
Attached patch fix for Z_BUF_ERROR loops (obsolete) — — Splinter Review 
I've investigated the text.gif problem a little further, and it turns out that
there is indeed a problem with the way the inflate() buffers were handled. For
some files, inflate() needs one dummy byte trailing the compressed data stream,
and that is what this patch provides. (Thanks to Mark Adler for pointing me in
the right direction).
Attached patch combined patch (obsolete) — — Splinter Review 
icorporates 79785 + fix to the .h file + 65978
text.gif now works
Darin can u please sr this ?
Comment on attachment 80012 [details] [diff] [review]
combined patch

>Index: netwerk/streamconv/converters/nsHTTPCompressConv.cpp

>+                     printf("tries = %d inputbuflen=%d outputbuflen=%d\n", 
>+                            tries, mInpBufferLen, mOutBufferLen);

this needs to be commented out or #ifdef DEBUG_COMPRESS_CONV or some such
#define.


>Index: modules/libpref/src/init/all.js

>-pref("network.http.accept-encoding" ,"gzip, deflate, compress;q=0.9");
>+pref("network.http.accept-encoding" ,"gzip, deflate;q=0.9");

will we still need this?  is compress still broken?

otherwise, sr=darin
Attachment #80012 - Flags: superreview+
Darin, since compress is not implemented anywhere, I'd say support for it 
should be removed entirely.
that does make sense ;-)  thx!
-> me
Assignee: badami → darin
Status: NEW → ASSIGNED
Priority: P1 → P2
Whiteboard: [patch needs r/sr=][eapp],AOLTW+, sun_621 [adt2] → [patch needs r/sr=][eapp],AOLTW+, sun_621 [adt2 RTM]
Target Milestone: --- → mozilla1.0.1
Attached patch fix for deflate support (obsolete) — — Splinter Review 
I had a try at the lastly posted patch (Attachment #80012 [details] [diff]), I'm afraid
it doesn't work for me.  There seems to be a few oversights in deflate
support.

First, if the response header contains "Content-Type: dEflate" ("E" is
an upper case character), finding the appropriate converter fails
because BFSTable for converter chain is an instance of nsObjectHashtable.

Second, headers and trailers of deflated data should be expected.
RFC2616 defines deflate coding as follows:

   deflate
	The "zlib" format defined in RFC 1950 [31] in combination with
	the "deflate" compression mechanism described in RFC 1951 [29].

In the section 2.2 of RFC1950, the data format is defined to have 2-or
6-byte headers and 4-byte trailers.  On the other hand
nsHTTPCompressConv::do_deflate calls inflateInit2() with the negative
window size to skip over headers and trailers.	If it work against some sites,
then I doubt it is carried out by a bug of mod_deflate.
wrt the inflateInit() call: when I developed the patch, I initally started with 
a version that did use the zlib format. It turned out that I wasn't able to 
find a single web server where this format worked. After much googling I found 
hints that the RFC is in fact misleading and that no zlib headers are used, so 
I changed the implementation. Instant success, all deflate servers worked.
I also found that the headerless format is supported by the HTTP access 
functions in Java.

If you can provide a URL for a deflating server that does not run 
Apache/mod_deflate, I'll check again.
Gereon, please try <http://cvs.m17n.org/~akr/diary/> and
<http://cvs.m17n.org/~akr/a/>.  They are successfully viewed with w3m.

Looking over inflate.c coming from w3m-0.3.tar.gz would help you to
support the headerless format at the same time.  First it tries to
decode without headers and trailers, then if inflate() call fails,
does try again with a dummy header.
Daiki, thanks for your suggestions. I've incorporated them into the attached
patch.

Two questions, though: do you know what deflating module is running on
cvs.m17.org? It doesn't seem to be mod_deflate, but I'm not aware of any other
deflate module.
Also, cvs.m17.org seems to ALWAYS generate deflated content, even when the
request does not contain an Accept-Encoding header at all. That's just for
debugging purposes, I presume?
FYI: MSIE 5.5 displays an empty page for cvs.m17n.org/~akr/diary/. It appears
that it only supports the headerless deflate format generated e.g. by mod_deflate.
I thought this was going to go into the 1.0 trunk/branch?
mkaply: it appears to me that there is still ongoing discussion about whether or
not this is the right patch.

gereon, daiki: please let me know if you are comfortable with the current patch.
Darin,

if by current you mean attachment #85171 [details] [diff] [review], I'm comfortable with it, since it
allows for both versions of deflated content.

Gereon
ok, good... then i will move forward on trying to land that patch.  thx for all
the help on this one!
Blocks: 143047
Keywords: mozilla0.9.9 → mozilla1.0.1
Whiteboard: [patch needs r/sr=][eapp],AOLTW+, sun_621 [adt2 RTM] → [adt2 RTM] [patch needs r/sr=][eapp],sun_621 [eta needed]
no time now that i'm starting my vacation...

-> 1.2alpha
Target Milestone: mozilla1.0.1 → mozilla1.2alpha
*** Bug 160526 has been marked as a duplicate of this bug. ***
*** Bug 160667 has been marked as a duplicate of this bug. ***
*** Bug 160501 has been marked as a duplicate of this bug. ***
*** Bug 160829 has been marked as a duplicate of this bug. ***
*** Bug 160856 has been marked as a duplicate of this bug. ***
*** Bug 160870 has been marked as a duplicate of this bug. ***
*** Bug 160879 has been marked as a duplicate of this bug. ***
*** Bug 160881 has been marked as a duplicate of this bug. ***
darin, what's the current status of this bug and its patch? it's starting to
collect duplicates :)
*** Bug 160984 has been marked as a duplicate of this bug. ***
One of the reasons why this bug will collect many dupes is that the Times of
India is a very well respected and widely read newspaper in India (paper was
launched in 1838) and amongst the Indian diaspora. One can consider this bug as
a showstopper for evangalizing Mozilla usage in India and amongst overseas Indians.


*** Bug 161065 has been marked as a duplicate of this bug. ***
*** Bug 161118 has been marked as a duplicate of this bug. ***
*** Bug 161157 has been marked as a duplicate of this bug. ***
*** Bug 161060 has been marked as a duplicate of this bug. ***
*** Bug 161022 has been marked as a duplicate of this bug. ***
this bug is starting to become a _very big_ problem. It it really so difficult
to remove 'deflate' this from the accept-header as a workaround for 1.1? see
comment #88, why this is so important - currently it's only one page btw...
Keywords: mozilla1.1
Target Milestone: mozilla1.2alpha → mozilla1.1final
*** Bug 161223 has been marked as a duplicate of this bug. ***
Blocks: 1.1
Darin, is this really a 1.1 stopper? It's not a recent regression and it seems
to only be highly visible at one website. What's it going to take to fix and how
far off is that? We're targeting 1.1 for this week so time is short for any
changes here.
It's not a recent regression on our end, but this site just started sending us
encoded content... It didn't use to do it.
here's a one-liner that makes timesofindia.com load correctly again. 
ultimately, i'd like to fixup the stream converter service to properly
lowercase fromType and toType before looking for a converter, etc.  but, for
the sake of moz1.1, i'd recommend this simple patch.
Comment on attachment 94193 [details] [diff] [review]
v1 simplest patch required to make timesofindia.com load again

sr=bzbarsky
Attachment #94193 - Flags: superreview+
Comment on attachment 94193 [details] [diff] [review]
v1 simplest patch required to make timesofindia.com load again

r-dougt
Attachment #94193 - Flags: superreview+ → review+
Attachment #94193 - Flags: superreview+
Comment on attachment 94193 [details] [diff] [review]
v1 simplest patch required to make timesofindia.com load again

a=asa (on behalf of drivers) for checkin to 1.1 branch. Please land this as
soon as we have tinderboxen running at
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla1.1
Attachment #94193 - Flags: approval+
Keywords: adt1.0.1
Whiteboard: [adt2 RTM] [patch needs r/sr=][eapp],sun_621 [eta needed] → [adt2 RTM] [eapp] sun_621 [eETA 08/08]
are there any other top sites, besides www.timesofindia.com, that would be
benefit from this fix?
Whiteboard: [adt2 RTM] [eapp] sun_621 [eETA 08/08] → [adt2 RTM] [eapp] sun_621 [ETA 08/08]
I've told ADT (via AIM session with Jaime) about certain enterprise customers of
Netscape bits (spun of Mozilla branches) that could be affected by this.
checked in attachment 94193 [details] [diff] [review] on the trunk... that doesn't completely fix this
bug, but it does take care of timesofindia.com.  i'm keeping this bug open in
order to address changes required to the stream converter service.
*** Bug 161455 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.1final → M1
Target Milestone: M1 → mozilla1.1final
fyi, www.timesofinida.com is working in build 2002080705. Thanks. Removing the
URL since it doesn't apply anymore.
URL: http://www.timesofindia.com
Based on Comment #104 and Comment #106, does this still need to be a blocker to
v1.1? I'm assuming that the fix is in the v1.1 branch (Darin, correct me if I'm
wrong on that).
*** Bug 161489 has been marked as a duplicate of this bug. ***
this hasn't been fixed on the moz1.1 branch yet.
verified that this fixes timesofindia urls on trunk.   Tested on winNT4, linux
rh6, mac osX using 08/07/2002 builds.  Leaving open per comment# 104.
adt1.0.1+ (on ADT's behalf) approval for the checkin of attachment 94193 [details] [diff] [review] (only),
to the 1.0 branch, pending Drivers' approval. pls check this in asap, the
replace "fixed1.0.1" with "verified1.0.1". thanks!
Keywords: adt1.0.1 → adt1.0.1+
a=chofman for 1.0.1
checked in the fix on the 1.0 branch for mozilla1.0.1
checked in fix on the 1.1 branch.
Keywords: mozilla1.0.1 → fixed1.0.1
verified 1.0 branch - win NT4, mac osX, linux rh6 08/08/02 builds
Keywords: fixed1.0.1 → verified1.0.1
No longer blocks: 1.1
*** Bug 162292 has been marked as a duplicate of this bug. ***
targeting 1.2 alpha for the remaining work...
Target Milestone: mozilla1.1final → mozilla1.2alpha
*** Bug 162673 has been marked as a duplicate of this bug. ***
*** Bug 162880 has been marked as a duplicate of this bug. ***
Gereon: your latest patch (attachment 85171 [details] [diff] [review]) looks like something we might
really want to consider adding to mozilla, but it does not apply cleanly at all
to the mozilla trunk.  any chance you could bring it up-to-date?  thx!

-> futured for now
Severity: major → normal
Priority: P2 → P3
Target Milestone: mozilla1.2alpha → Future
Darin - I'm off for vacation myself, so I won't be able to work on this in the 
next two weeks. If anyone else volunteers that's fine with me, otherwise I'll 
look into when I return.
*** Bug 163100 has been marked as a duplicate of this bug. ***
*** Bug 163346 has been marked as a duplicate of this bug. ***
*** Bug 163484 has been marked as a duplicate of this bug. ***
I am using Mozilla 1.1 (the build released on August 26th) and timesofindia.com
does not load in Mozilla. This was working in some pre-release builds of 1.1.
Manoj, Which OS. It works for me on 1.1 release on Linux
sorry, I should have included the OS version in my comment:

OS: Win XP Pro

It suddenly WFM too... This is interesting. But I checked some 5 times last 
night before I filed the bug. Dohhh!
Attached patch Updated patch (obsolete) — — Splinter Review 
Unsure what went wrong with the previos patch, here it is again, applies
cleanly to the 1.0 branch (as far as i can tell, nsHTTPCompressConv.cpp/.h
hasn't changed since the early 0.9 releases)
*** Bug 173457 has been marked as a duplicate of this bug. ***

        Attachment #59510 -
        Attachment is obsolete: true

        Attachment #59668 -
        Attachment is obsolete: true

        Attachment #65978 -
        Attachment is obsolete: true

        Attachment #70069 -
        Attachment is obsolete: true

        Attachment #73092 -
        Attachment is obsolete: true

        Attachment #79785 -
        Attachment is obsolete: true

        Attachment #80012 -
        Attachment is obsolete: true

        Attachment #84890 -
        Attachment is obsolete: true

        Attachment #85171 -
        Attachment is obsolete: true

        Attachment #94193 -
        Attachment is obsolete: true

        Attachment #97301 -
        Attachment is obsolete: true

    
  

        Attachment #115388 -
        Flags: superreview?

        Attachment #115388 -
        Flags: review?(darin)

    
    

    
  
Comment on attachment 115388 [details] [diff] [review]
cvs diff against tip

>Index: mozilla/netwerk/streamconv/converters/nsHTTPCompressConv.cpp

>                     if (mOutBuffer == NULL)
>                         return NS_ERROR_OUT_OF_MEMORY;
>+                     // update uLen as well: 
>+                     uLen = mOutBufferLen;
>+                     code = do_deflate (mOutBuffer, &uLen, mInpBuffer, streamLen+1);
>+                     printf("tries = %d inputbuflen=%d outputbuflen=%d\n", 
>+                            tries, mInpBufferLen, mOutBufferLen);

indentation is off-by-one.  please kill the printf or #ifdef DEBUG.

timeless: have you tested this patch?

        Attachment #115388 -
        Flags: review?(darin) → review-

    
  

        Attachment #115388 -
        Flags: superreview?

    
    

    
  
can we please remove the advertising for deflate and compress in time for 1.4?
This seems to have been going on forever.

    
    

    
  
David: deflate and compress work just fine in many cases.  i think we're only
talking about an edge case here.

    
    

    
  
g'day Darren,
i'm using 

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030210

and i can't get either compress || deflate to work with it.  Lynx works with
compress and Opera works with deflate. i got desperate and downloaded the

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030304

nightly binary and it didn't work either.  Which is why i requested the removal
of the Accept-Encoding: deflate,compress options until the issues get ironed
out.  I take it that the Win32 version is fine?

    
    

    
  
David: linux version seems to work correctly for me.  care to post a testcase?

    
    

    
  
Following perl cgi script works fine for Opera, nix for moz.

#! /usr/local/bin/perl -w

use Compress::Zlib();
use strict;

MAIN: {
        my ($html) = <<_HTML_;
<html><head><title>Accept-Encoding</title></head>
<body>
This is the Accept-Encoding test
</body>
</html>
_HTML_
        my ($deflatedBody);
        my ($deflationStream, $status) = Compress::Zlib::deflateInit();
        if ($status eq Compress::Zlib::Z_OK()) {
                my ($out, $status) = $deflationStream->deflate($html);
                if ($status eq Compress::Zlib::Z_OK()) {
                        $deflatedBody = $out;
                        my ($flush, $status) = $deflationStream->flush();
                        if ($status eq Compress::Zlib::Z_OK()) {
                                $deflatedBody .= $flush;
                        } else {
                                die('Deflate Failed in flush:' . $status);
                        }
                } else {
                        die('Deflate Failed:' . $status);
                }
        } else {
                die('Deflate Failed to initialise:' . $status);
        }
        print <<_HTTP_;
Content-Type: text/html
Content-Encoding: deflate

_HTTP_
        print $deflatedBody;
}


    
    

    
  
David, as I understand it, there are actually several slightly different
algorithms that go by the name "compress/deflate".  The zlib used in Mozilla
handles some of them but not others; it may be that the perl zlib uses some of
these others.

    
    

    
  
AFAIK, Compress::Zlib is just an interface to the zlib library.  The calls i
used in the test case should therefore translate directly to the zlib library. 
I don't know enough about compression to quickly compare the deflate definition
in rfc2616 with the deflate definition on zlib.org.  Can someone supply me with
a test case that mozilla does work with?

Compress::Zlib doco url follows;

http://search.cpan.org/author/PMQS/Compress-Zlib-1.19/Zlib.pm

    
    

    
  
Following is a perl cgi script to test the compress encoding.  It works on 
Lynx/2.8.4rel.1 libwww-FM/2.14 SSL-MM/1.4.1 OpenSSL/0.9.6b
but does not work with moz 1.3b.  This time there is less confusion about the
method as rfc2616 states that compress is the UNIX utility compress, which is
used by this script.

#! /usr/local/bin/perl -w

use POSIX();
use strict;

MAIN: {
        my ($html) = <<_HTML_;
<html><head><title>Accept-Encoding</title></head>
<body>
This is the Accept-Encoding test
</body>
</html>
_HTML_
        my ($fileName) = POSIX::tmpnam();
        open(FILE, '>' . $fileName) || die("Failed to open $fileName:$!");
        print FILE $html;
        close(FILE);
        $ENV{'PATH'} = '/usr/bin';
        `/usr/bin/compress $fileName`;
        open(FILE, $fileName . ".Z") || die("Failed to open $fileName:$!");
        my ($compressedBody);
        while(<FILE>) {
                $compressedBody .= $_;
        }
        close(FILE);
        unlink($fileName . ".Z");

        print <<_HTTP_;
Content-Type: text/html
Content-Encoding: compress

_HTTP_
        print $compressedBody;
}

    
    

    
  
This patch has nothing to do whatsoever with "compress", like I mentioned in 
an earlier comment.
I thought that advertising compress in the Accept-Encoding header had been 
dropped long ago...

    
    

    
  
if the patch doesn't do something about compress, then it's not fixing this bug
as currently defined.  either the patch for this bug needs to include a fix to
not advertise compress (one of the older patches here seems to have that one
line change included), or this bug should be resummarised and a new bug filed
for removing the compress thing (which is what Darin suggested way back in
comment 6)

    
    

    
  
From comments 6 and 141, a new bug has been filed for the removal of compress
from the Accept-Encoding headers.  The bug number is 196406

    
    

    
  
timeless's 
(http://bugzilla.mozilla.org/attachment.cgi?id=115388&action=view)
 patch works for me.

    
    

    
  
Correction.  The patch seems to improve moz's handling of deflate, but it is
still failing to handle some pages.  Seems to be large pages.  Still
investigating.  Error messages follow.

tries = 1 inputbuflen=3764 outputbuflen=33867
tries = 2 inputbuflen=3764 outputbuflen=101601
tries = 3 inputbuflen=3764 outputbuflen=304803
tries = 4 inputbuflen=3764 outputbuflen=914409
tries = 5 inputbuflen=3764 outputbuflen=2743227
tries = 6 inputbuflen=3764 outputbuflen=8229681
tries = 7 inputbuflen=3764 outputbuflen=24689043
tries = 8 inputbuflen=3764 outputbuflen=74067129
tries = 9 inputbuflen=3764 outputbuflen=222201387
tries = 10 inputbuflen=3764 outputbuflen=666604161
Error loading URL http://localhost/deflate : 8007000e (NS_ERROR_OUT_OF_MEMORY)

    
  
Summary: Mozilla is sending an Accept-encoding for deflate and compress but doesn't support it. → Accept-encoding: deflate and compress unsupported, but requested

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch to make deflate work (obsolete)
        — 
          — Splinter Review
      

    


  
ok, this patch works against mod_deflate when set-up following instructions in
comment 19 and against the perl cgi script in comment 136.  Previous patches
seemed to have a problem when the nsHTTPCompressConv::OnDataAvailable function
was called twice for one entity (which can seem to happen for large, for
example compressed size > 20k, pages).	This patch will also work for these
large pages.

    
    

    
  
modifying summary to reflect that this bug is only about deflate now. (compress
has been removed from the accept-encoding in bug 196406)
Keywords: mozilla1.1
Summary: Accept-encoding: deflate and compress unsupported, but requested → Accept-encoding: deflate unsupported, but requested

    
    

    
  


  
removed a function declaration in the previous patch that was not required from
nsHTTPCompressConv.h

    
  

        Attachment #118891 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        same patch using "cvs diff -u10" (obsolete)
        — 
          — Splinter Review
      

    


  
created the patch using -u10, tried it out, and it doesn't seem to work.  i get
a 100% CPU usage spike.  seems to be inside nsHTTPCompressConv::OnDataAvailable
for ever.

        Attachment #118906 -
        Attachment is obsolete: true

    
    

    
  
darin, what did you do to make the patch fail? can you tell me more about the
example that caused the spike.  i have checked everything i can think of, but as
stated previously, all the examples discussed in this thread seem to work for me.
i've downloaded the latest cvs.
cd'ed to netwerk/streamconv/converters
applied patch 118984 with
patch -p0 <conf.patch
cd'ed to base
gmake -f client.mk build
./dist/bin/mozilla
and pointed it at a fresh install of apache/mod_deflate or at the perl-cgi
script or anything else. More information would be appreciated.

    
    

    
  


  
Darin.	I suspect this patch will stop the spike.  Can you let me know if it
works?

    
    

    
  
.
Assignee: darin → david_dick
Status: ASSIGNED → NEW

    
    

    
  
david: thanks for the update.. i'll give it a try today.

    
  
Status: NEW → ASSIGNED

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch with indenting correction (obsolete)
        — 
          — Splinter Review
      

    


  
Darin, can you confirm that the patch works and check the patch in?

        Attachment #118984 -
        Attachment is obsolete: true

        Attachment #119430 -
        Attachment is obsolete: true

    
    

    
  
Darin, just a reminder about this patch.  Could you please review it and check
it in to cvs when you get the chance.  

    
  

        Attachment #119430 -
        Attachment is obsolete: false

        Attachment #119430 -
        Attachment is patch: false

        Attachment #119430 -
        Flags: review?(darin)

        Attachment #119797 -
        Flags: review?(darin)

    
    

    
  


  
cleaned up the patch a little bit.

this testcase works:

  http://cvs.m17n.org/~akr/diary/

my own testcase created using the unix zip utility (included with RH8 does
not).  i'm not quite sure why my testcase fails... it hits the endless loop
detection code.  anyhow, the fact that this patch seems to help mod_deflate is
enough of a reason to try to check it in.

        Attachment #119430 -
        Attachment is obsolete: true

        Attachment #119797 -
        Attachment is obsolete: true

    
  

        Attachment #119430 -
        Flags: review?(darin)

    
  

        Attachment #119797 -
        Flags: review?(darin)

    
    

    
  
Comment on attachment 120470 [details] [diff] [review]
v2 patch : revised formating

sr=darin

        Attachment #120470 -
        Flags: superreview+

        Attachment #120470 -
        Flags: review?(dougt)

    
    

    
  
Comment on attachment 120470 [details] [diff] [review]
v2 patch : revised formating

cvs -uw patch would probably be better. 


untabify before checking in.  i found at least one tab...

+nsHTTPCompressConv::OnDataAvailable (nsIRequest* request, 
							  nsISupports
*aContext, 
							  nsIInputStream *iStr, 
							  PRUint32
aSourceOffset, 
							  PRUint32 aCount)
 {
     nsresult rv = NS_ERROR_FAILURE;
-	PRUint32 streamLen;
+	PRUint32 streamLen = aCount;

Do you really have to init the buffer:

+		     memset (&d_stream, 0, sizeof (d_stream));

can you kill the space between the function name and the parameter in this
file.  we are inconsistent.

        Attachment #120470 -
        Flags: review?(dougt) → review+

    
    

    
  
i think memset is important since inflateInit documentation says:

   "The fields next_in, avail_in, zalloc, zfree and opaque must be initialized 
    before by the caller."

    
    

    
  
Checking in nsHTTPCompressConv.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsHTTPCompressConv.cpp,v  <-- 
nsHTTPCompressConv.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in nsHTTPCompressConv.h;
/cvsroot/mozilla/netwerk/streamconv/converters/nsHTTPCompressConv.h,v  <-- 
nsHTTPCompressConv.h
new revision: 1.8; previous revision: 1.7
done

marking FIXED... thanks David!!
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED

    
  
Blocks: 191676

    
    

    
  
*** Bug 214767 has been marked as a duplicate of this bug. ***

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: