Bug 102148 – Automatically run small plugins

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jon Lee]

Plugins that are smaller than 1px x 1px should automatically run.

[Radar WebKit Bug Importer]

<rdar://problem/12695560>

[Jon Lee]

Created attachment 174030 [details]
Patch

[Tim Horton]

Comment on attachment 174030 [details]
Patch

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

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:156
> +    if (plugInImageElement()->displayState() < HTMLPlugInElement::Playing
> +        && (!width || !height || (width <= autoStartPlugInSizeThresholdWidth && height <= autoStartPlugInSizeThresholdHeight)))
> +        plugInImageElement()->setDisplayState(HTMLPlugInElement::Playing);

Seems like this would let sites get around snapshotting/freezing by making their plugin 0x0 for the first layout and then the real size later.

[Jon Lee]

Filed bug 102157 to track this issue.

[Eric Seidel (no email)]

Interesting.  If I'm understanding this bug correctly (which I may not be!) this sounds like a different security decision than Chromium has made.

[Eric Seidel (no email)]

(It's not clear to me that this change affects chromium.  Just noting the possible divergence.)

[Darin Adler]

Comment on attachment 174030 [details]
Patch

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

I’m OK with this change, although Tim’s point is an interesting one.

> Source/WebCore/rendering/RenderEmbeddedObject.h:72
> +    virtual void layout();

As you are moving this, please also add the OVERRIDE keyword.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:41
> +static int autoStartPlugInSizeThresholdWidth = 1;
> +static int autoStartPlugInSizeThresholdHeight = 1;

These should be const. Once they are const they need not be static (although there is no harm in marking them so, except perhaps to annoy Alexey).

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:153
> +    int width = rect.width(), height = rect.height();

We don’t do multiple variables on one line like this in WebKit.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:155
> +    if (plugInImageElement()->displayState() < HTMLPlugInElement::Playing
> +        && (!width || !height || (width <= autoStartPlugInSizeThresholdWidth && height <= autoStartPlugInSizeThresholdHeight)))

I suggest computing the content box size only after checking the displayState.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.h:57
> +    virtual void layout();

Please add OVERRIDE.

[Jon Lee]

Comment on attachment 174030 [details]
Patch

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

>> Source/WebCore/rendering/RenderEmbeddedObject.h:72
>> +    virtual void layout();
> 
> As you are moving this, please also add the OVERRIDE keyword.

Done.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:41
>> +static int autoStartPlugInSizeThresholdHeight = 1;
> 
> These should be const. Once they are const they need not be static (although there is no harm in marking them so, except perhaps to annoy Alexey).

Always good to keep Alexey happy.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:153
>> +    int width = rect.width(), height = rect.height();
> 
> We don’t do multiple variables on one line like this in WebKit.

Split.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:155
>> +        && (!width || !height || (width <= autoStartPlugInSizeThresholdWidth && height <= autoStartPlugInSizeThresholdHeight)))
> 
> I suggest computing the content box size only after checking the displayState.

Done.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.h:57
>> +    virtual void layout();
> 
> Please add OVERRIDE.

Done.

[Jon Lee]

Committed r134526: <http://trac.webkit.org/changeset/134526>