Bug 76088 – The common case of content type = text/plain is not optimized and the plugin database is initialized instead

• Component: Page Loading
• Status: RESOLVED FIXED

[Adam Treat]

Hi,

In the dom/DOMImplementation.cpp file you can find the following comment:

    // Everything else except text/plain can be overridden by plugins. In particular, Adobe SVG Viewer should be used for SV
    // Disallowing plug-ins to use text/plain prevents plug-ins from hijacking a fundamental type that the browser is expect
    // and also serves as an optimization to prevent loading the plug-in database in the common case.

Unfortunately, this has been regressed since the patch for http://bugs.webkit.org/show_bug.cgi?id=16815 which refactored a bunch of the plugin code.  Now, the plugin database is initialized before we handle text/plain.  This line in DOMImplementation.cpp trigger plugin initialization:

        pluginData = frame->page()->pluginData();

Here is the call stack:

#0  WebCore::PluginPackage::load (Each port's implementation which generally calls dlopen of some sort)
#1  0x7a00b27c in WebCore::PluginPackage::fetchInfo (Each port's implementation which calls PluginPackage::load)
#2  0x79ff9af0 in WebCore::PluginPackage::createPackage (path=..., lastModified=@0x4e68d0) at /home/manyoso/Winchester/webkit/Source/WebCore/plugins/PluginPackage.cpp:164
#3  0x79fed0d8 in WebCore::PluginDatabase::refresh (this=0x525038) at /home/manyoso/Winchester/webkit/Source/WebCore/plugins/PluginDatabase.cpp:154
#4  0x79fecc44 in WebCore::PluginDatabase::installedPlugins (populate=true) at /home/manyoso/Winchester/webkit/Source/WebCore/plugins/PluginDatabase.cpp:74
#5  0x7a00a28c in WebCore::PluginData::initPlugins (this=0x3bcd70) at /home/manyoso/Winchester/webkit/Source/WebCore/plugins/blackberry/PluginDataBlackBerry.cpp:29
#6  0x7991df44 in WebCore::PluginData::PluginData (this=0x3bcd70, page=0x397410) at /home/manyoso/Winchester/webkit/Source/WebCore/plugins/PluginData.cpp:36
#7  0x79816700 in WebCore::PluginData::create (page=0x397410) at /home/manyoso/Winchester/webkit/Source/WebCore/plugins/PluginData.h:53
#8  0x79813aac in WebCore::Page::pluginData (this=0x397410) at /home/manyoso/Winchester/webkit/Source/WebCore/page/Page.cpp:447

I'm attaching a proposed fix.

[Adam Treat]

Created attachment 122067 [details]
Fix

[Adam Treat]

I will further add that the DOMImplementation::createDocument still has misleading comments.

Currently, image types != pdf and video are *also* not allowed to be overridden by plugins, but they plugin database is still initialized before these content types are evaluated.

Should I fix this too or should I make it so plugins can override these too?

[Adam Treat]

Created attachment 122077 [details]
Revision based on feedback from andersca

I've revised the patch to take into account that image types != PDF and HTML5 video are also not optimized to be handled before initializing plugins.  This is based on the assumption that these types should not be overridden by plugins which I think is correct and is currently the case.

[WebKit Review Bot]

Comment on attachment 122077 [details]
Revision based on feedback from andersca

Rejecting attachment 122077 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
u'Anders Carlsson', u'--..." exit_code: 1

Parsed 2 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/dom/DOMImplementation.cpp
Hunk #1 FAILED at 311.
Hunk #2 succeeded at 322 (offset 3 lines).
1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/dom/DOMImplementation.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Anders Carlsson', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/11108552

[Adam Treat]

Created attachment 122088 [details]
Same patch on latest HEAD so it will apply

Just rebased on top of latest HEAD so commitbot can apply it.

[WebKit Review Bot]

Comment on attachment 122088 [details]
Same patch on latest HEAD so it will apply

Clearing flags on attachment: 122088

Committed r104746: <http://trac.webkit.org/changeset/104746>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 122088 [details]
Same patch on latest HEAD so it will apply

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

> Source/WebCore/dom/DOMImplementation.cpp:336
> +    if (Image::supportsType(type) && type != "application/pdf" && type != "text/pdf")

Most other PDF code in WebKit only supports application/pdf. It would be nice to clean this up one day.

(In reply to comment #6)
> (From update of attachment 122088 [details])
> Clearing flags on attachment: 122088
> 
> Committed r104746: <http://trac.webkit.org/changeset/104746>

This change made iframes load PDFs as media documents, at least in OS X (where the media player supports PDFs), breaking PDF printing among other things.

Filed bug 77079.