Bug 195702 - Block all plugins smaller than 5x5px
Summary: Block all plugins smaller than 5x5px
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2019-03-13 14:44 PDT by Dean Jackson
Modified: 2019-03-14 11:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (30.81 KB, patch)
2019-03-13 14:53 PDT, Dean Jackson
sam: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (2.45 MB, application/zip)
2019-03-13 15:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.62 MB, application/zip)
2019-03-13 16:13 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2019-03-13 14:44:47 PDT
Block all plugins smaller than 5x5px
Comment 1 Dean Jackson 2019-03-13 14:45:33 PDT
<rdar://problem/28435204>
Comment 2 Dean Jackson 2019-03-13 14:53:27 PDT
Created attachment 364578 [details]
Patch
Comment 3 Sam Weinig 2019-03-13 15:07:42 PDT
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
> +2019-03-13  Dean Jackson  <dino@apple.com>
> +
> +        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.
> +
> +2019-03-13  Dean Jackson  <dino@apple.com>
> +
> +        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.
Comment 4 Sam Weinig 2019-03-13 15:10:10 PDT
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?
Comment 5 Simon Fraser (smfr) 2019-03-13 15:12:07 PDT
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
Comment 6 Build Bot 2019-03-13 15:54:46 PDT
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
Comment 7 Build Bot 2019-03-13 15:54:48 PDT
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
Comment 8 Build Bot 2019-03-13 16:13:06 PDT
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
Comment 9 Build Bot 2019-03-13 16:13:08 PDT
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
Comment 10 Dean Jackson 2019-03-13 16:38:03 PDT
Committed r242920: <https://trac.webkit.org/changeset/242920>
Comment 11 Dean Jackson 2019-03-13 16:40:28 PDT
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
Comment 12 Ryan Haddad 2019-03-13 17:23:54 PDT
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
Comment 13 Ryan Haddad 2019-03-13 17:45:24 PDT
(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