Reproducible null deref under ScriptedAnimationController::createDisplayRefreshMonitor
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Mar 2015 19:15:54 +0000 (19:15 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Mar 2015 19:15:54 +0000 (19:15 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@181656 268f45cc-cd09-0410-ab3c-d52691b4dbfc

19 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/animation/request-animation-frame-unparented-iframe-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/animation/request-animation-frame-unparented-iframe-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ScriptedAnimationController.cpp
Source/WebCore/dom/ScriptedAnimationController.h
Source/WebCore/page/ChromeClient.h
Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp
Source/WebCore/platform/graphics/DisplayRefreshMonitor.h
Source/WebCore/platform/graphics/DisplayRefreshMonitorClient.h
Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp
Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h
Source/WebCore/platform/graphics/GraphicsLayerUpdater.cpp
Source/WebCore/platform/graphics/GraphicsLayerUpdater.h
Source/WebCore/rendering/RenderLayerCompositor.cpp
Source/WebCore/rendering/RenderLayerCompositor.h
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp
Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h

index e4cc665..e08b475 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         Implement Scroll Container Animation Triggers
diff --git a/LayoutTests/fast/animation/request-animation-frame-unparented-iframe-crash-expected.txt b/LayoutTests/fast/animation/request-animation-frame-unparented-iframe-crash-expected.txt
new file mode 100644 (file)
index 0000000..654ddf7
--- /dev/null
@@ -0,0 +1 @@
+This test passes if it does not crash.
diff --git a/LayoutTests/fast/animation/request-animation-frame-unparented-iframe-crash.html b/LayoutTests/fast/animation/request-animation-frame-unparented-iframe-crash.html
new file mode 100644 (file)
index 0000000..4f6bcac
--- /dev/null
@@ -0,0 +1,22 @@
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+
+window.onload = function () {
+    var frame = document.getElementById("frame");
+    var frameWindow = frame.contentWindow;
+    var rAF = frameWindow.requestAnimationFrame;
+
+    frame.parentElement.removeChild(frame);
+    rAF.call(frameWindow, function () { });
+
+    if (window.testRunner)
+        testRunner.notifyDone();
+};
+</script>
+<body>
+<iframe id="frame"></iframe>
+This test passes if it does not crash.
+</body>
index 5384a6b..94e36a0 100644 (file)
@@ -1,3 +1,62 @@
+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>
 
         Implement Scroll Container Animation Triggers
index f906901..885421d 100644 (file)
@@ -226,9 +226,11 @@ void ScriptedAnimationController::displayRefreshFired(double monotonicTimeNow)
 
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-PassRefPtr<DisplayRefreshMonitor> ScriptedAnimationController::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
+Optional<RefPtr<DisplayRefreshMonitor>> ScriptedAnimationController::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
 {
-    return m_document->page()->chrome().client().createDisplayRefreshMonitor(displayID);
+    if (!m_document->page())
+        return Optional<RefPtr<DisplayRefreshMonitor>>(nullptr);
+    return Optional<RefPtr<DisplayRefreshMonitor>>(m_document->page()->chrome().client().createDisplayRefreshMonitor(displayID));
 }
 #endif
 
index 69b6dc2..1ace58b 100644 (file)
@@ -91,7 +91,7 @@ private:
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
     // 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;
     bool m_isThrottled;
index 0cc7b15..99da950 100644 (file)
@@ -290,7 +290,7 @@ public:
     virtual GraphicsLayerFactory* graphicsLayerFactory() const { return nullptr; }
 
 #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
 
     // Pass 0 as the GraphicsLayer to detatch the root layer.
index 0559dc5..fa8c02b 100644 (file)
 
 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)
     return DisplayRefreshMonitorMac::create(displayID);
index 98ad302..5c520c4 100644 (file)
@@ -41,7 +41,7 @@ class DisplayRefreshMonitorClient;
 
 class DisplayRefreshMonitor : public RefCounted<DisplayRefreshMonitor> {
 public:
-    static PassRefPtr<DisplayRefreshMonitor> create(DisplayRefreshMonitorClient*);
+    static RefPtr<DisplayRefreshMonitor> create(DisplayRefreshMonitorClient*);
     WEBCORE_EXPORT virtual ~DisplayRefreshMonitor();
     
     // Return true if callback request was scheduled, false if it couldn't be
index b6ee8d6..22fa572 100644 (file)
@@ -29,6 +29,7 @@
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
 
 #include "PlatformScreen.h"
+#include <wtf/Optional.h>
 
 namespace WebCore {
 
@@ -43,7 +44,10 @@ public:
     // Always called on the main thread.
     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; }
     bool hasDisplayID() const { return m_displayIDIsSet; }
index 411c2ce..4cf9a49 100644 (file)
@@ -44,7 +44,7 @@ DisplayRefreshMonitorManager& DisplayRefreshMonitorManager::sharedManager()
     return manager.get();
 }
 
-DisplayRefreshMonitor* DisplayRefreshMonitorManager::ensureMonitorForClient(DisplayRefreshMonitorClient* client)
+DisplayRefreshMonitor* DisplayRefreshMonitorManager::createMonitorForClient(DisplayRefreshMonitorClient* client)
 {
     PlatformDisplayID clientDisplayID = client->displayID();
     for (const RefPtr<DisplayRefreshMonitor>& monitor : m_monitors) {
@@ -55,6 +55,8 @@ DisplayRefreshMonitor* DisplayRefreshMonitorManager::ensureMonitorForClient(Disp
     }
 
     RefPtr<DisplayRefreshMonitor> monitor = DisplayRefreshMonitor::create(client);
+    if (!monitor)
+        return nullptr;
     monitor->addClient(client);
     DisplayRefreshMonitor* result = monitor.get();
     m_monitors.append(monitor.release());
@@ -66,7 +68,7 @@ void DisplayRefreshMonitorManager::registerClient(DisplayRefreshMonitorClient* c
     if (!client->hasDisplayID())
         return;
 
-    ensureMonitorForClient(client);
+    createMonitorForClient(client);
 }
 
 void DisplayRefreshMonitorManager::unregisterClient(DisplayRefreshMonitorClient* client)
@@ -92,7 +94,9 @@ bool DisplayRefreshMonitorManager::scheduleAnimation(DisplayRefreshMonitorClient
     if (!client->hasDisplayID())
         return false;
 
-    DisplayRefreshMonitor* monitor = ensureMonitorForClient(client);
+    DisplayRefreshMonitor* monitor = createMonitorForClient(client);
+    if (!monitor)
+        return false;
 
     client->setIsScheduled(true);
     return monitor->requestRefreshCallback();
index 9c43560..92111ce 100644 (file)
@@ -54,7 +54,7 @@ private:
     DisplayRefreshMonitorManager() { }
     virtual ~DisplayRefreshMonitorManager();
 
-    DisplayRefreshMonitor* ensureMonitorForClient(DisplayRefreshMonitorClient*);
+    DisplayRefreshMonitor* createMonitorForClient(DisplayRefreshMonitorClient*);
 
     Vector<RefPtr<DisplayRefreshMonitor>> m_monitors;
 };
index a070bc1..fa18715 100644 (file)
@@ -81,9 +81,11 @@ void GraphicsLayerUpdater::displayRefreshFired(double timestamp)
 #endif
 
 #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
 
index ef25dfa..b6dbd88 100644 (file)
@@ -38,7 +38,7 @@ public:
     virtual ~GraphicsLayerUpdaterClient() { }
     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
 };
 
@@ -55,7 +55,7 @@ public:
     void screenDidChange(PlatformDisplayID);
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-    virtual PassRefPtr<DisplayRefreshMonitor> createDisplayRefreshMonitor(PlatformDisplayID) const override;
+    virtual Optional<RefPtr<DisplayRefreshMonitor>> createDisplayRefreshMonitor(PlatformDisplayID) const override;
 #endif
 
 private:
index c0495f8..c4eec2c 100644 (file)
@@ -4150,14 +4150,14 @@ void RenderLayerCompositor::paintRelatedMilestonesTimerFired()
 }
 
 #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 Optional<RefPtr<DisplayRefreshMonitor>>(nullptr);
 
-    return page->chrome().client().createDisplayRefreshMonitor(displayID);
+    return Optional<RefPtr<DisplayRefreshMonitor>>(page->chrome().client().createDisplayRefreshMonitor(displayID));
 }
 #endif
 
