Closed Bug 309510 Opened 19 years ago Closed 18 years ago

bzip2-encoded (unsupported content-encoding) content can no longer be saved

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: vhaarr+bmo)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

adds content-type/encoding check for bzip2
5.92 KB, patch
darin.moz
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
patches the sky
7.52 KB, patch
Details | Diff | Splinter Review 
STEPS TO REPRODUCE:  Load
http://www.kernel.org/pub/linux/kernel/v2.6/patch-2.6.13.2.bz2

Actual results: A dialog that says "... could not be saved because the source
file could not be read"

Expected results: A helper app dialog

This is a regression from bug 184144.

What's going on here is that the server sends:

  Content-Type: application/x-bzip2
  Content-Encoding: x-bzip2

which is of course a base lie.  We handle this for gzip and compress, though, at
<http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#879>.
 For bzip2, we used to just ignore the Content-Encoding and treat it as  
application/x-bzip2 content, which is why things worked...

In any case, we at least need to add bzip2 handling to the http channel (in
terms of clearing out the Content-Encoding header).  And we should perhaps fix
nsExternalAppHandler::SendStatusChange to know about this content-encoding
error... Either that, or we somehow have a way for helper app service to ask
whether a conversion is supported and not apply it if it's not (which would be
ideal from my POV anyway).
Blocks: 184144
This adds another check for Content-Encoding/Type to nsHttpChannel, and removes
a silly, seemingly unused argument to nsExternalAppHandler::SetUpTempFile that
just had me confused when looking through the code :-)

It seems nsExternalAppHandler::SendStatusChange is only meant to deal with
local errors, so I'm not sure we should add a check for
NS_ERROR_INVALID_CONTENT_ENCODING there.

> Either that, or we somehow have a way for helper app service to ask
> whether a conversion is supported and not apply it if it's not (which
> would be ideal from my POV anyway).

Isn't that what
<http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsIExternalHelperAppService.idl#77>
does? I'm not sure I follow ...
Attachment #196944 - Flags: review?(darin)
> It seems nsExternalAppHandler::SendStatusChange is only meant to deal with
> local errors

No, it just happens that those are the only errors likely to popup up after
OnStartRequest.  If we plan to deliver this error after OnStartRequest, then we
need a useful error message for it here.

> Isn't that what
>
<http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsIExternalHelperAppService.idl#77>
> does?

No.  Please look at what the function actually does.  It knows nothing about
what encodings are supported....
Comment on attachment 196944 [details] [diff] [review]
adds content-type/encoding check for bzip2

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>     else if (encoding && PL_strcasestr(encoding, "compress") && (
>              mResponseHead->ContentType().EqualsLiteral(APPLICATION_COMPRESS) ||
>              mResponseHead->ContentType().EqualsLiteral(APPLICATION_COMPRESS2))) {
>         // clear the Content-Encoding header
>         mResponseHead->ClearHeader(nsHttp::Content_Encoding);
>     }
>+    else if (encoding && PL_strcasestr(encoding, "bzip2") && (
>+             mResponseHead->ContentType().EqualsLiteral(APPLICATION_BZIP) ||
>+             mResponseHead->ContentType().EqualsLiteral(APPLICATION_BZIP2) ||
>+             mResponseHead->ContentType().EqualsLiteral(APPLICATION_BZIP3) ||
>+             mResponseHead->ContentType().EqualsLiteral(APPLICATION_BZIP4))) {
>+        // clear the Content-Encoding header
>+        mResponseHead->ClearHeader(nsHttp::Content_Encoding);
>+    }

It seems to me that null checking encoding here is redundant.
PL_strcaststr already null checks its arguments.


It's amazing to me that this bug is showing up for bzip2-encoded
content.  I mean, I thought this was an old configuration bug in
Apache... what gives?
Attachment #196944 - Flags: superreview?(bzbarsky)
Attachment #196944 - Flags: review?(darin)
Attachment #196944 - Flags: review+
Hm, we get the same results by adding
{ APPLICATION_BZIP, "bz2"}
to
<http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#459>
as we do with attachment 196944 [details] [diff] [review] ...
Yes, because in that case the helper app service tells HTTP not to decode, and
in that case HTTP short-circuits and doesn't even try to decode and hence
doesn't pass in an error status.

