Bug 115487

Created attachment 200237 [details] patch

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?

(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.

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).

Err, leave a fixme and file a bug.

(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).

(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

(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!

(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/

(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.

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.

(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.

(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.