Bug 237052 – Prevent use of PDFKit when using PDF.js

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

[[e-mail]]

PDFKit is still used when PDF.js is enabled for embeds. This bug is to fix that issue.

[[e-mail]]

rdar://89251696

[[e-mail]]

Created attachment 452897 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 452897 [details]
Patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1709
> +        if (webPage->corePage()->settings().pdfJSViewerEnabled() && MIMETypeRegistry::isPDFOrPostScriptMIMEType(mimeType))
> +            return ObjectContentType::Frame;

The test says "isPDFOrPostScriptMIMEType" but PDF.js doesn't appear to be able to handle PostScript, so this doesn't seem quite right. Also, we should test PostScript if we don't already.

[Tim Nguyen (:ntim)]

Comment on attachment 452897 [details]
Patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1708
> +        if (webPage->corePage()->settings().pdfJSViewerEnabled() && MIMETypeRegistry::isPDFOrPostScriptMIMEType(mimeType))

Should this be `MIMETypeRegistry::isPDFMIMEType(mimeType)` ? PDF.js doesn't support PostScript: https://github.com/mozilla/pdf.js/issues/1594 (though we should figure out what to do in this case to avoid using PDFKit)

[[e-mail]]

Created attachment 452905 [details]
Patch

[[e-mail]]

Although there's references of PDFKit supposedly supporting Postscript, I haven't get it to render files locally, so it seems fine pdfjs doesn't either.

[[e-mail]]

Created attachment 452906 [details]
Patch

[Tim Horton]

(In reply to [e-mail] from comment #6)
> Although there's references of PDFKit supposedly supporting Postscript, I
> haven't get it to render files locally, so it seems fine pdfjs doesn't
> either.

Per Arne removed our PS->PDF translation a few weeks ago.

[Tim Nguyen (:ntim)]

Comment on attachment 452906 [details]
Patch

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

> Source/WebKit/ChangeLog:29
> [phone]  J Pascoe  <[e-mail]>
> +
> +        Prevent use of PDFKit when using PDF.js
> +        https://bugs.webkit.org/show_bug.cgi?id=237052
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
> +        (WebKit::WebFrameLoaderClient::objectContentType):
> +        * WebProcess/WebPage/mac/WebPageMac.mm:
> +        (WebKit::WebPage::shouldUsePDFPlugin const):
> +
> [phone]  J Pascoe  <[e-mail]>
> +
> +        Prevent use of PDFKit when using PDF.js
> +        https://bugs.webkit.org/show_bug.cgi?id=237052
> +        rdar://89251696
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        PDFKit was still being used by embeds, this patch fixes that issue by treating
> +        pdf embeds as frames if PDF.js is enabled and also prevents the loading of
> +        the PDFKit plugin entirely by modifying shouldUsePDFPlugin.
> +
> +        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
> +        (WebKit::WebFrameLoaderClient::objectContentType):
> +        * WebProcess/WebPage/mac/WebPageMac.mm:
> +        (WebKit::WebPage::shouldUsePDFPlugin const):
> +

nit: Double changelog

[Tim Horton]

Comment on attachment 452906 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:209
>      return pdfPluginEnabled()
> +        && !corePage()->settings().pdfJSViewerEnabled()

Does anyone call pdfPluginEnabled() (as opposed to shouldUsePDFPlugin) that might motivate you to move this check down a level?

[[e-mail]]

(In reply to Tim Horton from comment #10)
> Comment on attachment 452906 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452906&action=review
> 
> > Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:209
> >      return pdfPluginEnabled()
> > +        && !corePage()->settings().pdfJSViewerEnabled()
> 
> Does anyone call pdfPluginEnabled() (as opposed to shouldUsePDFPlugin) that
> might motivate you to move this check down a level?

No, there aren't other usages of pdfPluginEnabled() wrt using the plugin.

[[e-mail]]

Created attachment 452990 [details]
Patch for landing

[EWS]

Committed r290384 (247699@main): <https://commits.webkit.org/247699@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452990 [details].