Again, dealing with this well requires several changes in my opinion:

1)  The clearing that this patch does.
2)  The addition in comment 4.  I can understand if you would rather leave this
    to a followup bug and would rather not work on it at all.
3)  Some way provided by HTTP for the helper app service to be able to save the
    raw data when data _does_ come in in an encoding HTTP doesn't understand.
    Which means a way to ask whether HTTP understands the encoding.  This is
    critical, in my opinion -- at the moment there is no way for the user to
    save such data, after the changes for bug 184144, which is very wrong.
4)  Handling of this new error code in the error reporting in the helper app
    service.  Again, I would be fine with a separate bug on this, and I'd be
    willing to do this myself, I guess.
Comment on attachment 196944 [details] [diff] [review]
adds content-type/encoding check for bzip2

sr=bzbarsky on the nsHttpChannel changes.  Please do not make the external
helper app service change -- we have patches in the pipe that might use that
argument.

Also, I still want a patch to address item 3 in comment 5; please either file a
followup bug on that and fix it, or do it here....

Darin, the Apache misconfiguration is just there...  If people are using an
old-ish server and copy/paste the gzip config for bzip2, we get this...
Attachment #196944 - Flags: superreview?(bzbarsky) → superreview+
> 3)  Some way provided by HTTP for the helper app service to be able to save the
>     raw data when data _does_ come in in an encoding HTTP doesn't understand.
>     Which means a way to ask whether HTTP understands the encoding.  This is
>     critical, in my opinion -- at the moment there is no way for the user to
>     save such data, after the changes for bug 184144, which is very wrong.

Agreed.  This is very critical.

The Content-Encoding header removal is only really important when we support
bzip2 as a Content-Encoding, right?  Perhaps the best action to take right now
is to simply backout the fix for bug 184144.  It needs to be reworked to not
make it a fatal error to have an invalid encoding, but doing so would of course
 re-introduce bug 184144.  So, let's back it out and rethink this problem. 
Maybe a flag could be set on the channel to control whether or not it applies
the 'make it fatal' logic.
I think it's reasonable to make it a fatal error if the consumer didn't turn off
conversions.  My problem as a consumer is that to tell whether to turn off
conversions I need to know whether we can perform them... That information could
probably be exposed off nsIEncodedChannel...

The other option is to make this error non-fatal in the one case when we have an
unknown encoding (which is not what most of bug 184144 is about).  That would be
the safe thing to do, imo, if we want to take bug 184144 on a branch.  But for
trunk, I think exposing the info on nsIEncodedChannel is better.
Do you think we should try to fix bug 184144 on the 1.8 branch?  I'm leaning
towards no since we don't really have time to deal with regressions.
I'm leaning towards no as well, for the same reason.
> 2)  The addition in comment 4.  I can understand if you would rather leave this
>     to a followup bug and would rather not work on it at all.

I've tried wrapping my head around all the code in question here, but so far I
always end up with more questions than answers.

So what's needed is some way for the consumer to tell whether an encoding is
supported or not, and you want it exposed through nsIEncodedChannel? Would this
essentially be the value of network.http.accept-encoding?

The method would accept the value from PeekHeader(nsHttp::Content_Encoding) and
return true/false if the encoding is supported or not.
> The method would accept the value from PeekHeader(nsHttp::Content_Encoding) and

wouldn't it be a boolean readonly attribute?
What biesi said.  All the consumer wants to know is whether this particular
channel can handle its data.  If we put the method elsewhere, then we'd have to
pass a string, of course....
Excuse me while I miss the sky, but does this even come close to what you're
asking for?
I need to query the conversionSupported value in my OnStartRequest and decide
based on that whether to set applyConversion to true or false.  So you need to
set mConversionSupported before calling OnStartRequest.

