Closed
Bug 184144
Opened 22 years ago
Closed 19 years ago
Blank page and no error message a page with encoding gzip is not compressed
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
People
(Reporter: johann.petrak, Assigned: vhaarr+bmo)
References
(Blocks 1 open bug, )
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 4 obsolete files)
20.86 KB,
patch
|
vhaarr+bmo
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
Seeing this with 2002120422/linux, but this has been reported with other builds and under other OS too. The web site http://www.dnbscene.com seems to send a page with a header Content-Encoding: gzip when Mozilla sends a header Accept-Encoding: gzip,deflate,compress;q=0.9 (which is the default). But the subsequent content is not compressed, but plain text. Mozilla just shows an empty page and no error message, which is very confusing, since other browsers show the page just fine. Adding the line user_pref("network.http.accept-encoding","compress;q=0.9"); in user.js makes the problem go away.
Reporter | ||
Comment 1•22 years ago
|
||
Related bug 144814
Comment 2•22 years ago
|
||
This is a general issue... what do we do if a stream listener fails to deal with the data?
Comment 3•22 years ago
|
||
the stream converter probably cancels the channel with a non-specific error code like NS_ERROR_FAILURE (i'm just guessing here), and docshell ignores non-specific error codes (this is really a bug, but there are too many such errors passed to docshell that alerting the user would be very annoying... error pages will solve this problem though).
Reporter | ||
Comment 4•22 years ago
|
||
So it is really two issues - the error code should give information about the error instead of being nonspecific, and it should be communicated to the user in some way. Since Mozilla is often used by developers, I think it would be important to have informative error messages and as few cases of silent failure as possible.
Comment 5•22 years ago
|
||
> Since Mozilla is often used by developers, I think it would be important to
have informative error messages and as few cases of silent failure as possible.
Indeed.
Also a user sending a developer a mail with "your page has this and this critial
error according to mozilla" instead of a "um, your page doesn't show in my
browser for unknown reason" makes a difference.
In the first case the developer is probably going to fix it, in the second the
user might get a reply like "sorry, probably a browserbug, use IE instead".
Comment 6•22 years ago
|
||
Luckily, nsIStreamConverter is not frozen.. we could certainly add an error code to denote "conversion failed" or even something more specific. That may be a good idea. Stream converters were sorta next on my hit list anyway... ;)
Comment 7•22 years ago
|
||
With a view to providing useful feedback to the user as to why a content-encoding has failed, can someone point me in the direction of some instructions, examples, etc of mozilla sending such messages to the user? I'm ok with the content-encoding stuff now, so if someone could point me in the right direction for actually communicating the the user?
Comment 8•22 years ago
|
||
I'd think the best place to add this stuff would be over in nsWebShell::EndPageLoad. See the code starting at http://lxr.mozilla.org/seamonkey/source/docshell/base/nsWebShell.cpp#984 The real work is pretty much all handled by DisplayLoadError -- you may have to plug a string or two into a properties file. This function is called from nsDocShell::OnStateChange when it gets STATE_STOP. So it should be sufficient to make sure that the status for the STATE_STOP is an error status and then catch it in EndPageLoad... It looks like the status here is the loadgroup status, which should be coming from the channel that has the LOAD_DOCUMENT_URI flag (which is exactly what you want). So the issue is that the request would need to report an error somehow if a listener failed... perhaps error returns from OnDataAvailable should terminate the load and save the error? Alternately, the converter itself could Cancel() the request with the proper status.... (this last sounds best to me as far as not changing any API-stuff in frozen-API land, but it would be cool if channels noticed OnDataAvailable erroring out....)
Comment 10•22 years ago
|
||
Patch to warn for invalid gzip and deflate encoding methods. Servers that produce bzipped, compress(1)ed, etc content encodings will still not be handled well. Anyone got any ideas on how to handle content encodings that have not been requested?
Comment 11•22 years ago
|
||
Comment on attachment 122387 [details] [diff] [review] content-encoding error messages I'm not sure the wording of the error message is optimal; perhaps we can come up with something that even people who have no understanding of http could understand....
Attachment #122387 -
Flags: superreview?(darin)
Attachment #122387 -
Flags: review?(adamlock)
Comment 12•22 years ago
|
||
how about this: NS_ERROR_MALFORMED_CONTENT and simple error text like this: "The page is encoded in a format the browser does not understand."
Comment 13•22 years ago
|
||
I think Comment 5 has a valid point. We should let people know that mozilla is acting correctly (of course :)) and the site is issuing weird content encoding commands. Also, i think (and of course am open to other opinions) that people might understand the word compression better than anything to do with the more accurate content encoding. I would include academic references to back up this opinion, but...
Attachment #122387 -
Attachment is obsolete: true
Comment 14•22 years ago
|
||
Yeah, we definitely want to make it clear that the problem lies with the site here....
Comment 15•22 years ago
|
||
Comment on attachment 122387 [details] [diff] [review] content-encoding error messages r=adamlock on the docshell side. The network part looks okay too as far as I can tell.
Attachment #122387 -
Flags: review?(adamlock) → review+
Comment 16•22 years ago
|
||
Comment on attachment 122387 [details] [diff] [review] content-encoding error messages yeah, i agree... my suggested error text isn't what we want here. was too early in the morning! ;-) >Index: docshell/base/nsDocShell.cpp >+ case NS_ERROR_CONTENT_ENCODING: but i still think that this error code's name should include the word "MALFORMED" or something along those lines. what's erroneous about a content encoding? NS_ERROR_CONTENT_ENCODING doesn't make sense to me. how about NS_ERROR_MALFORMED_CONTENT_CODING? the HTTP/1.1 standard speaks of "content codings" rather than "content encodings". or if we really want to be specific about compression, then how about: NS_ERROR_MALFORMED_COMPRESSION should this error also be generated if nsHttpChannel cannot create a stream converter for the given Content-Encoding header? that wouldn't exactly be a case of malformed compression, but it would be included in the cases indicated by the error text below. >Index: docshell/resources/locale/en-US/appstrings.properties >+contentEncodingFailed=The site has used a broken or unknown form of compression that the browser could not handle. The browser has correctly advertised to the site the forms of compression that it can handle, so the site needs to correct it's compression methods and/or policies. how about this instead: compressionError=The page you are trying to view cannot be shown because it uses an invalid or unsupported form of compression. Please contact the website to inform them of this problem. >Index: netwerk/streamconv/converters/nsHTTPCompressConv.h >+#include "nsNetError.h" add this to the .cpp file instead. those who include nsHTTPCompressConv.h do not need to include nsNetError.h as well.
Attachment #122387 -
Flags: superreview?(darin) → superreview-
Comment 17•22 years ago
|
||
Agree that we should also present this error message for content-encodings that we do not understand, say compress(1) or bzip. As previously stated in comment 10, i am not sure how/where to go about this. Suggestions would be welcome.
Attachment #122409 -
Attachment is obsolete: true
Comment 18•22 years ago
|
||
Probably nsHttpChannel::ApplyContentConversions -- that's where the IsAcceptableEncoding() test would fail if the encoding is not one we accept. Not sure what we should do if we fail to get the stream converter service there....
Comment 19•21 years ago
|
||
page in url field seems to work now, replacing with new testcase
Updated•20 years ago
|
Whiteboard: [good first bug]
Comment 20•20 years ago
|
||
*** Bug 217924 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 21•19 years ago
|
||
Same as attachment 122612 [details] [diff] [review] updated to trunk and with bz's suggestion in comment 18 implemented.
Attachment #122612 -
Attachment is obsolete: true
Attachment #195535 -
Flags: review?(darin)
Comment 22•19 years ago
|
||
Comment on attachment 195535 [details] [diff] [review] with error page if IsAcceptableEncoding() fails. >Index: netwerk/protocol/http/src/nsHttpChannel.cpp >+ } else if (val != nsnull) { >+ return NS_ERROR_MALFORMED_CONTENT_ENCODING; > } In this case the content encoding is actually not malformed. It is just unsupported by the browser. Perhaps we should change the name of the error code to something less specific. How about this: NS_ERROR_INVALID_CONTENT_ENCODING Yeah, I like that better. r=darin with that change.
Attachment #195535 -
Flags: review?(darin) → review+
Assignee | ||
Comment 23•19 years ago
|
||
I'll comment in bug 301208 about the new error message in case they want to give it special review.
Assignee: darin → vhaarr+bmo
Attachment #195535 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #196134 -
Flags: superreview?(bzbarsky)
Attachment #196134 -
Flags: review+
Assignee | ||
Comment 24•19 years ago
|
||
Maybe <http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#1130> should use NS_ERROR_INVALID_CONTENT_ENCODING ?
Comment 25•19 years ago
|
||
Comment on attachment 196134 [details] [diff] [review] MALFORMED -> INVALID sr=bzbarsky
Attachment #196134 -
Flags: superreview?(bzbarsky) → superreview+
Comment 26•19 years ago
|
||
Comment on attachment 196134 [details] [diff] [review] MALFORMED -> INVALID Checked this in on trunk. Please mark fixed if it is and request 1.8 branch approval if you feel this is safe and really needed on branch....
Assignee | ||
Comment 27•19 years ago
|
||
Thanks, bz! I don't think this is needed for branch, but Darin would be a better judge of that. Resolving FIXED for now, please reopen if this is wanted for 1.8.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•