Bug 74800 – Perform case insensitive matching on MIME types in XHR

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jarred Nicholls]

Cache lowercased responseMIMEType and responseIsXML in XHR

[Jarred Nicholls]

Created attachment 119747 [details]
Patch

[Jarred Nicholls]

Created attachment 119748 [details]
Patch

[Jarred Nicholls]

Created attachment 119749 [details]
Patch

[Darin Adler]

Comment on attachment 119749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119749&action=review

> Source/WebCore/xml/XMLHttpRequest.h:203
> +    String m_responseMIMEType;

Why keep this around at all? I think that all that matters is: isHTML, isXML, and other. There is no reason to keep the actual MIME type. Am I missing something?

[Darin Adler]

Comment on attachment 119749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119749&action=review

>> Source/WebCore/xml/XMLHttpRequest.h:203
>> +    String m_responseMIMEType;
> 
> Why keep this around at all? I think that all that matters is: isHTML, isXML, and other. There is no reason to keep the actual MIME type. Am I missing something?

Why make the object bigger? Is this a measurable speed optimization? Is this for code clarity? This seems a bit like code churn for no real benefit.

[Jarred Nicholls]

> > Source/WebCore/xml/XMLHttpRequest.h:203
> > +    String m_responseMIMEType;
> 
> Why keep this around at all? I think that all that matters is: isHTML, isXML, and other. There is no reason to keep the actual MIME type. Am I missing something?

There's no reason to keep it, today.  I'm good with a couple of bits or an enum.

> Why make the object bigger? Is this a measurable speed optimization? Is this for code clarity? This seems a bit like code churn for no real benefit.

There's no good reason for keeping it, other than it might be useful later.  I'd prefer to do the MIME checks one time up front regardless.

Thanks.

[Darin Adler]

Comment on attachment 119749 [details]
Patch

The code we have today works fine. It uses the response we are already holding. Computing and storing additional state to speed things up later only seems worth doing if the speed difference is measurable.

[Jarred Nicholls]

(In reply to comment #7)
> (From update of attachment 119749 [details])
> The code we have today works fine. It uses the response we are already holding. Computing and storing additional state to speed things up later only seems worth doing if the speed difference is measurable.

This has nothing to do with performance.  We are currently doing case sensitive matching on MIME types without lowercasing the MIME type first.  If the MIME type is "text/XML" or someone calls overrideMimeType('text/XML') then the match will fail.  The caching was a nicety to avoid lowercasing multiple times, but the bug is actually about doing matching on a lowercased MIME string.

We can forget about caching altogether, but the lowercasing needs to happen.

[Darin Adler]

(In reply to comment #8)
> We can forget about caching altogether, but the lowercasing needs to happen.

Makes sense.

As a side note, I would say that “case insensitive comparison of MIME types” needs to happen. Normally we would not convert to lowercase to do that.

Code like x == "text/html" can be changed to equalIgnoringCase(x, "text/html").

But when I researched DOMImplementation::isXMLMIMEType I was surprised to find out it works in a case sensitive manner. It seems to me that functions like DOMImplementation::createDocument, DOMImplementation::isTextMIMEType, and DOMImplementation::isXMLMIMEType should be ignoring case and I am not sure why they are not. XMLMIMETypeRegExp even seems to go out of its way to be case sensitive.

If you are concentrating on XMLHttpRequest, I understand if you want to add a call to make the MIME type be lowercase, but overall for the project, that is the wrong direction. I can’t see why we’d even want to check MIME types in a case sensitive way and I think that DOMImplementation code that does so is suspect.

[Darin Adler]

even want -> ever want

[Darin Adler]

Since the patch is fixing a bug, not just refactoring for better performance or clarity, we need to land a test case with it, one that would show the old incorrect behavior and also shows the new correct behavior. We have machinery for doing MIME type tests in the http subdirectory of the LayoutTest automated test machinery, and people with experience with that can help you make a test.

[Jarred Nicholls]

> But when I researched DOMImplementation::isXMLMIMEType I was surprised to find out it works in a case sensitive manner. It seems to me that functions like DOMImplementation::createDocument, DOMImplementation::isTextMIMEType, and DOMImplementation::isXMLMIMEType should be ignoring case and I am not sure why they are not. XMLMIMETypeRegExp even seems to go out of its way to be case sensitive.

This is why I decided to lowercase in XHR.responseMIMEType, to handle both the DOMImplementation::isXMLMIMEType call, as well as a MIME type provided by the user via overrideMimeType, in one spot.

> 
> If you are concentrating on XMLHttpRequest, I understand if you want to add a call to make the MIME type be lowercase, but overall for the project, that is the wrong direction. I can’t see why we’d even want to check MIME types in a case sensitive way and I think that DOMImplementation code that does so is suspect.

I agree, and I'm willing to iron out those issues in DOMImplementation et al in a separate bug.

> Since the patch is fixing a bug, not just refactoring for better performance or clarity, we need to land a test case with it, one that would show the old incorrect behavior and also shows the new correct behavior. We have machinery for doing MIME type tests in the http subdirectory of the LayoutTest automated test machinery, and people with experience with that can help you make a test.

True, in addition to a fast test for overrideMimeType().

Thanks!

[Jarred Nicholls]

Created attachment 119862 [details]
Patch

[Darin Adler]

Comment on attachment 119862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119862&action=review

Patch is OK, but I’d like to see one that takes a different approach.

> Source/WebCore/xml/XMLHttpRequest.cpp:227
> -        bool isHTML = equalIgnoringCase(responseMIMEType(), "text/html");
> +        bool isHTML = responseMIMEType() == "text/html";

Lets not change this. I don’t want to move to the approach where we lowercase the MIME type.

Instead, we should use equalIgnoringCase in XMLHttpRequest::didReceiveData and work around the isXMLMIMEType issue.

> Source/WebCore/xml/XMLHttpRequest.cpp:932
> +        mimeType.makeLower();

I see this as a workaround for a bug in DOMImplementation::isXMLMIMEType so:

1) The lowercasing should be done inside XMLHttpRequest::responseIsXML.
2) A comment should mention that it's a workaround for that bug.
3) We should remove the lowercasing when we fix DOMImplementation::isXMLMIMEType.

[Jarred Nicholls]

> I see this as a workaround for a bug in DOMImplementation::isXMLMIMEType so:
> 
> 1) The lowercasing should be done inside XMLHttpRequest::responseIsXML.
> 2) A comment should mention that it's a workaround for that bug.
> 3) We should remove the lowercasing when we fix DOMImplementation::isXMLMIMEType.

You got it, thanks.

[Jarred Nicholls]

Created attachment 119903 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119903 [details]
Patch

Clearing flags on attachment: 119903

Committed r103344: <http://trac.webkit.org/changeset/103344>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.