index de5d693..0cf0256 100644 (file)
@@ -399,7 +399,7 @@ private:
     ScrollingCoordinator* scrollingCoordinator() const;
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-    PassRefPtr<DisplayRefreshMonitor> createDisplayRefreshMonitor(PlatformDisplayID) const override;
+    Optional<RefPtr<DisplayRefreshMonitor>> createDisplayRefreshMonitor(PlatformDisplayID) const override;
 #endif
 
     bool requiresCompositingForAnimation(RenderLayerModelObject&) const;
index cf5e157..a97bac0 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         Disk cache should support Vary: Cookie
index 5dae743..944fe6d 100644 (file)
@@ -850,9 +850,9 @@ GraphicsLayerFactory* WebChromeClient::graphicsLayerFactory() const
 }
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-PassRefPtr<WebCore::DisplayRefreshMonitor> WebChromeClient::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
+Optional<RefPtr<WebCore::DisplayRefreshMonitor>> WebChromeClient::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
 {
-    return m_page->drawingArea()->createDisplayRefreshMonitor(displayID);
+    return Optional<RefPtr<WebCore::DisplayRefreshMonitor>>(m_page->drawingArea()->createDisplayRefreshMonitor(displayID));
 }
 #endif
 
index 35daadf..fff49b3 100644 (file)
@@ -217,7 +217,7 @@ private:
     virtual bool adjustLayerFlushThrottling(WebCore::LayerFlushThrottleState::Flags) override;
 
 #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
 
     virtual CompositingTriggerFlags allowedCompositingTriggers() const override