Changeset 181656 in webkit

Timestamp:

Mar 17, 2015, 12:15:54 PM (11 years ago)

Author:

timothy_horton@apple.com

Message:

Reproducible null deref under ScriptedAnimationController::createDisplayRefreshMonitor
​https://bugs.webkit.org/show_bug.cgi?id=142776
<rdar://problem/18921338>

Reviewed by Alexey Proskuryakov.

Test: fast/animation/request-animation-frame-unparented-iframe-crash.html

In some cases (like the new test), we can end up trying to start
requestAnimationFrame on a Document that has no Page. Most paths null-checked
the Page and did the right thing, but one failed to do so. In addition,
the current fallback (when Page is null) can result in us constructing
the wrong kind of DisplayRefreshMonitor, which could lead to trouble
down the road when it's reused. Instead, just completely avoid making a
DisplayRefreshMonitor in the null-page case.

• dom/ScriptedAnimationController.cpp:

(WebCore::ScriptedAnimationController::createDisplayRefreshMonitor):
If the page is null, bail.

• dom/ScriptedAnimationController.h:
• platform/graphics/DisplayRefreshMonitor.cpp:

(WebCore::DisplayRefreshMonitor::create):
Use Optional<> to make it easy to distinguish between ChromeClient
being unreachable (because we don't have a Page for some reason) and
ChromeClient declaring that it doesn't want to override the type of
DisplayRefreshMonitor that is created.

If ChromeClient was unreachable for some reason, we'll get back an engaged
nullptr and return it (instead of creating a DisplayRefreshMonitor based
on the platform). This avoids creating the wrong type of DisplayRefreshMonitor
in the rare case where we can't reach the ChromeClient (e.g. a freshly unparented
IFrame).

If instead the client returns a disengaged Nullopt, we'll interpret that as
"construct the default type", which falls back on the platform #ifdefs to
decide what to make.

• platform/graphics/DisplayRefreshMonitorManager.cpp:

(WebCore::DisplayRefreshMonitorManager::ensureMonitorForClient):
(WebCore::DisplayRefreshMonitorManager::scheduleAnimation):
Silently handle the case where we failed to make a DisplayRefreshMonitor.

• platform/graphics/DisplayRefreshMonitor.h:
• platform/graphics/DisplayRefreshMonitorClient.h:
• platform/graphics/GraphicsLayerUpdater.cpp:

(WebCore::GraphicsLayerUpdater::createDisplayRefreshMonitor):

• platform/graphics/GraphicsLayerUpdater.h:
• rendering/RenderLayerCompositor.cpp:

(WebCore::RenderLayerCompositor::createDisplayRefreshMonitor):

• rendering/RenderLayerCompositor.h:

Adjust to the new signature of createDisplayRefreshMonitor, and return
an engaged (nullptr) Optional if we can't get to ChromeClient for any reason.

• page/ChromeClient.h:

Return Nullopt (indicating a lack of override) by default.

• WebProcess/WebCoreSupport/WebChromeClient.cpp:

(WebKit::WebChromeClient::createDisplayRefreshMonitor):

• WebProcess/WebCoreSupport/WebChromeClient.h:

Adjust to the new signature.

• fast/animation/request-animation-frame-unparented-iframe-crash-expected.txt: Added.
• fast/animation/request-animation-frame-unparented-iframe-crash.html: Added.

Add a test that ensures that calling requestAnimationFrame on a recently-unparented
frame doesn't crash.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/animation/request-animation-frame-unparented-iframe-crash-expected.txt (added)
• LayoutTests/fast/animation/request-animation-frame-unparented-iframe-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/ScriptedAnimationController.cpp (modified) (1 diff)
• Source/WebCore/dom/ScriptedAnimationController.h (modified) (1 diff)
• Source/WebCore/page/ChromeClient.h (modified) (1 diff)
• Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp (modified) (1 diff)
• Source/WebCore/platform/graphics/DisplayRefreshMonitor.h (modified) (1 diff)
• Source/WebCore/platform/graphics/DisplayRefreshMonitorClient.h (modified) (2 diffs)
• Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp (modified) (4 diffs)
• Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h (modified) (1 diff)
• Source/WebCore/platform/graphics/GraphicsLayerUpdater.cpp (modified) (1 diff)
• Source/WebCore/platform/graphics/GraphicsLayerUpdater.h (modified) (2 diffs)
• Source/WebCore/rendering/RenderLayerCompositor.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderLayerCompositor.h (modified) (1 diff)
• Source/WebKit2/ChangeLog (modified) (1 diff)
• Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp (modified) (1 diff)
• Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2015-03-17  Timothy Horton  <timothy_horton@apple.com>
+
+        Reproducible null deref under ScriptedAnimationController::createDisplayRefreshMonitor
+        https://bugs.webkit.org/show_bug.cgi?id=142776
+        <rdar://problem/18921338>
+
+        Reviewed by Alexey Proskuryakov.
+
+        * fast/animation/request-animation-frame-unparented-iframe-crash-expected.txt: Added.
+        * fast/animation/request-animation-frame-unparented-iframe-crash.html: Added.
+        Add a test that ensures that calling requestAnimationFrame on a recently-unparented
+        frame doesn't crash.
+
 2015-03-17  Dean Jackson  <dino@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-03-17  Timothy Horton  <timothy_horton@apple.com>
