Bug 75790 - PDFPlugins don't load when plugins are disabled, but they should
Summary: PDFPlugins don't load when plugins are disabled, but they should
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P3 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on: 115487 116289
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-07 23:21 PST by Chris Petersen
Modified: 2013-05-16 22:03 PDT (History)
11 users (show)

See Also:


Attachments
preliminary patch (34.85 KB, patch)
2013-05-01 02:25 PDT, Tim Horton
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
changelogs (48.40 KB, patch)
2013-05-01 03:43 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch (35.92 KB, patch)
2013-05-03 16:38 PDT, Tim Horton
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Petersen 2012-01-07 23:21:34 PST
Inline PDF doesn't load when plugins are disabled, need better error message. Maybe by design but inline PDF don't load when plugins are disabled. Right now you see a message Loading... ( like you do when a MP3 as a <audio> element ). However, this message isn't very information.

1) Launch Webkit NB r104385 and disable Plugins in Safari 5.1.1
2) Go to http://www.cs.tut.fi/~jkorpela/html/iframe-pdf.html
3) Notice "Loading.." message appears in both iframes and PDF fails to appear ( No message why )
Comment 1 Alexey Proskuryakov 2012-01-08 00:02:26 PST
Looks like a PDF iframe gets (unsuccessfully) handled as a media document when plug-ins are disabled.
Comment 2 Tim Horton 2012-11-29 18:43:27 PST
<rdar://problem/11650197>
Comment 3 Tim Horton 2012-11-29 18:46:26 PST
The symptom has changed a bit, now you get garbage (raw PDF data) in the frame instead.
Comment 4 Tim Horton 2013-05-01 02:25:21 PDT
Created attachment 200208 [details]
preliminary patch

Rather surprisingly large patch that's not ready for review (no change log, for starters), but if someone takes offense with the way I'm doing this, feel free to let me know!
Comment 5 Early Warning System Bot 2013-05-01 02:30:40 PDT
Comment on attachment 200208 [details]
preliminary patch

Attachment 200208 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/356004
Comment 6 Early Warning System Bot 2013-05-01 02:31:24 PDT
Comment on attachment 200208 [details]
preliminary patch

Attachment 200208 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/301065
Comment 7 Build Bot 2013-05-01 02:58:53 PDT
Comment on attachment 200208 [details]
preliminary patch

Attachment 200208 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/271029
Comment 8 Tim Horton 2013-05-01 03:43:11 PDT
Created attachment 200211 [details]
changelogs

I still think it should be possible to do this better without complicating all of the WebKit* side call sites, by making PluginData and the plugin cache invalidate when the plugin pref is flipped, and then just depending on PluginData not including any non-app-plug-ins if the pref is off. However, I'm not sure that'll work since it looks like the FrameLoaderClient can override allowPlugins for arbitrary reasons, not just the state of the setting... so I'm not doing that for now. Let's see what others think.

Not sure I'm totally happy with stuff yet, but this could use a round of scathing review.
Comment 9 Tim Horton 2013-05-01 03:45:22 PDT
Comment on attachment 200211 [details]
changelogs

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:939
> +    if (MIMETypeRegistry::isPDFOrPostScriptMIMEType(mimeType) && !WebContext::omitPDFSupport())

FWIW I think it's OK that we dropped the case-folding here, as MIMETypeRegistry::canShowMIMEType above is also case-sensitive.
Comment 10 Tim Horton 2013-05-01 03:48:06 PDT
Probably this should be split into more patches:

1) MIMETypeRegistry changes and adoption in WK2.
2) PluginData isApplicationPlugin field and adoption in plugin enumerators.
3) PluginData always being created and findPlugin/createPlugin/etc. getting application-plug-in filters.
Comment 11 Tim Horton 2013-05-03 16:38:44 PDT
Created attachment 200507 [details]
patch
Comment 12 Anders Carlsson 2013-05-16 14:50:01 PDT
Comment on attachment 200507 [details]
patch

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

> Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:125
> +PluginModuleInfo PluginInfoStore::findPluginForMIMEType(const String& mimeType, bool onlyApplicationPlugins) const

Couldn’t this take a AllowedPluginTypes enum instead of a boolean?

> Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:145
> +PluginModuleInfo PluginInfoStore::findPluginForExtension(const String& extension, String& mimeType, bool onlyApplicationPlugins) const

Ditto.

> Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp:208
> +PluginModuleInfo PluginInfoStore::findPlugin(String& mimeType, const KURL& url, bool onlyApplicationPlugins)

Ditto.
Comment 13 Tim Horton 2013-05-16 17:45:00 PDT
http://trac.webkit.org/changeset/150227
Comment 14 Csaba Osztrogonác 2013-05-16 22:03:44 PDT
(In reply to comment #13)
> http://trac.webkit.org/changeset/150227

FYI: It caused a serious regression - https://bugs.webkit.org/show_bug.cgi?id=116289