Bug 153657 – Add "WebKit built-in PDF" Plugin to set of publicly visible plugins

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

A number of poorly-coded sites break when WebKit reports that it understands PDF mime types, but does not have a list of plugins that the site can iterate through.

To avoid this compatibility problem, we should also expose the "WebKit built-in PDF" plugin to avoid this edge case.

[Brent Fulgham]

<rdar://problem/24413107>

[Brent Fulgham]

Created attachment 270244 [details]
Unveil "WebKit built-in PDF" plugin to JS.

[Alexey Proskuryakov]

Comment on attachment 270244 [details]
Unveil "WebKit built-in PDF" plugin to JS.

Is this name localized? Unlike most other plug-ins, this one does have a localized name, although I'm not sure if plugin.name is that one.

[Alexey Proskuryakov]

Comment on attachment 270244 [details]
Unveil "WebKit built-in PDF" plugin to JS.

Verified that it is localized, so this patch is wrong.

[Brent Fulgham]

(In reply to comment #4)
> Comment on attachment 270244 [details]
> Unveil "WebKit built-in PDF" plugin to JS.
> 
> Verified that it is localized, so this patch is wrong.

Good catch, thank you. I'll find the appropriate I18N stuff and revise the patch.

[Brent Fulgham]

I think this is just a matter of using builtInPDFPluginName().

[Brent Fulgham]

Created attachment 270343 [details]
Patch

[Darin Adler]

Comment on attachment 270343 [details]
Patch

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

Need to fix conditional so platforms without PDF plugin still compile.

Also should add a regression test.

> Source/WebCore/plugins/PluginData.cpp:57
> +        || plugin.name.containsIgnoringASCIICase(builtInPDFPluginName());

This should be equal rather than contains.

[Brent Fulgham]

Created attachment 270351 [details]
Patch

[Brent Fulgham]

(In reply to comment #8)
> Comment on attachment 270343 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270343&action=review
> 
> Need to fix conditional so platforms without PDF plugin still compile.

Fixed.

> Also should add a regression test.

Done.

> > Source/WebCore/plugins/PluginData.cpp:57
> > +        || plugin.name.containsIgnoringASCIICase(builtInPDFPluginName());
> 
> This should be equal rather than contains.

Done.

I've uploaded a new patch to confirm that my changes work on the other ports, and that the test works as expected outside of my development machine.

[Build Bot]

Comment on attachment 270351 [details]
Patch

Attachment 270351 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/765434

New failing tests:
http/tests/plugins/visible_plugins.html

[Build Bot]

Created attachment 270352 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Brent Fulgham]

(In reply to comment #11)
> Comment on attachment 270351 [details]
> Patch
> 
> Attachment 270351 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/765434
> 
> New failing tests:
> http/tests/plugins/visible_plugins.html

Oh, I didn't realize that the WK1 behavior was different for plugins. It looks like the "WebKit built-in PDF" plugin is only available in WK2.

I'll add an appropriate skip for WK1.

[Brent Fulgham]

Created attachment 270361 [details]
Patch

[Darin Adler]

Comment on attachment 270361 [details]
Patch

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

r=me as is, comments on ways to improve the patch further

> Source/WebCore/plugins/PluginData.cpp:58
> +#if PLATFORM(COCOA)
> +        || plugin.name == builtInPDFPluginName()
> +#endif

Three more thoughts:

1) It’s clever but confusing to stick this between two unconditional ones. Would be nicer if we could put this first or last and somehow deal with the ugliness of trying to make the #if work.

2) This function has a good comment explaining why it’s here, a comment that absolutely does *not* make it clear why builtInPDFPluginName would be here. It says “the set of plugins that Mozilla” has been using, which is simply wrong. We need a comment explaining the new policy, like the change log entry does.

3) The "contains ignoring ASCII case" sure is a pretty sloppy approach. I think the only reason it’s really OK is that there aren’t any plug-ins that just happen to have those substrings in their name that are not one of the super-common plug-ins. Pre-existing issue that your patch makes no worse, but ugly.

I think this is a good way to deal with (1):

#if PLATFORM(COCOA)

static inline bool isBuiltInPDFPlugIn(const PluginInfo&)
{
    return false;
}

#else

static inline bool isBuiltInPDFPlugIn(const PluginInfo& plugIn)
{
    return plugin.name == builtInPDFPluginName();
}

#endif

Then:

static bool shouldBePubliclyVisible(const PluginInfo& plugin)
{
...
        || plugin.name.containsIgnoringASCIICase("Java")
        || isBuiltInPDFPlugIn(plugin);
}