+
+        Reproducible null deref under ScriptedAnimationController::createDisplayRefreshMonitor
+        https://bugs.webkit.org/show_bug.cgi?id=142776
+        <rdar://problem/18921338>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Test: fast/animation/request-animation-frame-unparented-iframe-crash.html
+
+        In some cases (like the new test), we can end up trying to start
+        requestAnimationFrame on a Document that has no Page. Most paths null-checked
+        the Page and did the right thing, but one failed to do so. In addition,
+        the current fallback (when Page is null) can result in us constructing
+        the wrong kind of DisplayRefreshMonitor, which could lead to trouble
+        down the road when it's reused. Instead, just completely avoid making a
+        DisplayRefreshMonitor in the null-page case.
+
+        * dom/ScriptedAnimationController.cpp:
+        (WebCore::ScriptedAnimationController::createDisplayRefreshMonitor):
+        If the page is null, bail.
+
+        * dom/ScriptedAnimationController.h:
+        * platform/graphics/DisplayRefreshMonitor.cpp:
+        (WebCore::DisplayRefreshMonitor::create):
+        Use Optional<> to make it easy to distinguish between ChromeClient
+        being unreachable (because we don't have a Page for some reason) and
+        ChromeClient declaring that it doesn't want to override the type of
+        DisplayRefreshMonitor that is created.
+
+        If ChromeClient was unreachable for some reason, we'll get back an engaged
+        nullptr and return it (instead of creating a DisplayRefreshMonitor based
+        on the platform). This avoids creating the wrong type of DisplayRefreshMonitor
+        in the rare case where we can't reach the ChromeClient (e.g. a freshly unparented
+        IFrame).
+
+        If instead the client returns a disengaged Nullopt, we'll interpret that as
+        "construct the default type", which falls back on the platform #ifdefs to
+        decide what to make.
+
+        * platform/graphics/DisplayRefreshMonitorManager.cpp:
+        (WebCore::DisplayRefreshMonitorManager::ensureMonitorForClient):
+        (WebCore::DisplayRefreshMonitorManager::scheduleAnimation):
+        Silently handle the case where we failed to make a DisplayRefreshMonitor.
+
+        * platform/graphics/DisplayRefreshMonitor.h:
+        * platform/graphics/DisplayRefreshMonitorClient.h:
+        * platform/graphics/GraphicsLayerUpdater.cpp:
+        (WebCore::GraphicsLayerUpdater::createDisplayRefreshMonitor):
+        * platform/graphics/GraphicsLayerUpdater.h:
+        * rendering/RenderLayerCompositor.cpp:
+        (WebCore::RenderLayerCompositor::createDisplayRefreshMonitor):
+        * rendering/RenderLayerCompositor.h:
+        Adjust to the new signature of createDisplayRefreshMonitor, and return
+        an engaged (nullptr) Optional if we can't get to ChromeClient for any reason.
+
+        * page/ChromeClient.h:
+        Return Nullopt (indicating a lack of override) by default.
+
 2015-03-17  Dean Jackson  <dino@apple.com>
 

trunk/Source/WebCore/dom/ScriptedAnimationController.cpp

@@ -227 +227 @@
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-PassRefPtr<DisplayRefreshMonitor> ScriptedAnimationController::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
-{
-    return m_document->page()->chrome().client().createDisplayRefreshMonitor(displayID);
-}
-#endif
-
-
-}
-
-#endif
+Optional<RefPtr<DisplayRefreshMonitor>> ScriptedAnimationController::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
+{
+    if (!m_document->page())
+        return Optional<RefPtr<DisplayRefreshMonitor>>(nullptr);
+    return Optional<RefPtr<DisplayRefreshMonitor>>(m_document->page()->chrome().client().createDisplayRefreshMonitor(displayID));
+}
+#endif
+
+
+}
+
+#endif

