Bug 25352 – REGRESSION (r42322): style isn't applied at bmwusa.com due to Content-Type with comma in it

• Component: CSS
• Status: RESOLVED FIXED

[Alice Liu]

style isn't applied at bmwusa.com
regression caused by http://trac.webkit.org/changeset/42322

[Mark Rowe (bdash)]

<rdar://problem/6823239>

[Adam Barth]

Sad face.  What Content-Type header are they serving their CSS with?  I won't be able to investigate this until Sunday.

[Darin Adler]

(In reply to comment #2)
> Sad face.  What Content-Type header are they serving their CSS with?  I won't
> be able to investigate this until Sunday.

Content-Type:text/css,200904131203

[Darin Adler]

URL is http://www.bmwusa.com/HttpCombiner.ashx?s=Set_Css&t=text/css&v=1&t=200904131203

[Darin Adler]

Created attachment 29755 [details]
first cut at patch, no regression test

I'd like to know more about other browsers' behavior. Is the comma the only difference between us and them?

[Darin Adler]

Adam, any comments?

[Adam Barth]

(In reply to comment #6)
> Adam, any comments?

This fix seems reasonable.  I haven't dug into the Firefox version of this code to see how the algorithm compares, however.

[Darin Adler]

(In reply to comment #7)
> I haven't dug into the Firefox version of this code
> to see how the algorithm compares, however.

I'd very much like to know that.

[Adam Barth]

(In reply to comment #8)
> (In reply to comment #7)
> > I haven't dug into the Firefox version of this code
> > to see how the algorithm compares, however.
> 
> I'd very much like to know that.

Ok.  I'll take a look.

[Adam Barth]

Looks like the work is done at http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsURLHelper.cpp#869.  That code has the following BNF as a comment:

878     // Augmented BNF (from RFC 2616 section 3.7):
879     //
880     //   header-value = media-type *( LWS "," LWS media-type )
881     //   media-type   = type "/" subtype *( LWS ";" LWS parameter )
882     //   type         = token
883     //   subtype      = token
884     //   parameter    = attribute "=" value
885     //   attribute    = token
886     //   value        = token | quoted-string
887     //   
888     //
889     // Examples:
890     //
891     //   text/html
892     //   text/html, text/html
893     //   text/html,text/html; charset=ISO-8859-1
894     //   text/html,text/html; charset="ISO-8859-1"
895     //   text/html;charset=ISO-8859-1, text/html
896     //   text/html;charset='ISO-8859-1', text/html
897     //   application/octet-stream

The code doesn't quite seem to follow this BNF.  For example, http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsURLHelper.cpp#766 treats the "(" character as special.  Also, media types of "*/*" and those without a "/" character are treated like the empty media type: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsURLHelper.cpp#823

[Darin Adler]

OK, this jogs my memory a bit.

The comma simply separates multiple field values. This can be used in any HTTP header field. So a Content-Type with a comma in it (outside of quotes) is actually two separate Content-Type header fields.

So one question is how we should handle multiple Content-Type header fields: Ignore all fields after the first? Ignore all fields we don’t know how to parse?

And of course we’d like to match the RFC as closely as possible and match Gecko as closely as possible too. But I think fixes involving characters other than the comma should be tracked by other bugs.

[Adam Barth]

(In reply to comment #11)
> So one question is how we should handle multiple Content-Type header fields:
> Ignore all fields after the first? Ignore all fields we don’t know how to
> parse?

I think Internet Explorer takes the first Content-Type header and Firefox takes the last (could have it backwards, but it's fairly easy to test).  I think we want to match Firefox here (which is also what Chrome does) because Firefox has stricter Content-Type processing than IE (and that means whichever header Firefox looks at is more likely to be what the web site actually means).

Note that the "must have a /" requirement is probably needed to make this case work properly.  We should test the following case to be sure:

Content-Type: text/css,image/png

[Alexey Proskuryakov]

> The comma simply separates multiple field values. This can be used in any HTTP
> header field.

Non-compliant servers can of course do this, but the HTTP spec doesn't allow it for Content-Type.

[Darin Adler]

Created attachment 29960 [details]
patch

[Darin Adler]

http://trac.webkit.org/changeset/43149