789077 - Since Bug 567077 landed, .xhtml files encoded as UTF-16 Little Endian are not displayed, but instead offered for download as MP3 files.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[xeno42alpha]

Steps to reproduce:
Open a .xhtml file.

Actual results:
The file gets offered for download as a MP3 file.

Expected results:
The .xhtml file is displayed as a webpage.
------------------------------------------------------------
Summary: 
After I updated Nightly to 18.0a1 (2012-09-06), I tried to open a local .xhtml file I was working on. Instead getting displayed as a web page, it was offered for download as an MP3 file. 
Trying to open http://www.rialgar.de/ got the same result.
------------------------------------------------------------
Additional Information:
Response from Server:
HTTP/1.1 200 OK
Date: Thu, 06 Sep 2012 15:41:30 GMT
Server: Apache
Last-Modified: Sat, 25 Aug 2012 14:33:24 GMT
ETag: "1c7348cb-1a40-4c817f995f500"
Accept-Ranges: bytes
Content-Length: 6720
X-Powered-By: PleskLin
Keep-Alive: timeout=15, max=98
Connection: Keep-Alive
Content-Type: application/xhtml+xml

Comment 1

[xeno42alpha]

Attachment: Screenshot of Nightly 2012-09-05 displaying website.

Comment 2

[xeno42alpha]

Attachment: The same URI on Nightly 2012-09-06, file gets offered for download instead.

Comment 3

[Paul Adenot (:padenot)]

I can reproduce, but only on certain web pages. Most of them are not affected. I'll investigate right now.

Comment 4

[Ralph Giles (:rillian)]

Note that this occurs even when the site sends a mimetype:

$ curl --head http://rialgar.de/
...
Content-Type: application/xhtml+xml

Comment 5

[Ralph Giles (:rillian)]

rialgar.de uses a UTF-16 encoding, starting with the 0xfffe byte-order marker, which looks like it matches your 0xfffa,0xfffb mask pattern. You may need to look for the next header at the expected offset to reliably identify mp3.

Comment 6

[Ralph Giles (:rillian)]

So my idea doesn't work, because draft-ietf-websec-mime-sniff says the sniffer stops after 512 bytes, and an mp3 frame can be longer than that.

I checked 8 different mp3 files with 'iconv --from-code=UTF-16 --to-code=UTF-8 file.mp3 > /dev/null' and while all contained an invalid UTF-16 sequence, for several files it with more than 512 bytes in, so that doesn't work either.

Looks like mp3 sniffing will require some format research.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Remove sniffing for mp3 without ID3, because of false positive. r=

This patch removes the pattern for an mp3 file without ID3 tag, so we don't
break the web. Bug 789123 has been filed to add a decent mechanism to sniff for
and mp3 file without ID3 tags.

This patch also removes the test. It does not remove the file, since I have good
hope to be able to re-enable the test in the near future.

Comment 8

[Boris Zbarsky [:bzbarsky]]

But why are we sniffing at all if the server sent "Content-Type: application/xhtml+xml"?

Comment 9

[Paul Adenot (:padenot)]

bz, as far as my research went, [1] allows the overriding of the doctype using sniffers. I _guess_ this is the code path that is used when loading a page.

[1]: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9430

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, but the media sniffer is not supposed to do overriding, I would think.  It _can_ given how it hooks in, but it _shouldn't_.

Comment 11

[Paul Adenot (:padenot)]

Attachment: Sniff for a media only if the Content-Type is unknown, or octet-stream. r=

Here is a patch. As discussed on IRC, we return early if the type is not empty,
or if it is not application/octet-stream.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 658962 [details] [diff] [review]
Sniff for a media only if the Content-Type is unknown, or octet-stream. r=

As written, this will return if the type is nonempty.  As in, it'll break octet-stream sniffing (which I hope we have tests for!)

I think you want something more like this:

  if (!contentType.IsEmpty() &&
      !contentType.EqualsLiteral(APPLICATION_OCTET_STREAM) &&
      !contentType.EqualsLiteral(UNKNOWN_CONTENT_TYPE)) {
    return NS_ERROR_NOT_AVAILABLE;
  }

r=me with that.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 658927 [details] [diff] [review]
Remove sniffing for mp3 without ID3, because of false positive. r=

Review of attachment 658927 [details] [diff] [review]:
-----------------------------------------------------------------

Good. Can you make Boris's changes, upload the patch, and push to Try? Let's get this landed ASAP.

Comment 14

[xeno42alpha]

I ran some test with local files, results as following:
– UTF-8 without Byte Order Mark: works
– UTF-8 with Byte Order Mark: works
– UTF-16 Big Endian: works
– UTF-16 Little Endian: fails

Comment 15

[Paul Adenot (:padenot)]

Both patches (with bz's comment addressed) pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ac22cdf7b347

Comment 16

[Ralph Giles (:rillian)]

(In reply to xeno42alpha from comment #14)
> I ran some test with local files, results as following:
> – UTF-8 without Byte Order Mark: works
> – UTF-8 with Byte Order Mark: works
> – UTF-16 Big Endian: works
> – UTF-16 Little Endian: fails

Thanks for testing, that matches my results as well.

Comment 17

[khagaroth]

I have never seen a MP3 file with FFFE, it's always FFFA or more commonly FFFB. The only completely safe way to detect MP3 would be to find the first frame using the frame sync, read the header and compute the frame length and then search for the next frame at the specific offset, if it's there, it's MP3, if not, it isn't.

Comment 18

[Paul Adenot (:padenot)]

We investigated doing something like comment 17 describes, but unfortunately, the spec requires us to read at most 512 bytes, and we thought that an mp3 frame could be longer than that. Can someone confirm or infirm that?

We should discuss this in bug 789123, which is about implementing mp3 sniffing properly.

Comment 19

[Paul Adenot (:padenot)]

Attachment: Remove sniffing for mp3 without ID3, because of false positive.

This is green on try: https://tbpl.mozilla.org/?tree=Try&rev=ac22cdf7b347

Comment 20

[Paul Adenot (:padenot)]

Attachment: Sniff for a media only if the Content-Type is unknown, or octet-stream.

Updated commit message.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

(In reply to Paul ADENOT (:padenot) from comment #19)
> Created attachment 659392 [details] [diff] [review]
> This is green on try: https://tbpl.mozilla.org/?tree=Try&rev=ac22cdf7b347

Pushing directly to m-c for tomorrow's nightly.

https://hg.mozilla.org/mozilla-central/rev/882b5dcaed46
https://hg.mozilla.org/mozilla-central/rev/530da2e30cef

Should this have tests?

Comment 22

[Paul Adenot (:padenot)]

Ryan, this is a bug fix for a feature that already have tests.

On the other hands our test suite did check the behavior we want, i.e. that the media sniffer sniff only when we want it to sniff. I'll see to write a test for that part, I suppose we can do it with an xpcshell test.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

I figure that when things regress, there's a hole in our test coverage :)

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking bug 567077 in case there are more regressions we need to have awareness about but not tracking here since this is resolved.