Dealing with (2) requires a little bit more thought. The existing comment does not explain why these three plug-ins are so important they need to be visible. Even then comment about "track record about compatibility" is unclear. The point there is that no further plug-ins besides these seem to be required visible to ensure website compatibility. But the wording does not make that clear, almost sounds like the reverse. And then the comment about PDF needs to be written carefully enough to make it clear as well. Especially the fact that even though we are keying off the plug-in name, it’s not the name that matters for compatibility, but rather that there is some plug-in that handles the PDF type. In fact, if we had some other way to know this was the built-in PDF plug-in it would be better.

[Brent Fulgham]

(In reply to comment #15)
> I think this is a good way to deal with (1):
> 
> #if PLATFORM(COCOA)
[ ... ]
> 
> static bool shouldBePubliclyVisible(const PluginInfo& plugin)
> {
> ...
>         || plugin.name.containsIgnoringASCIICase("Java")
>         || isBuiltInPDFPlugIn(plugin);
> }

I agree, and have revised the code to match.

I did not attempt to deal with (2) or (3) in this patch, but will give it more thought.

[Brent Fulgham]

Committed r195950: <http://trac.webkit.org/changeset/195950>

[Darin Adler]

Don’t need to deal with (3), it’s not even a new issue, but should come back and tweak the comment to resolve (2).

[Brent Fulgham]

(In reply to comment #18)
> Don’t need to deal with (3), it’s not even a new issue, but should come back
> and tweak the comment to resolve (2).

I was thinking of something along these lines. What do you think?

    // We can greatly reduce fingerprinting opportunities by only advertising plugins
    // that are widely needed for general website compatibility. Since many users
    // will have these plugins, we are not revealing much user-specific information.
    //
    // Web compatibility data indicates that Flash, Quicktime, Java, and PDF support
    // are the most frequently needed plugins, and also the least user-specific.
    //
    // It would be better if we could expose plugins based on their function, rather
    // than name, since that is the only real information needed by the website.

[Darin Adler]

(In reply to comment #19)
>     // We can greatly reduce fingerprinting opportunities by only advertising plugins
>     // that are widely needed for general website compatibility. Since many users
>     // will have these plugins, we are not revealing much user-specific information.

Love this paragraph.

(Should be "plug-ins" rather than "plugins".)

>     // Web compatibility data indicates that Flash, Quicktime, Java, and PDF support
>     // are the most frequently needed plugins, and also the least user-specific.

This isn’t quite right. The issue isn’t which are the most needed plug-ins. This code doesn’t make plug-ins go away. It just omits them from the list of plug-ins you can iterate.

These are the plug-ins that websites need to be able to check for by looking at the list of plug-ins (not sure I have the terminology right), which is *not* the same thing as the plug-ins that are needed the most.

I think the rationale here is that we can cut down fingerprinting a lot by leaving all those other plug-ins out of the list without hurting compatibility, but that for each of these, if we leave them out, we break a lot of websites.

I don’t think the logic is quite the same for PDF. It’s a smaller number of websites.

QuickTime is spelled with a capital T.

>     // It would be better if we could expose plugins based on their function, rather
>     // than name, since that is the only real information needed by the website.

This is not my objection to checking by name.

Exposing plug-ins based on finding a substring in the plug-in’s name is an inexact way to single out Flash, QuickTime, and Java that will possibly make other plug-ins visible if they are unlucky enough to have "shockwave", "quicktime", or "java" as substrings of their names. It seems that "java" is a particular unfortunate one since a plug-in might choose to mention "JavaScript" in its name if it’s providing functions you can get at with JavaScript.

Exposing our built-in PDF plug-in based on its name is particularly funny; it’s convenient but absolutely unnecessary. We could store data that unambiguously identifies a plug-in as the built-in PDF one. We are really checking the name just because that was the most obvious way to do it given we were already checking names.

There’s another issue, which is that we do nothing to obscure the precise version of the plug-in, which is helpful for fingerprinting. Exposing exactly which PDF plug-in is present, and exposing the exact version of Flash, QuickTime, and Java, is handy for fingerprinting. May be required for compatibility or not.

I’m not saying all of this needs to be covered in the comment.

[Brent Fulgham]

(In reply to comment #20)

Here's my latest:

    // We can greatly reduce fingerprinting opportunities by only advertising plug-ins
    // that are widely needed for general website compatibility. Since many users
    // will have these plug-ins, we are not revealing much user-specific information.
    //
    // Web compatibility data indicate that Flash, QuickTime, Java, and PDF support
    // are frequently accessed through the bad practice of iterating over the contents
    // of the navigator.plugins list. Luckily, these plug-ins happen to be the least
    // user-specific.

[Darin Adler]

Seems good.

[Brent Fulgham]

Landed update to comments in <http://trac.webkit.org/changeset/196164>.