Also, I'm not sure what "null" means for a boolean.  They should be PR_TRUE or
PR_FALSE.
you should also use : 1 in the header change, to save on space.
Attached patch patches the sky — — Splinter Review 
Right ... I wasn't sure what objects you'd have access to, so I just tried to
patch the sky.

All objects can be null in Java, it's just something that's hard to shake off
:-P
Attachment #197415 - Attachment is obsolete: true
Comment on attachment 197521 [details] [diff] [review]
patches the sky

- you need to change the IID of all interfaces you are changing
- why are you changing nsIStreamConverter here, rather than
nsIStreamConverterService? And if you do, why don't you need to change all
implementations of it?
- the Content-Encoding header is a list of encodings, not necessarily just a
single one...

Oh, urg, ApplyContentConversions screws this up too :( at least
GetContentEncodings doesn't...

+	     serv->SupportsConversion(from.get(), "uncompressed", &result);

could make this:
  return serv->SupportsConversion(from.get(), "uncompressed", value);

+nsStreamConverterService::SupportsConversion(const char *aFromType,
what's the point of your result variable?

@@ -114,6 +138,9 @@ nsHTTPCompressConv::AsyncConvertData(con
+    else
+	 return NS_ERROR_INVALID_CONTENT_ENCODING;

Hmm, is this reachable? Shouldn't the stream converter service only instantiate
this class for one of the supported conversions?


There is no point in nullchecking out variables, please don't do that.
> All objects can be null in Java

bool variables and such can't be null in java either :-P
(In reply to comment #19)
> > All objects can be null in Java
> 
> bool variables and such can't be null in java either :-P

Not primitives, no, but the 'Boolean' object can, and 'PRBool' sounds like an
object to me.
(In reply to comment #18)
Attachment 197521 [details] [diff] is just a "would this do what you wanted?" question (again).
Obviously Mozilla would never run with that patch.

In retrospect, the changes to nsIStreamConverter aren't really needed or useful
at all, I guess.
Is bug 318836 a dupe of this?
No, this is a bug in our code, while that bug's url is just broken...
Severity: normal → major
Flags: blocking1.9a1+
*** Bug 313371 has been marked as a duplicate of this bug. ***
Summary: bzip2-encoded content can no longer be saved → bzip2-encoded (unsupported content-encoding) content can no longer be saved
Vidar, any eta on this?
(In reply to comment #25)
> Vidar, any eta on this?

I won't be doing any coding for a long time :(
Do we have any timeframe for 1.9?
I think we want this bug fixed by summer at the latest...
FWIW, I just created a small patch to thttpd and a test file with the following results:

$ wget  -S http://localhost:9091/banner.txt.bz2
--20:05:55--  http://localhost:9091/banner.txt.bz2
           => `banner.txt.bz2.1'
Resolving localhost... 127.0.0.1
Connecting to localhost|127.0.0.1|:9091... connected.
HTTP request sent, awaiting response... 
  HTTP/1.0 200 OK
  Server: thttpd/2.25b 29dec2003
  Content-Type: text/plain; charset=iso-8859-1
  Date: Sun, 05 Mar 2006 19:05:55 GMT
  Last-Modified: Sat, 31 Dec 2005 21:35:11 GMT
  Accept-Ranges: bytes
  Connection: close
  Content-Encoding: x-bzip2
  Content-Length: 234
Length: 234 [text/plain]

When viewing this in Firefox 1.5 or SeaMonkey 2006020301, I get an "unsupported compression" page and not, as expected, a dialogue that allows me to at least select to store the file on disk.
In my opinion, the behavior in comment 28 is ok for now.  It's certainly not what this bug is about.
Blocks: 357958
Flags: blocking1.9a1+
This got fixed by the checkin for bug 357958.
No longer blocks: 357958
Status: NEW → RESOLVED
Closed: 18 years ago
Depends on: 357958
Flags: in-testsuite?
Resolution: --- → FIXED
Hurray, it works! No IE anymore... :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: