394647 - [FIX]nsBinaryDetector::DetermineContentType is outdated

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Marco Pesenti Gritti]

Comment from the code:

// Make sure to do a case-sensitive exact match comparison here.  Apache
// 1.x just sends text/plain for "unknown", while Apache 2.x sends
// text/plain with a ISO-8859-1 charset.  Debian's Apache version, just to
// be different, sends text/plain with iso-8859-1 charset.  Don't do
// general case-insensitive comparison, since we really want to apply this
// crap as rarely as we can.

I tested it Apache with stock Fedora Core 7, RHEL 4 and Ubuntu Feisty. The header is now "text/plain; charset=UTF-8".

As a result all the unknown (to the Apache server) file types are opened inside firefox rather than downloaded by an external helper.

I'm not sure why we are checking the charset at all there, instead of just the mime type. Maybe someone more familiar with this code can explain it?

Comment 1

[Boris Zbarsky [:bzbarsky]]

The problem is that, say, Japanese text will reliably test as binary.  So if we just check the text/plain thing for any text/plain response anyone not using Western languages loses if they send plaintext files.

We can't even check for UTF-8, because then those non-Western language users still get screwed.  So I don't think this code will be changing.

Please file bugs on the relevant distributions to set their fallback MIME type to something that will get sniffed.  If this was a core Apache change, please file a bug on Apache too.

I suppose we could change the code to check the Server: header and just release note that people shouldn't host their content on Apache if they actually want to send non-Western plaintext files.  But that seems like a much more serious problem than the very few non-text files this is going to bite so far.... and we can keep hoping that Apache will fix their default type bug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, it looks like LastDitchSniff only looks for control characters, which I think would not appear in valid UTF-8.  So we might be able to do this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Christian, the test as written does the GETs one after another.  It could also easily do all the asyncOpens in a single loop and then wait for the server to respond to them all.  Let me know whether you have a preference, ok?

Comment 4

[Mike Schroepfer]

Biesi - ping - we'd like this for B2 which is coming up soon..

Comment 5

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

Comment on attachment 286496 [details] [diff] [review]
Like so

I don't have a preference for GET-in-a-loop vs all-at-once, use whatever is simpler :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Is this a full dupe of bug 261263, or only a subset of it?

Comment 8

[Boris Zbarsky [:bzbarsky]]

It has nothing at all to do with bug 261263, basically.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Well, I guess as that bug is initially filed this one sorta solves a similar problem, in some cases.

Comment 10

[Boris Zbarsky [:bzbarsky]]

This test made tinderbox time out... I disabled it for now, until robcee ups the timeout value a bit.  :(

Comment 11

[Boris Zbarsky [:bzbarsky]]

So even with the bigger timeout value this is failing.  I'm getting back an HTTP channel whose status is either NS_ERROR_ABORT or NS_ERROR_FAILURE.  Hard to tell from the actual output... :(

I have no idea why this is happening.  I'll try to hunt down a windows setup I can test in.