Potential use-after-free with an event fired at a HTMLMediaElement which is currently...
authoreric.carlson@apple.com <eric.carlson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Jun 2013 17:18:59 +0000 (17:18 +0000)
committereric.carlson@apple.com <eric.carlson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Jun 2013 17:18:59 +0000 (17:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=117466

Reviewed by Oliver Hunt.

Merge https://chromium.googlesource.com/chromium/blink/+/f4200a0093b3d9376f703961615359ec7fb712b4

If an event is created using as target an HTMLMediaElement which is
currently being deleted it becomes a heap-use-after free situation.

The GenericEventQueue instance is already owned by the HTMLMediaElement,
and there already is an underlying mechanism to set the target of the
event to NULL, if their target is owner of the queue.

In order to avoid creating this reference in the first place, we enqueue
the event with a NULL target to defer the refcount increment until the
timer for dispatching the event happens (which won't happen at all if
garbage collection is already destroying the objects).

Source/WebCore:

Test: media/track/media-element-enqueue-event-crash.html

* dom/GenericEventQueue.cpp:
(WebCore::GenericEventQueue::enqueueEvent): Don't ASSERT if the event has no target.

* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::~HTMLMediaElement): Close the event queue so it won't try to
    dispatch any pending events.
(WebCore::HTMLMediaElement::scheduleEvent): Don't set the event target, it will happen just
    prior to event dispatch.
(WebCore::HTMLMediaElement::stop): Close the event queue.

LayoutTests:

* media/track/media-element-enqueue-event-crash-expected.txt: Added.
* media/track/media-element-enqueue-event-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/media/track/media-element-enqueue-event-crash-expected.txt [new file with mode: 0644]
LayoutTests/media/track/media-element-enqueue-event-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/GenericEventQueue.cpp
Source/WebCore/html/HTMLMediaElement.cpp

index 5fd8564..743f129 100644 (file)
@@ -1,3 +1,27 @@
+2013-06-14  Eric Carlson  <eric.carlson@apple.com>
+
+        Potential use-after-free with an event fired at a HTMLMediaElement which is currently being deleted
+        https://bugs.webkit.org/show_bug.cgi?id=117466
+
+        Reviewed by Oliver Hunt.
+        
+        Merge https://chromium.googlesource.com/chromium/blink/+/f4200a0093b3d9376f703961615359ec7fb712b4
+        
+        If an event is created using as target an HTMLMediaElement which is
+        currently being deleted it becomes a heap-use-after free situation.
+
+        The GenericEventQueue instance is already owned by the HTMLMediaElement,
+        and there already is an underlying mechanism to set the target of the
+        event to NULL, if their target is owner of the queue.
+
+        In order to avoid creating this reference in the first place, we enqueue
+        the event with a NULL target to defer the refcount increment until the
+        timer for dispatching the event happens (which won't happen at all if
+        garbage collection is already destroying the objects).
+
+        * media/track/media-element-enqueue-event-crash-expected.txt: Added.
+        * media/track/media-element-enqueue-event-crash.html: Added.
+
 2013-06-14  Gabor Abraham  <abrhm@inf.u-szeged.hu>
 
         [Qt] Unreviewed gardening. Generate new baseline to some tests and bot greening.
