Bug 25352 - REGRESSION (r42322): style isn't applied at bmwusa.com due to Content-Type with comma in it
Summary: REGRESSION (r42322): style isn't applied at bmwusa.com due to Content-Type wi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Darin Adler
URL: http://www.bmwusa.com
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2009-04-23 17:21 PDT by Alice Liu
Modified: 2009-05-02 14:16 PDT (History)
5 users (show)

See Also:


Attachments
first cut at patch, no regression test (2.28 KB, patch)
2009-04-24 12:46 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch (8.91 KB, patch)
2009-05-02 14:05 PDT, Darin Adler
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alice Liu 2009-04-23 17:21:07 PDT
style isn't applied at bmwusa.com
regression caused by http://trac.webkit.org/changeset/42322
Comment 1 Mark Rowe (bdash) 2009-04-23 20:09:34 PDT
<rdar://problem/6823239>
Comment 2 Adam Barth 2009-04-23 23:43:03 PDT
Sad face.  What Content-Type header are they serving their CSS with?  I won't be able to investigate this until Sunday.
Comment 3 Darin Adler 2009-04-24 12:36:39 PDT
(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
Comment 5 Darin Adler 2009-04-24 12:46:28 PDT
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?
Comment 6 Darin Adler 2009-05-01 17:24:10 PDT
Adam, any comments?
Comment 7 Adam Barth 2009-05-01 17:54:18 PDT
(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.
Comment 8 Darin Adler 2009-05-02 11:48:34 PDT
(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.
Comment 9 Adam Barth 2009-05-02 12:28:14 PDT
(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.
Comment 10 Adam Barth 2009-05-02 12:43:12 PDT
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
Comment 11 Darin Adler 2009-05-02 12:57:07 PDT
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.
Comment 12 Adam Barth 2009-05-02 13:05:16 PDT
(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

Comment 13 Alexey Proskuryakov 2009-05-02 13:11:50 PDT
> 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.
Comment 14 Darin Adler 2009-05-02 14:05:11 PDT
Created attachment 29960 [details]
patch
Comment 15 Darin Adler 2009-05-02 14:16:12 PDT
http://trac.webkit.org/changeset/43149