- Revision
- 196717
- Author
- [email protected]
- Date
- 2016-02-17 13:27:02 -0800 (Wed, 17 Feb 2016)
Log Message
PDFPlugin's scrollableArea container is not properly unregistered when page is going into the PageCache
https://bugs.webkit.org/show_bug.cgi?id=148182
Reviewed by Brent Fulgham.
Source/WebCore:
When handling Command-arrow key while showing a scrollable PDF, the timing of PDFPlugin
teardown and navigation could result in PDFPlugin::destroy() getting the wrong FrameView,
so the old FrameView was left with a stale pointer in its scrollableAreaSet.
Fix this by adding an explicit willDetatchRenderer() which is called on the plugin
before the Frame gets a new FrameView.
Also narrow the scope of the RefPtr<Widget> in HTMLPlugInElement::defaultEventHandler()
so that the Widget is not kept alive over a possible navigation.
I was unable to make an automated test, because reproducing the bug requires handling
a Command-arrow key event in a way that the last ref to a Widget is held over the event
handling, and this wasn't possible in an iframe.
* html/HTMLPlugInElement.cpp:
(WebCore::HTMLPlugInElement::defaultEventHandler):
* html/HTMLPlugInImageElement.cpp:
(WebCore::HTMLPlugInImageElement::willDetachRenderers):
* plugins/PluginViewBase.h:
(WebCore::PluginViewBase::willDetatchRenderer):
* style/StyleTreeResolver.cpp:
(WebCore::Style::detachRenderTree): Drive-by nullptr.
Source/WebKit2:
When handling Command-arrow key while showing a scrollable PDF, the timing of PDFPlugin
teardown and navigation could result in PDFPlugin::destroy() getting the wrong FrameView,
so the old FrameView was left with a stale pointer in its scrollableAreaSet.
Fix this by adding an explicit willDetatchRenderer() which is called on the plugin
before the Frame gets a new FrameView.
Also narrow the scope of the RefPtr<Widget> in HTMLPlugInElement::defaultEventHandler()
so that the Widget is not kept alive over a possible navigation.
I was unable to make an automated test, because reproducing the bug requires handling
a Command-arrow key event in a way that the last ref to a Widget is held over the event
handling, and this wasn't possible in an iframe.
* WebProcess/Plugins/PDF/DeprecatedPDFPlugin.h:
* WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm:
(WebKit::PDFPlugin::willDetatchRenderer):
* WebProcess/Plugins/Plugin.h:
(WebKit::Plugin::willDetatchRenderer):
* WebProcess/Plugins/PluginView.cpp:
(WebKit::PluginView::willDetatchRenderer):
* WebProcess/Plugins/PluginView.h:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (196716 => 196717)
--- trunk/Source/WebCore/ChangeLog 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebCore/ChangeLog 2016-02-17 21:27:02 UTC (rev 196717)
@@ -1,3 +1,33 @@
+2016-02-17 Simon Fraser <[email protected]>
+
+ PDFPlugin's scrollableArea container is not properly unregistered when page is going into the PageCache
+ https://bugs.webkit.org/show_bug.cgi?id=148182
+
+ Reviewed by Brent Fulgham.
+
+ When handling Command-arrow key while showing a scrollable PDF, the timing of PDFPlugin
+ teardown and navigation could result in PDFPlugin::destroy() getting the wrong FrameView,
+ so the old FrameView was left with a stale pointer in its scrollableAreaSet.
+
+ Fix this by adding an explicit willDetatchRenderer() which is called on the plugin
+ before the Frame gets a new FrameView.
+
+ Also narrow the scope of the RefPtr<Widget> in HTMLPlugInElement::defaultEventHandler()
+ so that the Widget is not kept alive over a possible navigation.
+
+ I was unable to make an automated test, because reproducing the bug requires handling
+ a Command-arrow key event in a way that the last ref to a Widget is held over the event
+ handling, and this wasn't possible in an iframe.
+
+ * html/HTMLPlugInElement.cpp:
+ (WebCore::HTMLPlugInElement::defaultEventHandler):
+ * html/HTMLPlugInImageElement.cpp:
+ (WebCore::HTMLPlugInImageElement::willDetachRenderers):
+ * plugins/PluginViewBase.h:
+ (WebCore::PluginViewBase::willDetatchRenderer):
+ * style/StyleTreeResolver.cpp:
+ (WebCore::Style::detachRenderTree): Drive-by nullptr.
+
2016-02-17 Brady Eidson <[email protected]>
Modern IDB: Encoder/Decoder/Messaging scaffolding for WK2 IPC.
Modified: trunk/Source/WebCore/html/HTMLPlugInElement.cpp (196716 => 196717)
--- trunk/Source/WebCore/html/HTMLPlugInElement.cpp 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebCore/html/HTMLPlugInElement.cpp 2016-02-17 21:27:02 UTC (rev 196717)
@@ -225,12 +225,15 @@
return;
}
- RefPtr<Widget> widget = downcast<RenderWidget>(*renderer).widget();
- if (!widget)
- return;
- widget->handleEvent(event);
- if (event->defaultHandled())
- return;
+ // Don't keep the widget alive over the defaultEventHandler call, since that can do things like navigate.
+ {
+ RefPtr<Widget> widget = downcast<RenderWidget>(*renderer).widget();
+ if (!widget)
+ return;
+ widget->handleEvent(event);
+ if (event->defaultHandled())
+ return;
+ }
HTMLFrameOwnerElement::defaultEventHandler(event);
}
Modified: trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp (196716 => 196717)
--- trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp 2016-02-17 21:27:02 UTC (rev 196717)
@@ -272,6 +272,10 @@
setNeedsWidgetUpdate(true);
}
+ Widget* widget = pluginWidget(PluginLoadingPolicy::DoNotLoad);
+ if (is<PluginViewBase>(widget))
+ downcast<PluginViewBase>(*widget).willDetatchRenderer();
+
HTMLPlugInElement::willDetachRenderers();
}
Modified: trunk/Source/WebCore/plugins/PluginViewBase.h (196716 => 196717)
--- trunk/Source/WebCore/plugins/PluginViewBase.h 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebCore/plugins/PluginViewBase.h 2016-02-17 21:27:02 UTC (rev 196717)
@@ -81,6 +81,8 @@
virtual void setJavaScriptPaused(bool) { }
virtual RefPtr<JSC::Bindings::Instance> bindingInstance() { return nullptr; }
+
+ virtual void willDetatchRenderer() { }
protected:
explicit PluginViewBase(PlatformWidget widget = 0) : Widget(widget) { }
Modified: trunk/Source/WebCore/style/StyleTreeResolver.cpp (196716 => 196717)
--- trunk/Source/WebCore/style/StyleTreeResolver.cpp 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebCore/style/StyleTreeResolver.cpp 2016-02-17 21:27:02 UTC (rev 196717)
@@ -613,7 +613,7 @@
if (current.renderer())
current.renderer()->destroyAndCleanupAnonymousWrappers();
- current.setRenderer(0);
+ current.setRenderer(nullptr);
if (current.hasCustomStyleResolveCallbacks())
current.didDetachRenderers();
Modified: trunk/Source/WebKit2/ChangeLog (196716 => 196717)
--- trunk/Source/WebKit2/ChangeLog 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebKit2/ChangeLog 2016-02-17 21:27:02 UTC (rev 196717)
@@ -1,3 +1,33 @@
+2016-02-17 Simon Fraser <[email protected]>
+
+ PDFPlugin's scrollableArea container is not properly unregistered when page is going into the PageCache
+ https://bugs.webkit.org/show_bug.cgi?id=148182
+
+ Reviewed by Brent Fulgham.
+
+ When handling Command-arrow key while showing a scrollable PDF, the timing of PDFPlugin
+ teardown and navigation could result in PDFPlugin::destroy() getting the wrong FrameView,
+ so the old FrameView was left with a stale pointer in its scrollableAreaSet.
+
+ Fix this by adding an explicit willDetatchRenderer() which is called on the plugin
+ before the Frame gets a new FrameView.
+
+ Also narrow the scope of the RefPtr<Widget> in HTMLPlugInElement::defaultEventHandler()
+ so that the Widget is not kept alive over a possible navigation.
+
+ I was unable to make an automated test, because reproducing the bug requires handling
+ a Command-arrow key event in a way that the last ref to a Widget is held over the event
+ handling, and this wasn't possible in an iframe.
+
+ * WebProcess/Plugins/PDF/DeprecatedPDFPlugin.h:
+ * WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm:
+ (WebKit::PDFPlugin::willDetatchRenderer):
+ * WebProcess/Plugins/Plugin.h:
+ (WebKit::Plugin::willDetatchRenderer):
+ * WebProcess/Plugins/PluginView.cpp:
+ (WebKit::PluginView::willDetatchRenderer):
+ * WebProcess/Plugins/PluginView.h:
+
2016-02-17 Brady Eidson <[email protected]>
Modern IDB: Encoder/Decoder/Messaging scaffolding for WK2 IPC.
Modified: trunk/Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.h (196716 => 196717)
--- trunk/Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.h 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.h 2016-02-17 21:27:02 UTC (rev 196717)
@@ -163,6 +163,7 @@
virtual bool getFormValue(String& formValue) override { return false; }
virtual bool handleScroll(WebCore::ScrollDirection, WebCore::ScrollGranularity) override;
virtual RefPtr<WebCore::SharedBuffer> liveResourceData() const override;
+ virtual void willDetatchRenderer() override;
virtual bool isBeingAsynchronouslyInitialized() const override { return false; }
Modified: trunk/Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm (196716 => 196717)
--- trunk/Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm 2016-02-17 21:27:02 UTC (rev 196717)
@@ -1091,6 +1091,14 @@
return true;
}
+void PDFPlugin::willDetatchRenderer()
+{
+ if (webFrame()) {
+ if (FrameView* frameView = webFrame()->coreFrame()->view())
+ frameView->removeScrollableArea(this);
+ }
+}
+
void PDFPlugin::destroy()
{
m_pdfLayerController.get().delegate = 0;
Modified: trunk/Source/WebKit2/WebProcess/Plugins/Plugin.h (196716 => 196717)
--- trunk/Source/WebKit2/WebProcess/Plugins/Plugin.h 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebKit2/WebProcess/Plugins/Plugin.h 2016-02-17 21:27:02 UTC (rev 196717)
@@ -301,6 +301,8 @@
virtual bool requiresUnifiedScaleFactor() const { return false; }
+ virtual void willDetatchRenderer() { }
+
protected:
Plugin(PluginType);
Modified: trunk/Source/WebKit2/WebProcess/Plugins/PluginView.cpp (196716 => 196717)
--- trunk/Source/WebKit2/WebProcess/Plugins/PluginView.cpp 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebKit2/WebProcess/Plugins/PluginView.cpp 2016-02-17 21:27:02 UTC (rev 196717)
@@ -1005,6 +1005,14 @@
return m_pluginElement->displayState() < HTMLPlugInElement::Restarting && !m_plugin->supportsSnapshotting();
}
+void PluginView::willDetatchRenderer()
+{
+ if (!m_isInitialized || !m_plugin)
+ return;
+
+ m_plugin->willDetatchRenderer();
+}
+
PassRefPtr<SharedBuffer> PluginView::liveResourceData() const
{
if (!m_isInitialized || !m_plugin)
Modified: trunk/Source/WebKit2/WebProcess/Plugins/PluginView.h (196716 => 196717)
--- trunk/Source/WebKit2/WebProcess/Plugins/PluginView.h 2016-02-17 21:17:00 UTC (rev 196716)
+++ trunk/Source/WebKit2/WebProcess/Plugins/PluginView.h 2016-02-17 21:27:02 UTC (rev 196717)
@@ -167,6 +167,7 @@
virtual void beginSnapshottingRunningPlugin() override;
virtual bool shouldAllowNavigationFromDrags() const override;
virtual bool shouldNotAddLayer() const override;
+ virtual void willDetatchRenderer() override;
// WebCore::Widget
virtual void setFrameRect(const WebCore::IntRect&) override;
