Changeset 196717 in webkit


Ignore:
Timestamp:
Feb 17, 2016, 1:27:02 PM (9 years ago)
Author:
Simon Fraser
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:
Location:
trunk/Source
Files:
11 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r196715 r196717  
     12016-02-17  Simon Fraser  <simon.fraser@apple.com>
     2
     3        PDFPlugin's scrollableArea container is not properly unregistered when page is going into the PageCache
     4        https://bugs.webkit.org/show_bug.cgi?id=148182
     5
     6        Reviewed by Brent Fulgham.
     7
     8        When handling Command-arrow key while showing a scrollable PDF, the timing of PDFPlugin
     9        teardown and navigation could result in PDFPlugin::destroy() getting the wrong FrameView,
     10        so the old FrameView was left with a stale pointer in its scrollableAreaSet.
     11
     12        Fix this by adding an explicit willDetatchRenderer() which is called on the plugin
     13        before the Frame gets a new FrameView.
     14
     15        Also narrow the scope of the RefPtr<Widget> in HTMLPlugInElement::defaultEventHandler()
     16        so that the Widget is not kept alive over a possible navigation.
     17
     18        I was unable to make an automated test, because reproducing the bug requires handling
     19        a Command-arrow key event in a way that the last ref to a Widget is held over the event
     20        handling, and this wasn't possible in an iframe.
     21
     22        * html/HTMLPlugInElement.cpp:
     23        (WebCore::HTMLPlugInElement::defaultEventHandler):
     24        * html/HTMLPlugInImageElement.cpp:
     25        (WebCore::HTMLPlugInImageElement::willDetachRenderers):
     26        * plugins/PluginViewBase.h:
     27        (WebCore::PluginViewBase::willDetatchRenderer):
     28        * style/StyleTreeResolver.cpp:
     29        (WebCore::Style::detachRenderTree): Drive-by nullptr.
     30
    1312016-02-17  Brady Eidson  <beidson@apple.com>
    232

  • trunk/Source/WebCore/html/HTMLPlugInElement.cpp

    r194819 r196717  
    226226    }
    227227
    228     RefPtr<Widget> widget = downcast<RenderWidget>(*renderer).widget();
    229     if (!widget)
    230         return;
    231     widget->handleEvent(event);
    232     if (event->defaultHandled())
    233         return;
     228    // Don't keep the widget alive over the defaultEventHandler call, since that can do things like navigate.
     229    {
     230        RefPtr<Widget> widget = downcast<RenderWidget>(*renderer).widget();
     231        if (!widget)
     232            return;
     233        widget->handleEvent(event);
     234        if (event->defaultHandled())
     235            return;
     236    }
    234237    HTMLFrameOwnerElement::defaultEventHandler(event);
    235238}

  • trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp

    r194819 r196717  
    273273    }
    274274
     275    Widget* widget = pluginWidget(PluginLoadingPolicy::DoNotLoad);
     276    if (is<PluginViewBase>(widget))
     277        downcast<PluginViewBase>(*widget).willDetatchRenderer();
     278
    275279    HTMLPlugInElement::willDetachRenderers();
    276280}

  • trunk/Source/WebCore/plugins/PluginViewBase.h

    r189651 r196717  
    8282
    8383    virtual RefPtr<JSC::Bindings::Instance> bindingInstance() { return nullptr; }
     84   
     85    virtual void willDetatchRenderer() { }
    8486
    8587protected:

  • trunk/Source/WebCore/style/StyleTreeResolver.cpp

    r196669 r196717  
    614614    if (current.renderer())
    615615        current.renderer()->destroyAndCleanupAnonymousWrappers();
    616     current.setRenderer(0);
     616    current.setRenderer(nullptr);
    617617
    618618    if (current.hasCustomStyleResolveCallbacks())

  • trunk/Source/WebKit2/ChangeLog

    r196715 r196717  
     12016-02-17  Simon Fraser  <simon.fraser@apple.com>
     2
     3        PDFPlugin's scrollableArea container is not properly unregistered when page is going into the PageCache
     4        https://bugs.webkit.org/show_bug.cgi?id=148182
     5
     6        Reviewed by Brent Fulgham.
     7       
     8        When handling Command-arrow key while showing a scrollable PDF, the timing of PDFPlugin
     9        teardown and navigation could result in PDFPlugin::destroy() getting the wrong FrameView,
     10        so the old FrameView was left with a stale pointer in its scrollableAreaSet.
     11
     12        Fix this by adding an explicit willDetatchRenderer() which is called on the plugin
     13        before the Frame gets a new FrameView.
     14
     15        Also narrow the scope of the RefPtr<Widget> in HTMLPlugInElement::defaultEventHandler()
     16        so that the Widget is not kept alive over a possible navigation.
     17
     18        I was unable to make an automated test, because reproducing the bug requires handling
     19        a Command-arrow key event in a way that the last ref to a Widget is held over the event
     20        handling, and this wasn't possible in an iframe.
     21
     22        * WebProcess/Plugins/PDF/DeprecatedPDFPlugin.h:
     23        * WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm:
     24        (WebKit::PDFPlugin::willDetatchRenderer):
     25        * WebProcess/Plugins/Plugin.h:
     26        (WebKit::Plugin::willDetatchRenderer):
     27        * WebProcess/Plugins/PluginView.cpp:
     28        (WebKit::PluginView::willDetatchRenderer):
     29        * WebProcess/Plugins/PluginView.h:
     30
    1312016-02-17  Brady Eidson  <beidson@apple.com>
    232

  • trunk/Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.h

    r194515 r196717  
    164164    virtual bool handleScroll(WebCore::ScrollDirection, WebCore::ScrollGranularity) override;
    165165    virtual RefPtr<WebCore::SharedBuffer> liveResourceData() const override;
     166    virtual void willDetatchRenderer() override;
    166167
    167168    virtual bool isBeingAsynchronouslyInitialized() const override { return false; }

  • trunk/Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm

    r195743 r196717  
    10921092}
    10931093
     1094void PDFPlugin::willDetatchRenderer()
     1095{
     1096    if (webFrame()) {
     1097        if (FrameView* frameView = webFrame()->coreFrame()->view())
     1098            frameView->removeScrollableArea(this);
     1099    }
     1100}
     1101
    10941102void PDFPlugin::destroy()
    10951103{

  • trunk/Source/WebKit2/WebProcess/Plugins/Plugin.h

    r191922 r196717  
    302302    virtual bool requiresUnifiedScaleFactor() const { return false; }
    303303
     304    virtual void willDetatchRenderer() { }
     305
    304306protected:
    305307    Plugin(PluginType);

  • trunk/Source/WebKit2/WebProcess/Plugins/PluginView.cpp

    r196505 r196717  
    10061006}
    10071007
     1008void PluginView::willDetatchRenderer()
     1009{
     1010    if (!m_isInitialized || !m_plugin)
     1011        return;
     1012
     1013    m_plugin->willDetatchRenderer();
     1014}
     1015
    10081016PassRefPtr<SharedBuffer> PluginView::liveResourceData() const
    10091017{

  • trunk/Source/WebKit2/WebProcess/Plugins/PluginView.h

    r196505 r196717  
    168168    virtual bool shouldAllowNavigationFromDrags() const override;
    169169    virtual bool shouldNotAddLayer() const override;
     170    virtual void willDetatchRenderer() override;
    170171
    171172    // WebCore::Widget
Note: See TracChangeset for help on using the changeset viewer.