Bug 195702 – Block all plugins smaller than 5x5px

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Block all plugins smaller than 5x5px

[Dean Jackson]

<rdar://problem/28435204>

[Dean Jackson]

Created attachment 364578 [details]
Patch

[Sam Weinig]

Comment on attachment 364578 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        5x5px. Other browsers have implemented this for a while, and now

s/5x5px/5pxx5px/ :)

> Source/WebKitLegacy/mac/ChangeLog:25
> [phone]  Dean Jackson  <[e-mail]>
> +
> +        Block all plugins smaller than 5x5px
> +        https://bugs.webkit.org/show_bug.cgi?id=195702
> +        <rdar://problem/28435204>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Removed a function that was never being called.
> +
> +        * WebCoreSupport/WebFrameLoaderClient.h:
> +        * WebCoreSupport/WebFrameLoaderClient.mm:
> +        (WebFrameLoaderClient::recreatePlugin): Deleted.
> +
> [phone]  Dean Jackson  <[e-mail]>
> +
> +        Block all plugins smaller than 5x5px
> +        https://bugs.webkit.org/show_bug.cgi?id=195702
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebCoreSupport/WebFrameLoaderClient.h:
> +        * WebCoreSupport/WebFrameLoaderClient.mm:
> +        (WebFrameLoaderClient::recreatePlugin): Deleted.
> +

Double ChangeLog!

> Source/WebCore/platform/LocalizedStrings.cpp:683
> +    return WEB_UI_STRING_KEY("Plug-in too small", "Plug-In too small", "Label text to be used when a plug-in was blocked from loading because it was too small");

You have different capitalization of "plug-in" in the first two strings. Is that intentional?

> LayoutTests/plugins/small-plugin-blocked.html:34
> +<embed id="plugin3" type="application/x-webkit-test-netscape" width="4" height="4"></embed>

Probably want to do one on the boundary, 5px, as well.  And maybe one that has a width too small, but a big height or vice versa.

[Sam Weinig]

Comment on attachment 364578 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:893
> +static bool pluginIsSmall(WebCore::HTMLPlugInElement* pluginElement)

Can this take a reference?

[Simon Fraser (smfr)]

Comment on attachment 364578 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInElement.h:86
> +    WEBCORE_EXPORT bool isTooSmall() const;

"too small" sounds subjective. isBelowEnablingSizeThreshold or something?

> Source/WebCore/page/Settings.yaml:814
> +blockSmallPluginsEnabled:

"block" is a verb so this is a bit hard to read. "smallPluginBlockingEnabled"? blockingOfSmallPluginsEnabled?

> Source/WebKit/Shared/WebPreferences.yaml:1250
> +BlockSmallPluginsEnabled:

smallPluginBlockingEnabled

> Source/WebKit/Shared/WebPreferences.yaml:1254
> +  humanReadableDescription: "Stop plugins smaller than a certain threshold from loading."

Maybe you can actually say what the threshold is here.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:900
> +    return box.contentWidth() <= smallPluginDimensionThreshold && box.contentHeight() <= smallPluginDimensionThreshold;

Is this what other browsers do? Should we not block 1x6px plugins?

> LayoutTests/plugins/small-plugin-blocked.html:35
> +<embed id="plugin4" type="application/x-webkit-test-netscape" width="6" height="6"></embed>

Maybe test 1x6

[EWS Watchlist]

Comment on attachment 364578 [details]
Patch

Attachment 364578 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11494760

New failing tests:
plugins/small-plugin-blocked.html

[EWS Watchlist]

Created attachment 364586 [details]
Archive of layout-test-results from ews103 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 364578 [details]
Patch

Attachment 364578 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11494915

New failing tests:
fast/frames/sandboxed-iframe-navigation-allowed.html
fast/frames/sandboxed-iframe-about-blank.html
compositing/plugins/small-to-large-composited-plugin.html

[EWS Watchlist]

Created attachment 364589 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Dean Jackson]

Committed r242920: <https://trac.webkit.org/changeset/242920>

[Dean Jackson]

Comment on attachment 364578 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        5x5px. Other browsers have implemented this for a while, and now
> 
> s/5x5px/5pxx5px/ :)

fixed.

>> Source/WebKitLegacy/mac/ChangeLog:25
>> +
> 
> Double ChangeLog!

fixed.

>> Source/WebCore/html/HTMLPlugInElement.h:86
>> +    WEBCORE_EXPORT bool isTooSmall() const;
> 
> "too small" sounds subjective. isBelowEnablingSizeThreshold or something?

changed

>> Source/WebCore/page/Settings.yaml:814
>> +blockSmallPluginsEnabled:
> 
> "block" is a verb so this is a bit hard to read. "smallPluginBlockingEnabled"? blockingOfSmallPluginsEnabled?

changed

>> Source/WebCore/platform/LocalizedStrings.cpp:683
>> +    return WEB_UI_STRING_KEY("Plug-in too small", "Plug-In too small", "Label text to be used when a plug-in was blocked from loading because it was too small");
> 
> You have different capitalization of "plug-in" in the first two strings. Is that intentional?

fixed.

>> Source/WebKit/Shared/WebPreferences.yaml:1250
>> +BlockSmallPluginsEnabled:
> 
> smallPluginBlockingEnabled

Changed.

>> Source/WebKit/Shared/WebPreferences.yaml:1254
>> +  humanReadableDescription: "Stop plugins smaller than a certain threshold from loading."
> 
> Maybe you can actually say what the threshold is here.

I didn't because this string is really only used for a tooltip in the internal debug menu.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:893
>> +static bool pluginIsSmall(WebCore::HTMLPlugInElement* pluginElement)
> 
> Can this take a reference?

fixed.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:900
>> +    return box.contentWidth() <= smallPluginDimensionThreshold && box.contentHeight() <= smallPluginDimensionThreshold;
> 
> Is this what other browsers do? Should we not block 1x6px plugins?

Other browsers seem to check for any dimension.

>> LayoutTests/plugins/small-plugin-blocked.html:35
>> +<embed id="plugin4" type="application/x-webkit-test-netscape" width="6" height="6"></embed>
> 
> Maybe test 1x6

Added tests for 5x5 1x6 and 6x1

[Ryan Haddad]

This change broke the windows build:
win10-release\build\source\webkitlegacy\win\webcoresupport\webframeloaderclient.h(190): error C3668: 'WebFrameLoaderClient::recreatePlugin': method with override specifier 'override' did not override any base class methods
https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/builds/2851

[Ryan Haddad]

(In reply to Ryan Haddad from comment #12)
> This change broke the windows build:
> win10-
> release\build\source\webkitlegacy\win\webcoresupport\webframeloaderclient.
> h(190): error C3668: 'WebFrameLoaderClient::recreatePlugin': method with
> override specifier 'override' did not override any base class methods
> https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/
> builds/2851
Attempted a fix in https://trac.webkit.org/changeset/242925