Bug 237052 - Prevent use of PDFKit when using PDF.js
Summary: Prevent use of PDFKit when using PDF.js
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: pascoe@apple.com
URL:
Keywords: InRadar
Depends on:
Blocks: 235969
  Show dependency treegraph
 
Reported: 2022-02-22 13:10 PST by pascoe@apple.com
Modified: 2022-02-23 11:04 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.59 KB, patch)
2022-02-22 13:11 PST, pascoe@apple.com
no flags Details | Formatted Diff | Diff
Patch (2.62 KB, patch)
2022-02-22 14:53 PST, pascoe@apple.com
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (4.40 KB, patch)
2022-02-22 15:23 PST, pascoe@apple.com
no flags Details | Formatted Diff | Diff
Patch for landing (4.00 KB, patch)
2022-02-23 09:31 PST, pascoe@apple.com
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description pascoe@apple.com 2022-02-22 13:10:18 PST
PDFKit is still used when PDF.js is enabled for embeds. This bug is to fix that issue.
Comment 1 pascoe@apple.com 2022-02-22 13:10:23 PST
rdar://89251696
Comment 2 pascoe@apple.com 2022-02-22 13:11:48 PST
Created attachment 452897 [details]
Patch
Comment 3 Simon Fraser (smfr) 2022-02-22 14:13:36 PST
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.
Comment 4 Tim Nguyen (:ntim) 2022-02-22 14:14:25 PST
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)
Comment 5 pascoe@apple.com 2022-02-22 14:53:54 PST
Created attachment 452905 [details]
Patch
Comment 6 pascoe@apple.com 2022-02-22 14:54:33 PST
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.
Comment 7 pascoe@apple.com 2022-02-22 15:23:16 PST
Created attachment 452906 [details]
Patch
Comment 8 Tim Horton 2022-02-22 16:34:44 PST
(In reply to j_pascoe@apple.com 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.
Comment 9 Tim Nguyen (:ntim) 2022-02-22 22:26:05 PST
Comment on attachment 452906 [details]
Patch

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

> Source/WebKit/ChangeLog:29
> +2022-02-22  J Pascoe  <j_pascoe@apple.com>
> +
> +        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):
> +
> +2022-02-22  J Pascoe  <j_pascoe@apple.com>
> +
> +        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
Comment 10 Tim Horton 2022-02-22 22:49:45 PST
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?
Comment 11 pascoe@apple.com 2022-02-23 09:31:17 PST
(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.
Comment 12 pascoe@apple.com 2022-02-23 09:31:53 PST
Created attachment 452990 [details]
Patch for landing
Comment 13 EWS 2022-02-23 11:04:05 PST
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].