Use-after-free in media player handling
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Nov 2012 21:00:23 +0000 (21:00 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Nov 2012 21:00:23 +0000 (21:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=103426

Patch by Aaron Colwell <acolwell@chromium.org> on 2012-11-27
Reviewed by Eric Carlson.

Source/WebCore:

Fixed use-after-free bugs caused by the MediaSource not being closed before the HTMLMediaElement or the MediaPlayer
is destroyed. Closing the MediaSource causes it to clear its reference to the MediaPlayer which prevents
the use-after-free problems from happening.

Test: http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction.html

* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::~HTMLMediaElement):
(WebCore::HTMLMediaElement::clearMediaPlayer):

LayoutTests:

- Added a test to verify that the MediaSource gets closed when the HTMLMediaElement is destroyed
  after it is removed from the DOM.
- Updated video-media-source-state-changes-expected.txt to reflect a slight change in event dispatch ordering.

* http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction-expected.txt: Added.
* http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction.html: Added.
* http/tests/media/media-source/video-media-source-state-changes-expected.txt:

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

LayoutTests/ChangeLog
LayoutTests/http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction.html [new file with mode: 0644]
LayoutTests/http/tests/media/media-source/video-media-source-state-changes-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLMediaElement.cpp

index 348d3e0..4ac65de 100644 (file)
@@ -1,3 +1,19 @@
+2012-11-27  Aaron Colwell  <acolwell@chromium.org>
+
+        Use-after-free in media player handling
+        https://bugs.webkit.org/show_bug.cgi?id=103426
+
+        Reviewed by Eric Carlson.
+
+        - Added a test to verify that the MediaSource gets closed when the HTMLMediaElement is destroyed
+          after it is removed from the DOM.
+        - Updated video-media-source-state-changes-expected.txt to reflect a slight change in event dispatch ordering.
+
+
+        * http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction-expected.txt: Added.
+        * http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction.html: Added.
+        * http/tests/media/media-source/video-media-source-state-changes-expected.txt:
+
 2012-11-27  David Grogan  <dgrogan@chromium.org>
 
         IndexedDB: Remove IDBDatabase.setVersion API
diff --git a/LayoutTests/http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction-expected.txt b/LayoutTests/http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction-expected.txt
new file mode 100644 (file)
index 0000000..093fb1f
--- /dev/null
@@ -0,0 +1,9 @@
+Tests that the MediaSource is closed when the HTMLMediaElement is destroyed.
+
+sourceOpened called.
+Removing video element from DOM.
+Running the garbage collector.
+Setting timestampOffset failed as expected: InvalidStateError
+sourceClosed called.
+END OF TEST
+
diff --git a/LayoutTests/http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction.html b/LayoutTests/http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction.html
new file mode 100644 (file)
index 0000000..98f0197
--- /dev/null
@@ -0,0 +1,56 @@
+<!DOCTYPE html>
+<html>
+    <head>
+      <script src="/media-resources/video-test.js"></script>
+      <script src="media-source.js"></script>
+
+      <script>
+          var ms = new WebKitMediaSource();
+
+          function sourceOpened()
+          {
+              consoleWrite("sourceOpened called.");
+              var vid = document.getElementById('vid');
+              var buffer = ms.addSourceBuffer('video/webm; codecs="vorbis,vp8"');
+
+              MediaSourceTest.expectSourceState(ms, "open");
+
+              consoleWrite("Removing video element from DOM.");
+              vid.parentNode.removeChild(vid);
+              vid = null;
+
+              consoleWrite("Running the garbage collector.");
+              gc();
+
+              MediaSourceTest.expectSourceState(ms, "closed");
+
+              try {
+                  buffer.timestampOffset = 42;
+                  failTest("Expected an exception");
+              } catch (e) {
+                  consoleWrite("Setting timestampOffset failed as expected: " + e.name);
+              }
+          }
+
+          function sourceClosed()
+          {
+              consoleWrite("sourceClosed called.");
+              endTest();
+          }
+
+          function onLoad()
+          {
+              waitForEventAndFail('error');
+
+              ms.addEventListener('webkitsourceopen', sourceOpened);
+              ms.addEventListener('webkitsourceclose', sourceClosed);
+
+              document.getElementById('vid').src = window.URL.createObjectURL(ms);
+          }
+      </script>
+    </head>
+    <body onload="onLoad()">
+      <video id="vid"></video>
+      <p>Tests that the MediaSource is closed when the HTMLMediaElement is destroyed.</p>
+    </body>
+</html>
index 72bcd72..37905e2 100644 (file)
@@ -16,18 +16,18 @@ EVENT(seeking)
 onSecondSeeking
 EVENT(seeked)
 onSecondSeeked
-EVENT(emptied)
 EVENT(webkitsourceclose) : closed
 onFirstSourceClose
+EVENT(emptied)
 EVENT(webkitsourceopen) : open
 onSecondSourceOpen
 EVENT(webkitsourceended) : ended
 onSecondSourceEnded
 EVENT(playing)
 triggerSecondSourceClose
-EVENT(emptied)
 EVENT(webkitsourceclose) : closed
 onSecondSourceClose
+EVENT(emptied)
 EVENT(webkitsourceopen) : open
 onThirdSourceOpen
 END OF TEST
index 5939308..794ed0d 100644 (file)
@@ -1,3 +1,20 @@
+2012-11-27  Aaron Colwell  <acolwell@chromium.org>
+
+        Use-after-free in media player handling
+        https://bugs.webkit.org/show_bug.cgi?id=103426
+
+        Reviewed by Eric Carlson.
+
+        Fixed use-after-free bugs caused by the MediaSource not being closed before the HTMLMediaElement or the MediaPlayer
+        is destroyed. Closing the MediaSource causes it to clear its reference to the MediaPlayer which prevents
+        the use-after-free problems from happening.
+
+        Test: http/tests/media/media-source/video-media-source-closed-on-htmlmediaelement-destruction.html
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::~HTMLMediaElement):
+        (WebCore::HTMLMediaElement::clearMediaPlayer):
+
 2012-11-27  David Grogan  <dgrogan@chromium.org>
 
         IndexedDB: Remove IDBDatabase.setVersion API
index ffa68dc..85ae0b7 100644 (file)
@@ -316,6 +316,10 @@ HTMLMediaElement::~HTMLMediaElement()
     if (m_mediaController)
         m_mediaController->removeMediaElement(this);
 
+#if ENABLE(MEDIA_SOURCE)
+    setSourceState(MediaSource::closedKeyword());
+#endif
+
     removeElementFromDocumentMap(this, document());
 }
 
@@ -3750,6 +3754,11 @@ void HTMLMediaElement::userCancelledLoad()
 void HTMLMediaElement::clearMediaPlayer(int flags)
 {
 #if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
+
+#if ENABLE(MEDIA_SOURCE)
+    setSourceState(MediaSource::closedKeyword());
+#endif
+
     m_player.clear();
 #endif
     stopPeriodicTimers();