diff --git a/LayoutTests/media/track/media-element-enqueue-event-crash-expected.txt b/LayoutTests/media/track/media-element-enqueue-event-crash-expected.txt
new file mode 100644 (file)
index 0000000..8fbc051
--- /dev/null
@@ -0,0 +1,4 @@
+Tests that appending events for dispatching doesn't crash
+
+** No crash. Pass **
+
diff --git a/LayoutTests/media/track/media-element-enqueue-event-crash.html b/LayoutTests/media/track/media-element-enqueue-event-crash.html
new file mode 100644 (file)
index 0000000..7f70377
--- /dev/null
@@ -0,0 +1,59 @@
+<!DOCTYPE  html>
+<html>
+    <head>
+        <script src=../video-test.js></script>
+        <script src=../../resources/gc.js></script>
+
+        <script>
+        if (window.testRunner)
+        {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        function startTest()
+        {
+            if (localStorage.testRuns)
+                localStorage.testRuns = Number(localStorage.testRuns) + 1;
+            else {
+                localStorage.testRuns = 1;
+                localStorage.totalRuns = 5;
+            }
+
+            document.getElementsByTagName('track')[0].track.mode = 'showing';
+            setTimeout(CFcrash, 100);
+        }
+
+        function forceGC() {
+            gc();
+
+            // End the test only if it ran at least totalRuns.
+            if (window.testRunner && localStorage.testRuns == localStorage.totalRuns) {
+                consoleWrite("** No crash. Pass **");
+                testRunner.notifyDone();
+            } else
+                window.location.reload();
+        }
+
+        function CFcrash()
+        {
+            document1 = document.implementation.createDocument("", null);
+            document1.appendChild(videoElement);
+            delete document1;
+
+            setTimeout(forceGC, 0);
+        }
+
+        document.addEventListener("DOMContentLoaded", startTest, false);
+        </script>
+    </head>
+
+    <body>
+        <p>Tests that appending events for dispatching doesn't crash</p>
+        <video autoplay id="videoElement">
+            <source src="../content/test.ogv">
+            <source src="../content/test.mp4">
+            <track src="captions-webvtt/captions-fast.vtt" default>
+        </video>
+    </body>
+</html>
index 277893e..1502143 100644 (file)
@@ -1,3 +1,36 @@
+2013-06-14  Eric Carlson  <eric.carlson@apple.com>
+
+        Potential use-after-free with an event fired at a HTMLMediaElement which is currently being deleted
+        https://bugs.webkit.org/show_bug.cgi?id=117466
+
+        Reviewed by Oliver Hunt.
+        
+        Merge https://chromium.googlesource.com/chromium/blink/+/f4200a0093b3d9376f703961615359ec7fb712b4
+        
+        If an event is created using as target an HTMLMediaElement which is
+        currently being deleted it becomes a heap-use-after free situation.
+
+        The GenericEventQueue instance is already owned by the HTMLMediaElement,
+        and there already is an underlying mechanism to set the target of the
+        event to NULL, if their target is owner of the queue.
+
+        In order to avoid creating this reference in the first place, we enqueue
+        the event with a NULL target to defer the refcount increment until the
+        timer for dispatching the event happens (which won't happen at all if
+        garbage collection is already destroying the objects).
+
+        Test: media/track/media-element-enqueue-event-crash.html
+
+        * dom/GenericEventQueue.cpp:
+        (WebCore::GenericEventQueue::enqueueEvent): Don't ASSERT if the event has no target.
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::~HTMLMediaElement): Close the event queue so it won't try to
+            dispatch any pending events.
+        (WebCore::HTMLMediaElement::scheduleEvent): Don't set the event target, it will happen just
+            prior to event dispatch.
+        (WebCore::HTMLMediaElement::stop): Close the event queue.
+
 2013-06-14  Dean Jackson  <dino@apple.com>
 
         Clicking on snapshotting plug-ins does not restart them
index a177d61..a04e489 100644 (file)
@@ -52,7 +52,6 @@ bool GenericEventQueue::enqueueEvent(PassRefPtr<Event> event)
     if (m_isClosed)
         return false;
 
-    ASSERT(event->target());
     if (event->target() == m_owner)
         event->setTarget(0);
 
index f705ba1..2a9e3ac 100644 (file)
@@ -346,6 +346,8 @@ HTMLMediaElement::~HTMLMediaElement()
 {
     LOG(Media, "HTMLMediaElement::~HTMLMediaElement");
 
+    m_asyncEventQueue->close();
+
     if (m_isWaitingUntilMediaCanStart)
         document()->removeMediaCanStartListener(this);
     setShouldDelayLoadEvent(false);
@@ -681,7 +683,9 @@ void HTMLMediaElement::scheduleEvent(const AtomicString& eventName)
     LOG(Media, "HTMLMediaElement::scheduleEvent - scheduling '%s'", eventName.string().ascii().data());
 #endif
     RefPtr<Event> event = Event::create(eventName, false, true);
-    event->setTarget(this);
+    
+    // Don't set the event target, the event queue will set it in GenericEventQueue::timerFired and setting it here
+    // will trigger an ASSERT if this element has been marked for deletion.
 
     m_asyncEventQueue->enqueueEvent(event.release());
 }
@@ -4155,6 +4159,8 @@ void HTMLMediaElement::stop()
     
     stopPeriodicTimers();
     cancelPendingEventsAndCallbacks();
+
+    m_asyncEventQueue->close();
 }
 
 void HTMLMediaElement::suspend(ReasonForSuspension why)