PlugIn Snapshotting: Crashes refreshing non-main-frame PDFPlugins

[Tim Horton]

If a plugin shouldAlwaysAutoStart() and is also detected as primary, setIsPrimarySnapshottedPlugIn can switch displayState() to Restarting and reattach, but then when the plugin is created, we're pushed forward to Playing, and things go very very poorly from there on out (we keep two plugin instances around, etc.)

This is reproducible with PDFPlugin, clicking Chase.com account statements.

<rdar://problem/13542020>

[Tim Horton]

Created attachment 198439 [details]
patch

[Dean Jackson]

Comment on attachment 198439 [details]
patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:86
> +    , m_plugInWasCreated(false)

Could we use the value of the SnapshotDecision enum to avoid this?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:519
> +        if (m_plugInWasCreated) {
> +            LOG(Plugins, "%p Plug-in was detected as the primary element in the page. Restart.", this);
> +            restartSnapshottedPlugIn();
> +            restartSimilarPlugIns();
> +        } else {
> +            LOG(Plugins, "%p Plug-in was detected as the primary element in the page, but is not yet created. Will restart later.", this);
> +            m_deferredPromotionToPrimaryPlugIn = true;
> +        }

And we could set snapshotDecision to NeverSnapshot here.

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 198439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198439&action=review
> 
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:86
> > +    , m_plugInWasCreated(false)
> 
> Could we use the value of the SnapshotDecision enum to avoid this?

Nope, because we need to know if we are strictly between willCreatePlugIn and didCreatePlugIn, which isn't something we can tell from SnapshotDecision.

> > Source/WebCore/html/HTMLPlugInImageElement.cpp:519
> > +        if (m_plugInWasCreated) {
> > +            LOG(Plugins, "%p Plug-in was detected as the primary element in the page. Restart.", this);
> > +            restartSnapshottedPlugIn();
> > +            restartSimilarPlugIns();
> > +        } else {
> > +            LOG(Plugins, "%p Plug-in was detected as the primary element in the page, but is not yet created. Will restart later.", this);
> > +            m_deferredPromotionToPrimaryPlugIn = true;
> > +        }
> 
> And we could set snapshotDecision to NeverSnapshot here.

Nope, because nothing ever consults snapshotDecision again after willCreatePlugin. And even if we added a check for snapshotDecision in didCreatePlugin, we wouldn't know to restart *similar* plugins as well (like we do for primary plugins).

[Dean Jackson]

Comment on attachment 198439 [details]
patch

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

>>> Source/WebCore/html/HTMLPlugInImageElement.cpp:86
>>> +    , m_plugInWasCreated(false)
>> 
>> Could we use the value of the SnapshotDecision enum to avoid this?
> 
> Nope, because we need to know if we are strictly between willCreatePlugIn and didCreatePlugIn, which isn't something we can tell from SnapshotDecision.

Bummer.

>>> Source/WebCore/html/HTMLPlugInImageElement.cpp:519
>>> +        }
>> 
>> And we could set snapshotDecision to NeverSnapshot here.
> 
> Nope, because nothing ever consults snapshotDecision again after willCreatePlugin. And even if we added a check for snapshotDecision in didCreatePlugin, we wouldn't know to restart *similar* plugins as well (like we do for primary plugins).

Double bummer.

[Tim Horton]

Bummer x2, indeed :(

https://trac.webkit.org/changeset/148560

Thanks, Dean!