HTMLMediaPlayer should free m_player when src is set/changed
authorfischman@chromium.org <fischman@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Nov 2012 01:38:30 +0000 (01:38 +0000)
committerfischman@chromium.org <fischman@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Nov 2012 01:38:30 +0000 (01:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=99647

Reviewed by Eric Carlson.

.:

* ManualTests/media-players-are-dropped-on-error.html: Added.
    Various scenarios are tested to make sure players aren't
    leaked in different ways for each of them.

Source/WebCore:

New ManualTest added; manual since leaking media players doesn't have layoutTestController-visible effects.

* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::parseAttribute): clearMediaPlayer() when src is set/changed
(WebCore::HTMLMediaElement::userCancelledLoad): use new clearMediaPlayer() helper
(WebCore::HTMLMediaElement::clearMediaPlayer): clear m_player and associated timers/flags
(WebCore):
(WebCore::HTMLMediaElement::createMediaPlayer): whitespace-only change
* html/HTMLMediaElement.h: new method: createMediaPlayer().
(HTMLMediaElement):

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

ChangeLog
ManualTests/media-players-are-dropped-on-error.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLMediaElement.cpp
Source/WebCore/html/HTMLMediaElement.h

index b4ee463..53e2b65 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2012-11-01  Ami Fischman  <fischman@chromium.org>
+
+        HTMLMediaPlayer should free m_player when src is set/changed
+        https://bugs.webkit.org/show_bug.cgi?id=99647
+
+        Reviewed by Eric Carlson.
+
+        * ManualTests/media-players-are-dropped-on-error.html: Added.
+            Various scenarios are tested to make sure players aren't
+            leaked in different ways for each of them.
+
 2012-11-01  Beth Dakin  <bdakin@apple.com>
 
         https://bugs.webkit.org/show_bug.cgi?id=100917
diff --git a/ManualTests/media-players-are-dropped-on-error.html b/ManualTests/media-players-are-dropped-on-error.html
new file mode 100644 (file)
index 0000000..0389668
--- /dev/null
@@ -0,0 +1,40 @@
+<html>
+<head>
+<script>
+var urls = [
+    "file:///does not exist oh noes/test.mp4",
+    "../LayoutTests/media/content/test-25fps.mp4"
+];
+var kickoffFunctions = [
+    "load",
+    "play"
+];
+
+var mediaElementHolder = [];
+
+function releaseAndAddMediaElements() {
+    for (var i = 0; i < mediaElementHolder.length; ++i)
+        mediaElementHolder[i].src = "";
+    mediaElementHolder = [];
+
+    for (var i = 0; i < 5; ++i) {
+        for (var url in urls) {
+            for (var kickoffFunction in kickoffFunctions) {
+                var a = document.createElement('video');
+                a.controls = "controls";
+                a.src = urls[url];
+                eval("a." + kickoffFunctions[kickoffFunction] + "();");
+                mediaElementHolder.push(a);
+            }
+        }
+    }
+}
+</script>
+</head>
+<body onload="setInterval('releaseAndAddMediaElements()', 100)">
+    Test that media players aren't leaked on error.
+    Load this page and verify the number of threads used by the browser doesn't
+    seem unreasonable (e.g. chrome uses 4-5 threads per video tag so staying
+    under 100 threads is "success", since this instantiates 20 &lt;video&gt; elements).
+</body>
+</html>
index b2aeedf..db7b301 100644 (file)
@@ -1,3 +1,21 @@
+2012-11-01  Ami Fischman  <fischman@chromium.org>
+
+        HTMLMediaPlayer should free m_player when src is set/changed
+        https://bugs.webkit.org/show_bug.cgi?id=99647
+
+        Reviewed by Eric Carlson.
+
+        New ManualTest added; manual since leaking media players doesn't have layoutTestController-visible effects.
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::parseAttribute): clearMediaPlayer() when src is set/changed
+        (WebCore::HTMLMediaElement::userCancelledLoad): use new clearMediaPlayer() helper
+        (WebCore::HTMLMediaElement::clearMediaPlayer): clear m_player and associated timers/flags
+        (WebCore):
+        (WebCore::HTMLMediaElement::createMediaPlayer): whitespace-only change
+        * html/HTMLMediaElement.h: new method: createMediaPlayer().
+        (HTMLMediaElement):
+
 2012-11-01  Tom Sepez  <tsepez@chromium.org>
 
         XSS blocker false positive when page contains <iframe src="">
index e9cd1af..c882e62 100644 (file)
@@ -363,8 +363,10 @@ void HTMLMediaElement::parseAttribute(const Attribute& attribute)
 {
     if (attribute.name() == srcAttr) {
         // Trigger a reload, as long as the 'src' attribute is present.
-        if (fastHasAttribute(srcAttr))
+        if (fastHasAttribute(srcAttr)) {
+            clearMediaPlayer(MediaResource);
             scheduleLoad(MediaResource);
+        }
     } else if (attribute.name() == controlsAttr)
         configureMediaControls();
 #if PLATFORM(MAC)
@@ -3669,13 +3671,7 @@ void HTMLMediaElement::userCancelledLoad()
     // If the media data fetching process is aborted by the user:
 
     // 1 - The user agent should cancel the fetching process.
-#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
-    m_player.clear();
-#endif
-    stopPeriodicTimers();
-    m_loadTimer.stop();
-    m_loadState = WaitingForSource;
-    m_pendingLoadFlags = 0;
+    clearMediaPlayer(-1);
 
     // 2 - Set the error attribute to a new MediaError object whose code attribute is set to MEDIA_ERR_ABORTED.
     m_error = MediaError::create(MediaError::MEDIA_ERR_ABORTED);
@@ -3713,6 +3709,18 @@ void HTMLMediaElement::userCancelledLoad()
 #endif
 }
 
+void HTMLMediaElement::clearMediaPlayer(unsigned flags)
+{
+#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
+    m_player.clear();
+#endif
+    stopPeriodicTimers();
+    m_loadTimer.stop();
+
+    m_pendingLoadFlags &= ~flags;
+    m_loadState = WaitingForSource;
+}
+
 bool HTMLMediaElement::canSuspend() const
 {
     return true; 
@@ -4263,7 +4271,7 @@ void HTMLMediaElement::createMediaPlayer()
     if (m_audioSourceNode)
         m_audioSourceNode->lock();
 #endif
-        
+
     m_player = MediaPlayer::create(this);
 
 #if ENABLE(MEDIA_SOURCE)
index c289fff..3ae9f2f 100644 (file)
@@ -464,6 +464,7 @@ private:
     void scheduleNextSourceChild();
     void loadNextSourceChild();
     void userCancelledLoad();
+    void clearMediaPlayer(unsigned flags);
     bool havePotentialSourceChild();
     void noneSupported();
     void mediaEngineError(PassRefPtr<MediaError> err);