trunk/Source/WebCore/dom/ScriptedAnimationController.h

@@ -92 +92 @@
     // Override for DisplayRefreshMonitorClient
     virtual void displayRefreshFired(double timestamp) override;
-    virtual PassRefPtr<DisplayRefreshMonitor> createDisplayRefreshMonitor(PlatformDisplayID) const override;
+    virtual Optional<RefPtr<DisplayRefreshMonitor>> createDisplayRefreshMonitor(PlatformDisplayID) const override;
 
     bool m_isUsingTimer;

trunk/Source/WebCore/page/ChromeClient.h

@@ -291 +291 @@
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-    virtual PassRefPtr<DisplayRefreshMonitor> createDisplayRefreshMonitor(PlatformDisplayID) const { return nullptr; }
+    virtual Optional<RefPtr<DisplayRefreshMonitor>> createDisplayRefreshMonitor(PlatformDisplayID) const { return Nullopt; }
 #endif
 

trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp

@@ -36 +36 @@
 namespace WebCore {
 
-PassRefPtr<DisplayRefreshMonitor> DisplayRefreshMonitor::create(DisplayRefreshMonitorClient* client)
+RefPtr<DisplayRefreshMonitor> DisplayRefreshMonitor::create(DisplayRefreshMonitorClient* client)
 {
     PlatformDisplayID displayID = client->displayID();
 
-    if (RefPtr<DisplayRefreshMonitor> monitor = client->createDisplayRefreshMonitor(displayID))
-        return monitor.release();
+    if (Optional<RefPtr<DisplayRefreshMonitor>> monitor = client->createDisplayRefreshMonitor(displayID))
+        return monitor.value();
+
+    // If ChromeClient returned Nullopt, we'll go ahead and make one of the default type.
 
 #if PLATFORM(MAC)

trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h

@@ -42 +42 @@
 class DisplayRefreshMonitor : public RefCounted<DisplayRefreshMonitor> {
 public:
-    static PassRefPtr<DisplayRefreshMonitor> create(DisplayRefreshMonitorClient*);
+    static RefPtr<DisplayRefreshMonitor> create(DisplayRefreshMonitorClient*);
     WEBCORE_EXPORT virtual ~DisplayRefreshMonitor();
     

trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitorClient.h

@@ -30 +30 @@
 
 #include "PlatformScreen.h"
+#include <wtf/Optional.h>
 
 namespace WebCore {
@@ -44 +45 @@
     virtual void displayRefreshFired(double timestamp) = 0;
 
-    virtual PassRefPtr<DisplayRefreshMonitor> createDisplayRefreshMonitor(PlatformDisplayID) const = 0;
+    // Returning nullopt indicates that WebCore should create whatever DisplayRefreshMonitor it deems
+    // most appropriate for the current platform. Returning nullptr indicates that we should not try to
+    // create a DisplayRefreshMonitor at all (and should instead fall back to using a timer).
+    virtual Optional<RefPtr<DisplayRefreshMonitor>> createDisplayRefreshMonitor(PlatformDisplayID) const = 0;
 
     PlatformDisplayID displayID() const { return m_displayID; }

trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp

@@ -45 +45 @@
 }
 
-DisplayRefreshMonitor* DisplayRefreshMonitorManager::ensureMonitorForClient(DisplayRefreshMonitorClient* client)
+DisplayRefreshMonitor* DisplayRefreshMonitorManager::createMonitorForClient(DisplayRefreshMonitorClient* client)
 {
     PlatformDisplayID clientDisplayID = client->displayID();
@@ -56 +56 @@
 
     RefPtr<DisplayRefreshMonitor> monitor = DisplayRefreshMonitor::create(client);
+    if (!monitor)
+        return nullptr;
     monitor->addClient(client);
     DisplayRefreshMonitor* result = monitor.get();
@@ -67 +69 @@
         return;
 
-    ensureMonitorForClient(client);
+    createMonitorForClient(client);
 }
 
@@ -93 +95 @@
         return false;
 
-    DisplayRefreshMonitor* monitor = ensureMonitorForClient(client);
+    DisplayRefreshMonitor* monitor = createMonitorForClient(client);
+    if (!monitor)
+        return false;
 
     client->setIsScheduled(true);

trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h

@@ -55 +55 @@
     virtual ~DisplayRefreshMonitorManager();
 
-    DisplayRefreshMonitor* ensureMonitorForClient(DisplayRefreshMonitorClient*);
+    DisplayRefreshMonitor* createMonitorForClient(DisplayRefreshMonitorClient*);
 
     Vector<RefPtr<DisplayRefreshMonitor>> m_monitors;

trunk/Source/WebCore/platform/graphics/GraphicsLayerUpdater.cpp

@@ -82 +82 @@
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-PassRefPtr<DisplayRefreshMonitor> GraphicsLayerUpdater::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
+Optional<RefPtr<DisplayRefreshMonitor>> GraphicsLayerUpdater::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
 {
-    return m_client ? m_client->createDisplayRefreshMonitor(displayID) : nullptr;
+    if (!m_client)
+        return Optional<RefPtr<DisplayRefreshMonitor>>(nullptr);
+    return m_client->createDisplayRefreshMonitor(displayID);
 }
 #endif

trunk/Source/WebCore/platform/graphics/GraphicsLayerUpdater.h

@@ -39 +39 @@
     virtual void flushLayersSoon(GraphicsLayerUpdater*) = 0;
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-    virtual PassRefPtr<DisplayRefreshMonitor> createDisplayRefreshMonitor(PlatformDisplayID) const = 0;
+    virtual Optional<RefPtr<DisplayRefreshMonitor>> createDisplayRefreshMonitor(PlatformDisplayID) const = 0;
 #endif
 };
@@ -56 +56 @@
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-    virtual PassRefPtr<DisplayRefreshMonitor> createDisplayRefreshMonitor(PlatformDisplayID) const override;
+    virtual Optional<RefPtr<DisplayRefreshMonitor>> createDisplayRefreshMonitor(PlatformDisplayID) const override;
 #endif
 

trunk/Source/WebCore/rendering/RenderLayerCompositor.cpp

@@ -4151 +4151 @@
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-PassRefPtr<DisplayRefreshMonitor> RenderLayerCompositor::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
+Optional<RefPtr<DisplayRefreshMonitor>> RenderLayerCompositor::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
 {
     Frame& frame = m_renderView.frameView().frame();
     Page* page = frame.page();
     if (!page)
-        return nullptr;
-
-    return page->chrome().client().createDisplayRefreshMonitor(displayID);
+        return Optional<RefPtr<DisplayRefreshMonitor>>(nullptr);
+
+    return Optional<RefPtr<DisplayRefreshMonitor>>(page->chrome().client().createDisplayRefreshMonitor(displayID));
 }
 #endif

trunk/Source/WebCore/rendering/RenderLayerCompositor.h

@@ -400 +400 @@
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-    PassRefPtr<DisplayRefreshMonitor> createDisplayRefreshMonitor(PlatformDisplayID) const override;
+    Optional<RefPtr<DisplayRefreshMonitor>> createDisplayRefreshMonitor(PlatformDisplayID) const override;
 #endif
 

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2015-03-17  Timothy Horton  <timothy_horton@apple.com>
+
+        Reproducible null deref under ScriptedAnimationController::createDisplayRefreshMonitor
+        https://bugs.webkit.org/show_bug.cgi?id=142776
+        <rdar://problem/18921338>
+
+        Reviewed by Alexey Proskuryakov.
+
+        * WebProcess/WebCoreSupport/WebChromeClient.cpp:
+        (WebKit::WebChromeClient::createDisplayRefreshMonitor):
+        * WebProcess/WebCoreSupport/WebChromeClient.h:
+        Adjust to the new signature.
+
 2015-03-17  Antti Koivisto  <antti@apple.com>
 

trunk/Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp

@@ -851 +851 @@
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-PassRefPtr<WebCore::DisplayRefreshMonitor> WebChromeClient::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
-{
-    return m_page->drawingArea()->createDisplayRefreshMonitor(displayID);
+Optional<RefPtr<WebCore::DisplayRefreshMonitor>> WebChromeClient::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
+{
+    return Optional<RefPtr<WebCore::DisplayRefreshMonitor>>(m_page->drawingArea()->createDisplayRefreshMonitor(displayID));
 }
 #endif

trunk/Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h

@@ -218 +218 @@
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-    virtual PassRefPtr<WebCore::DisplayRefreshMonitor> createDisplayRefreshMonitor(PlatformDisplayID) const override;
+    virtual Optional<RefPtr<WebCore::DisplayRefreshMonitor>> createDisplayRefreshMonitor(PlatformDisplayID) const override;
 #endif