Bug 131553 – Snapshotted plugins may need to be restarted if style properties are changed after initial load of plugin.

• Component: Plug-ins
• Status: RESOLVED FIXED

[Roger Fong]

When looking at the method subframeLoaderWillCreatePlugin you'll notice that the state of the plugin sometimes can depend on the style.
The style can change at any point. It can also be resolved after the initial load of the plugin, which could cause a plugin to be snapshotted when it shouldn't be.

For example, consider a plugin that is styled to be 1x1. 
The 1x1 plugin should never be snapshotted but if the style isn't resolved before the initial load of the plugin we assume the default RenderReplaced size for the plugin (300 x 150).
A plugin at this size will indeed get snapshotted. Thus we end up with a 1x1 plugin that is snapshotted.

When performing postLayoutTasks we checkSnapshotStatus. 
I think this would be a good place to also check to see if the style conditions are met.
If they are and the plugin is currently snapshotted then we should restart the plugin so that it actually runs.

[Roger Fong]

<rdar://problem/15443375>

[Roger Fong]

Created attachment 229149 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 229149 [details]
patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:620
> +            auto& style = renderEmbedded.style();
> +            bool isFullPage = is100Percent(style.width()) && is100Percent(style.height());
> +            IntSize visibleViewSize = document().frame()->view()->visibleSize();
> +            float contentArea = contentWidth * contentHeight;
> +            float visibleArea = visibleViewSize.width() * visibleViewSize.height();
> +            if (isFullPage && contentArea > visibleArea * sizingFullPageAreaRatioThreshold) {

You should share this code with HTMLPlugInImageElement::subframeLoaderWillCreatePlugIn().

[Brent Fulgham]

Comment on attachment 229149 [details]
patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:634
> +        checkSizeChangeForSnapshotting();

This has weird indention. Is this the only thing that the 'if' statement is meant to control? Or do you need brackets so the return is part of this test, too?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:640
> +    }

Bad whitespace.

[Roger Fong]

Created attachment 229173 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 229173 [details]
patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:604
> +bool HTMLPlugInImageElement::isTopLevelFullPage(const RenderEmbeddedObject& renderEmbedded, int contentWidth, int contentHeight)

