[wk2] REGRESSION (r125500): WebFrameLoaderClient's active m_pluginView can be zeroed by PluginDocuments leaving the page cache

[Tim Horton]

http://trac.webkit.org/changeset/125500 made a change that purported to be Chromium only (it even had a [chromium] tag!) but actually changed the behavior for WebKit2 as well:

It now zeroes the FrameLoaderClient's redirecting-PluginView in PluginDocument::detach(), which can happen *way later* in the case that the PluginDocument went into the page cache, clearing a legitimate-and-different PluginView from (Web)FrameLoaderClient.

This change also made sure not to change behavior for other ports, ignoring the new redirectDataToPlugin(0) case, but missed WebKit2 (possibly intentionally because m_pluginView is a RefPtr for WK2 and not for most other non-Chromium ports). We already clear the m_pluginView RefPtr when the load finishes or fails, so I think we can safely early-return in the redirectDataToPlugin(0) case just like all the other ports, returning to our behavior before r125500.

<rdar://problem/12729564>

[Tim Horton]

Created attachment 195369 [details]
patch

[Brady Eidson]

Comment on attachment 195369 [details]
patch

Sloppy patch originally  :(  Fix seems reasonable.

[Tim Horton]

http://trac.webkit.org/changeset/147152

[Darin Adler]

Comment on attachment 195369 [details]
patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1350
> -    m_pluginView = static_cast<PluginView*>(pluginWidget);
> +    if (m_pluginView)
> +        m_pluginView = static_cast<PluginView*>(pluginWidget);

I think you mean:

    if (pluginWidget)

not

    if (m_pluginView)

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 195369 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195369&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1350
> > -    m_pluginView = static_cast<PluginView*>(pluginWidget);
> > +    if (m_pluginView)
> > +        m_pluginView = static_cast<PluginView*>(pluginWidget);
> 
> I think you mean:
> 
>     if (pluginWidget)
> 
> not
> 
>     if (m_pluginView)

Woah, you're right. How I manage to screw up one line patches I'll never know.

[Brady Eidson]

And me too  :(

[Tim Horton]

Reopening for correct fix.

[Tim Horton]

Created attachment 195638 [details]
patch

[Tim Horton]

(In reply to comment #8)
> Created an attachment (id=195638) [details]
> patch

http://trac.webkit.org/changeset/147168