Bug 115487 – Move knowledge of PDF/PostScript MIME types into MIMETypeRegistry

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

[Tim Horton]

Right now we have this list of MIME types ineffectively and incorrectly duplicated in multiple places, and it's only going to get worse.

WebCore has a place to put this knowledge, so let's do it!

[Tim Horton]

Created attachment 200237 [details]
patch

[Darin Adler]

Comment on attachment 200237 [details]
patch

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

> Source/WebCore/platform/MIMETypeRegistry.cpp:361
> +    static const char* types[] = {
> +        "application/pdf",
> +        "text/pdf",
> +        "application/postscript",
> +    };

Why is this marked static? Since the function is called only once, I don’t know if that’s beneficial. Also could make this more const, const char* const, but that probably doesn’t matter; it’s usually not important if a local variable is const.

> Source/WebKit2/UIProcess/WebFrameProxy.cpp:118
> -    return WebContext::pdfAndPostScriptMIMETypes().contains(m_MIMEType);
> +    return MIMETypeRegistry::isPDFOrPostScriptMIMEType(m_MIMEType);

Old code was case folding, but the new function is not case folding. Is that OK? Does something ensure that the letters in m_MIMEType are lowercase?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:559
> -        if ((parameters.mimeType == "application/pdf" || parameters.mimeType == "application/postscript")
> -            || (parameters.mimeType.isEmpty() && (path.endsWith(".pdf", false) || path.endsWith(".ps", false)))) {
> +        if (MIMETypeRegistry::isPDFOrPostScriptMIMEType(parameters.mimeType) || (parameters.mimeType.isEmpty() && (path.endsWith(".pdf", false) || path.endsWith(".ps", false)))) {

Old code was case folding, but the new function is not case folding. Is that OK? Can someone pass WebPage::createPlugin a MIME type that contains uppercase letters?

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 200237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200237&action=review
> 
> > Source/WebCore/platform/MIMETypeRegistry.cpp:361
> > +    static const char* types[] = {
> > +        "application/pdf",
> > +        "text/pdf",
> > +        "application/postscript",
> > +    };
> 
> Why is this marked static? Since the function is called only once, I don’t know if that’s beneficial. Also could make this more const, const char* const, but that probably doesn’t matter; it’s usually not important if a local variable is const.

Good points.

> > Source/WebKit2/UIProcess/WebFrameProxy.cpp:118
> > -    return WebContext::pdfAndPostScriptMIMETypes().contains(m_MIMEType);
> > +    return MIMETypeRegistry::isPDFOrPostScriptMIMEType(m_MIMEType);
> 
> Old code was case folding, but the new function is not case folding. Is that OK? Does something ensure that the letters in m_MIMEType are lowercase?

I *think* it's OK. I don't think anything guarantees that m_MIMEType is lowercase -- in our case, it comes straight from NSURLResponse, never getting lowercased along the way. I have no idea if NSURLResponse guarantees lower-casing, but even if it did, I can't make a guarantee about other platforms.

Here's why I'm not horribly worried, though: everything else that compares against m_MIMEType compares in a case sensitive way (isDisplayingStandaloneImageDocument and isDisplayingMarkupDocument, specifically).

Do you think we should explicitly lowercase the characters earlier on?

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:559
> > -        if ((parameters.mimeType == "application/pdf" || parameters.mimeType == "application/postscript")
> > -            || (parameters.mimeType.isEmpty() && (path.endsWith(".pdf", false) || path.endsWith(".ps", false)))) {
> > +        if (MIMETypeRegistry::isPDFOrPostScriptMIMEType(parameters.mimeType) || (parameters.mimeType.isEmpty() && (path.endsWith(".pdf", false) || path.endsWith(".ps", false)))) {
> 
> Old code was case folding, but the new function is not case folding. Is that OK? Can someone pass WebPage::createPlugin a MIME type that contains uppercase letters?

WebPageProxy::FindPlugin (called from WebPage::createPlugin and handed that same MIME type) also compares in a case-sensitive way, so I think things would already be broken. In addition, before it gets to WebPage::createPlugin, it's compared in a case-sensitive way (in various places in SubframeLoader).

It looks like createPlugin eventually gets its mimeType argument from HTMLEmbedElement and HTMLObjectElement. HTMLEmbedElement explicitly lower-cases the MIME type. HTMLObjectElement appears to also lower case its MIMETypes *except* if they come from a data-URL. So that case might be broken, but it's already broken.

But, again, maybe we should improve the existing situation? Or maybe that should be a separate bug.

[Tim Horton]

I think I'll make mine continue to be case-folding just to be safe, and file a fix me to investigate the other cases (especially the object-data-url case).

[Tim Horton]

Err, leave a fixme and file a bug.

[Tim Horton]

(In reply to comment #4)
> I think I'll make mine continue to be case-folding just to be safe, and file a fix me to investigate the other cases (especially the object-data-url case).

I'm actually just going to fix that, since it's easy (and is actually broken).

[Tim Horton]

(In reply to comment #6)
> (In reply to comment #4)
> > I think I'll make mine continue to be case-folding just to be safe, and file a fix me to investigate the other cases (especially the object-data-url case).
> 
> I'm actually just going to fix that, since it's easy (and is actually broken).

https://bugs.webkit.org/show_bug.cgi?id=115494

[Tim Horton]

(In reply to comment #4)
> I think I'll make mine continue to be case-folding just to be safe

Actually, I think this is wrong. We should fix cases where case-insensitive MIME types make it into WebCore, because there fewer of those than there are places that do The Wrong Thing(TM) and compare to lower-case strings.

I'm going to leave the patch as is, but we should keep looking out for things like https://bugs.webkit.org/show_bug.cgi?id=115494.

Thanks, Darin!

[Tim Horton]

(In reply to comment #8)
> (In reply to comment #4)
> > I think I'll make mine continue to be case-folding just to be safe
> 
> Actually, I think this is wrong. We should fix cases where case-insensitive MIME types make it into

s/case-insensitive/non-lower-cased/

[Tim Horton]

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

[Darin Adler]

(In reply to comment #8)
> We should fix cases where case-insensitive MIME types make it into WebCore, because there fewer of those than there are places that do The Wrong Thing(TM) and compare to lower-case strings.

OK.

[Darin Adler]

Comment on attachment 200237 [details]
patch

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

>>> Source/WebKit2/UIProcess/WebFrameProxy.cpp:118
>>> +    return MIMETypeRegistry::isPDFOrPostScriptMIMEType(m_MIMEType);
>> 
>> Old code was case folding, but the new function is not case folding. Is that OK? Does something ensure that the letters in m_MIMEType are lowercase?
> 
> I *think* it's OK. I don't think anything guarantees that m_MIMEType is lowercase -- in our case, it comes straight from NSURLResponse, never getting lowercased along the way. I have no idea if NSURLResponse guarantees lower-casing, but even if it did, I can't make a guarantee about other platforms.
> 
> Here's why I'm not horribly worried, though: everything else that compares against m_MIMEType compares in a case sensitive way (isDisplayingStandaloneImageDocument and isDisplayingMarkupDocument, specifically).
> 
> Do you think we should explicitly lowercase the characters earlier on?

I do not think that NSURLResponse guarantees lower-casing. We should figure out where to put the call to lower() to fix that.

[Tim Horton]

(In reply to comment #12)
> (From update of attachment 200237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200237&action=review
> > Do you think we should explicitly lowercase the characters earlier on?
> 
> I do not think that NSURLResponse guarantees lower-casing. We should figure out where to put the call to lower() to fix that.

Agreed. Ideally, somewhere far enough away from NSURLResponse that it works for other platforms too.

[Tim Horton]

(In reply to comment #12)
> (From update of attachment 200237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200237&action=review
> 
> >>> Source/WebKit2/UIProcess/WebFrameProxy.cpp:118
> >>> +    return MIMETypeRegistry::isPDFOrPostScriptMIMEType(m_MIMEType);
> >> 
> >> Old code was case folding, but the new function is not case folding. Is that OK? Does something ensure that the letters in m_MIMEType are lowercase?
> > 
> > I *think* it's OK. I don't think anything guarantees that m_MIMEType is lowercase -- in our case, it comes straight from NSURLResponse, never getting lowercased along the way. I have no idea if NSURLResponse guarantees lower-casing, but even if it did, I can't make a guarantee about other platforms.
> > 
> > Here's why I'm not horribly worried, though: everything else that compares against m_MIMEType compares in a case sensitive way (isDisplayingStandaloneImageDocument and isDisplayingMarkupDocument, specifically).
> > 
> > Do you think we should explicitly lowercase the characters earlier on?
> 
> I do not think that NSURLResponse guarantees lower-casing. We should figure out where to put the call to lower() to fix that.

Filed https://bugs.webkit.org/show_bug.cgi?id=115501.