Fewer weird abbreviations please!
isTopLevelFullPagePlugin(const RenderEmbeddedObject& embeddedObject, ...

> Source/WebCore/html/HTMLPlugInImageElement.cpp:606
> +    if (document().frame()->isMainFrame()) {

if (!document().frame()->isMainFrame())
  return false

> Source/WebCore/html/HTMLPlugInImageElement.cpp:613
> +        if (isFullPage && contentArea > visibleArea * sizingFullPageAreaRatioThreshold)
> +            return true;

return isFullPage && (contentArea > visibleArea * sizingFullPageAreaRatioThreshold);

> Source/WebCore/html/HTMLPlugInImageElement.cpp:622
> +    if (contentWidth <= sizingTinyDimensionThreshold || contentHeight <= sizingTinyDimensionThreshold)
> +        return true;
> +    return false;

return (contentWidth <= sizingTinyDimensionThreshold || contentHeight <= sizingTinyDimensionThreshold)

This could be a local static function.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:630
> +        auto& renderEmbedded = toRenderEmbeddedObject(*this->renderer());

Why not use the renderEmbeddedObject() function (and check its return value)?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:642
> +        if (displayState() == Playing)
> +        checkSizeChangeForSnapshotting();

Wrong indentation.

> Source/WebCore/html/HTMLPlugInImageElement.h:144
> +    bool isTopLevelFullPage(const RenderEmbeddedObject&, int contentWidth, int contentHeight);

Should be a const function.

[Roger Fong]

(In reply to comment #6)
> (From update of attachment 229173 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229173&action=review
> 
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:604
> > +bool HTMLPlugInImageElement::isTopLevelFullPage(const RenderEmbeddedObject& renderEmbedded, int contentWidth, int contentHeight)
> 
> Fewer weird abbreviations please!
> isTopLevelFullPagePlugin(const RenderEmbeddedObject& embeddedObject, ...
> 
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:606
> > +    if (document().frame()->isMainFrame()) {
> 
> if (!document().frame()->isMainFrame())
>   return false
> 
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:613
> > +        if (isFullPage && contentArea > visibleArea * sizingFullPageAreaRatioThreshold)
> > +            return true;
> 
> return isFullPage && (contentArea > visibleArea * sizingFullPageAreaRatioThreshold);
> 
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:622
> > +    if (contentWidth <= sizingTinyDimensionThreshold || contentHeight <= sizingTinyDimensionThreshold)
> > +        return true;
> > +    return false;
> 
> return (contentWidth <= sizingTinyDimensionThreshold || contentHeight <= sizingTinyDimensionThreshold)
> 
> This could be a local static function.
> 
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:630
> > +        auto& renderEmbedded = toRenderEmbeddedObject(*this->renderer());
> 
> Why not use the renderEmbeddedObject() function (and check its return value)?

Should I assert that it's not null? When would it not exist?

> 
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:642
> > +        if (displayState() == Playing)
> > +        checkSizeChangeForSnapshotting();
> 
> Wrong indentation.
> 
> > Source/WebCore/html/HTMLPlugInImageElement.h:144
> > +    bool isTopLevelFullPage(const RenderEmbeddedObject&, int contentWidth, int contentHeight);
> 
> Should be a const function.

[Roger Fong]

Created attachment 229179 [details]
patch

[Darin Adler]

Comment on attachment 229179 [details]
patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:604
> +static inline bool isSmallerThanTinySizingThreshold(int contentWidth, int contentHeight)

This should take a LayoutSize instead of two ints for the height and width. I would suggest just calling it size. I’m not sure that “content size” says anything more than “size” does. What other size would it be?

Maybe a const LayoutSize& or maybe just LayoutSize would be better.

Or maybe we should just have this function take a RenderEmbeddedObject& instead. That would be more convenient in both call sites.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:606
> +    return (contentWidth <= sizingTinyDimensionThreshold || contentHeight <= sizingTinyDimensionThreshold);

No need for the parentheses here.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:609
> +bool HTMLPlugInImageElement::isTopLevelFullPagePlugin(const RenderEmbeddedObject* renderEmbedded, int contentWidth, int contentHeight) const

This should take a RenderEmbeddedObject&, not a const RenderEmbeddedObject*. Or const if you insist, but it should not be a pointer since it can never be null. Also, I suggest naming this just “renderer”. There’s no need to encode the type in the name.

This function should get the contentBoxRect from the renderer. It’s not really valuable to pass it in, since both callers compute it the same way.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:612
> +    if (!document().frame()->isMainFrame())
> +        return false;

Since we use Frame more than once in this function I suggest putting it in a reference:

    Frame& frame = *document().frame();

> Source/WebCore/html/HTMLPlugInImageElement.cpp:615
> +    bool isFullPage = is100Percent(style.width()) && is100Percent(style.height());

Instead of a boolean, we should have an early return here. That’s one of the advantages of moving this to a separate function. No need to compute the areas below if the styles aren’t 100%.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:619
> +    IntSize visibleViewSize = document().frame()->view()->visibleSize();
> +    float contentArea = contentWidth * contentHeight;
> +    float visibleArea = visibleViewSize.width() * visibleViewSize.height();
> +    return isFullPage && (contentArea > visibleArea * sizingFullPageAreaRatioThreshold);

If we made an area helper function (overloaded for IntSize and LayoutSize) then these four lines could be written in one line like this:

    return area(renderer.contentBoxRect()) > area(frame.view()->visibleSize()) * sizingFullPageAreaRatioThreshold;

I think that’s better.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:627
> +        RenderEmbeddedObject* renderEmbedded = renderEmbeddedObject();

This should be a reference. The code above will already crash if the renderer is null so there is no point in using a pointer.

The type of this should be RenderSnapshottedPlugIn, not RenderEmbeddedObject. We always want to use the most-specific type we can, based on the type check we did. And here, the type check is isSnapshottedPlugIn, not isEmbeddedObject.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:628
> +        ASSERT(renderer);

Looks like this won’t compile, because the code above uses the name renderEmbedded.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:732
> +    RenderEmbeddedObject* renderer = renderEmbeddedObject();

This local variable should remain a reference, not a pointer. The patch changes it to a pointer for no good reason.

[Dean Jackson]

(In reply to comment #9)
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:619
> > +    IntSize visibleViewSize = document().frame()->view()->visibleSize();
> > +    float contentArea = contentWidth * contentHeight;
> > +    float visibleArea = visibleViewSize.width() * visibleViewSize.height();
> > +    return isFullPage && (contentArea > visibleArea * sizingFullPageAreaRatioThreshold);
> 
> If we made an area helper function (overloaded for IntSize and LayoutSize) then these four lines could be written in one line like this:
> 
>     return area(renderer.contentBoxRect()) > area(frame.view()->visibleSize()) * sizingFullPageAreaRatioThreshold;
> 

There already is an area() member function on IntSize. We don't have one on LayoutSize. I like the idea of a helper function so here is a patch: http://webkit.org/b/131606

[Jon Lee]

I have some concerns here that it’s possible for a plugin to resize thrice to get to a non snapshotted state with this patch.

- Plugin starts off small, decision = MaySnapshotWhenResized

- Plugin gets resized larger, decision = Snapshotted

- Plugin gets resized small again; we restart the plugin but decision remains Snapshotted

- Plugin gets resized large again; the plugin does not get snapshotted

I think we need to make sure that the restart path we’re allowing here is specifically the case where the plugin starts off at the default plugin size (300x150) and then gets resized later to a small size.

[Jon Lee]

Also, can we write a test for this?

[Roger Fong]

(In reply to comment #11)
> I have some concerns here that it’s possible for a plugin to resize thrice to get to a non snapshotted state with this patch.
> 
> - Plugin starts off small, decision = MaySnapshotWhenResized
> 
> - Plugin gets resized larger, decision = Snapshotted
> 
> - Plugin gets resized small again; we restart the plugin but decision remains Snapshotted
> 
> - Plugin gets resized large again; the plugin does not get snapshotted

Shouldn't we just change the decision everytime the plugin gets resized?

> 
> I think we need to make sure that the restart path we’re allowing here is specifically the case where the plugin starts off at the default plugin size (300x150) and then gets resized later to a small size.

[Roger Fong]

(In reply to comment #12)
> Also, can we write a test for this?

Hmm...I guess we could do something have a timer to apply CSS and then check to see if the plugin has shadow dom content (the snapshot), I'm assuming this is doable via layout tests somehow.

[Roger Fong]

(In reply to comment #13)
> (In reply to comment #11)
> > I have some concerns here that it’s possible for a plugin to resize thrice to get to a non snapshotted state with this patch.
> > 
> > - Plugin starts off small, decision = MaySnapshotWhenResized
> > 
> > - Plugin gets resized larger, decision = Snapshotted
> > 
> > - Plugin gets resized small again; we restart the plugin but decision remains Snapshotted

Wouldn't setting decision = MaySnapshotWhenResized here (which is the change I made comes into play), do the trick?

> > 
> > - Plugin gets resized large again; the plugin does not get snapshotted
> 
> Shouldn't we just change the decision everytime the plugin gets resized?
> 
> > 
> > I think we need to make sure that the restart path we’re allowing here is specifically the case where the plugin starts off at the default plugin size (300x150) and then gets resized later to a small size.

[Roger Fong]

Created attachment 229326 [details]
patch

In this version of the patch we only restart the plugin if the dimensions are changed to being too small if the existing dimensions were not specified yet (using default intrinsic size).

If the dimensions were already specified then we want to stick with whatever snapshotting decision was made with those initial dimensions.

[Roger Fong]

ping

[Tim Horton]

Comment on attachment 229326 [details]
patch

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

This is fine with me as long as the behavior is fine with Jon/Dean, but please confirm.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:609
> +    int contentWidth = contentRect.width();
> +    int contentHeight = contentRect.height();

why the temporaries? they're only used once below.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:646
> +    if (!renderer()->isSnapshottedPlugIn()) {

if you move this above the previous block, you can get rid of that check in its condition

> LayoutTests/plugins/snapshotting/set-plugin-size-to-tiny.html:15
> +        testRunner.notifyDone()

semicolon

[Roger Fong]

Committed http://trac.webkit.org/changeset/167339
w/ Tim's comments.

Behavior was discussed with Jon earlier.
thx!