[MSE][Mac] Crash when removing MediaSource from HTMLMediaElement.
authorjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 8 Dec 2013 06:39:39 +0000 (06:39 +0000)
committerjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 8 Dec 2013 06:39:39 +0000 (06:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=125269

Reviewed by Sam Weinig.

Fixes the media/media-source/media-source-fastseek.html test when run with MallocScribble enabled.

It's possible for a SourceBufferPrivateAVFObjC to outlive its MediaSourcePrivateAVFObjC, so
make sure to clear the pointer from the former to the latter when the latter is destroyed.
That means we now have to check to see if the pointer to the latter is still valid at every
call site.

As a drive-by fix, rename m_parent to m_mediaSource to more accurately reflect what the pointer
points to.

* platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.mm:
(WebCore::MediaSourcePrivateAVFObjC::~MediaSourcePrivateAVFObjC): Clear the SourceBuffer's backpointer.
* platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:
(WebCore::SourceBufferPrivateAVFObjC::clearMediaSource):
* platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:
(WebCore::SourceBufferPrivateAVFObjC::SourceBufferPrivateAVFObjC): Rename m_parent -> m_mediaSource.
(WebCore::SourceBufferPrivateAVFObjC::append): Check m_mediaSource before calling.
(WebCore::SourceBufferPrivateAVFObjC::removedFromMediaSource): Ditto.
(WebCore::SourceBufferPrivateAVFObjC::readyState): Ditto.
(WebCore::SourceBufferPrivateAVFObjC::setReadyState): Ditto.
(WebCore::SourceBufferPrivateAVFObjC::trackDidChangeEnabled): Ditto.
(WebCore::SourceBufferPrivateAVFObjC::flushAndEnqueueNonDisplayingSamples): Ditto.
(WebCore::SourceBufferPrivateAVFObjC::enqueueSample): Ditto.
(WebCore::SourceBufferPrivateAVFObjC::setActive): Ditto.
* platform/mock/mediasource/MockMediaSourcePrivate.cpp:
(WebCore::MockMediaSourcePrivate::~MockMediaSourcePrivate): Clear the SourceBuffer's backpointer.
* platform/mock/mediasource/MockSourceBufferPrivate.cpp:
(WebCore::MockSourceBufferPrivate::MockSourceBufferPrivate): Rename m_parent -> m_mediaSource.
(WebCore::MockSourceBufferPrivate::removedFromMediaSource): Check m_mediaSource before calling.
(WebCore::MockSourceBufferPrivate::readyState): Ditto.
(WebCore::MockSourceBufferPrivate::setReadyState): Ditto.
(WebCore::MockSourceBufferPrivate::setActive): Ditto.
* platform/mock/mediasource/MockSourceBufferPrivate.h:
(WebCore::MockSourceBufferPrivate::clearMediaSource):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.mm
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm
Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp
Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp
Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.h

index 4084059..f4f04a4 100644 (file)
@@ -1,3 +1,45 @@
+2013-12-04  Jer Noble  <jer.noble@apple.com>
+
+        [MSE][Mac] Crash when removing MediaSource from HTMLMediaElement.
+        https://bugs.webkit.org/show_bug.cgi?id=125269
+
+        Reviewed by Sam Weinig.
+
+        Fixes the media/media-source/media-source-fastseek.html test when run with MallocScribble enabled.
+
+        It's possible for a SourceBufferPrivateAVFObjC to outlive its MediaSourcePrivateAVFObjC, so
+        make sure to clear the pointer from the former to the latter when the latter is destroyed.
+        That means we now have to check to see if the pointer to the latter is still valid at every
+        call site.
+
+        As a drive-by fix, rename m_parent to m_mediaSource to more accurately reflect what the pointer
+        points to.
+
+        * platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.mm:
+        (WebCore::MediaSourcePrivateAVFObjC::~MediaSourcePrivateAVFObjC): Clear the SourceBuffer's backpointer.
+        * platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:
+        (WebCore::SourceBufferPrivateAVFObjC::clearMediaSource): 
+        * platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:
+        (WebCore::SourceBufferPrivateAVFObjC::SourceBufferPrivateAVFObjC): Rename m_parent -> m_mediaSource.
+        (WebCore::SourceBufferPrivateAVFObjC::append): Check m_mediaSource before calling.
+        (WebCore::SourceBufferPrivateAVFObjC::removedFromMediaSource): Ditto.
+        (WebCore::SourceBufferPrivateAVFObjC::readyState): Ditto.
+        (WebCore::SourceBufferPrivateAVFObjC::setReadyState): Ditto.
+        (WebCore::SourceBufferPrivateAVFObjC::trackDidChangeEnabled): Ditto.
+        (WebCore::SourceBufferPrivateAVFObjC::flushAndEnqueueNonDisplayingSamples): Ditto.
+        (WebCore::SourceBufferPrivateAVFObjC::enqueueSample): Ditto.
+        (WebCore::SourceBufferPrivateAVFObjC::setActive): Ditto.
+        * platform/mock/mediasource/MockMediaSourcePrivate.cpp:
+        (WebCore::MockMediaSourcePrivate::~MockMediaSourcePrivate): Clear the SourceBuffer's backpointer.
+        * platform/mock/mediasource/MockSourceBufferPrivate.cpp:
+        (WebCore::MockSourceBufferPrivate::MockSourceBufferPrivate): Rename m_parent -> m_mediaSource.
+        (WebCore::MockSourceBufferPrivate::removedFromMediaSource): Check m_mediaSource before calling.
+        (WebCore::MockSourceBufferPrivate::readyState): Ditto.
+        (WebCore::MockSourceBufferPrivate::setReadyState): Ditto.
+        (WebCore::MockSourceBufferPrivate::setActive): Ditto.
+        * platform/mock/mediasource/MockSourceBufferPrivate.h:
+        (WebCore::MockSourceBufferPrivate::clearMediaSource):
+
 2013-12-07  Zoltan Horvath  <zoltan@webkit.org>
 
         Remove statusWithDirection static function from RenderBlockLineLayout
index c56b50b..c2d3e38 100644 (file)
@@ -57,6 +57,8 @@ MediaSourcePrivateAVFObjC::MediaSourcePrivateAVFObjC(MediaPlayerPrivateMediaSour
 
 MediaSourcePrivateAVFObjC::~MediaSourcePrivateAVFObjC()
 {
+    for (auto it = m_sourceBuffers.begin(), end = m_sourceBuffers.end(); it != end; ++it)
+        (*it)->clearMediaSource();
 }
 
 MediaSourcePrivate::AddStatus MediaSourcePrivateAVFObjC::addSourceBuffer(const ContentType& contentType, RefPtr<SourceBufferPrivate>& outPrivate)
index 75889c1..2e23fa4 100644 (file)
@@ -59,6 +59,8 @@ public:
     static RefPtr<SourceBufferPrivateAVFObjC> create(MediaSourcePrivateAVFObjC*);
     virtual ~SourceBufferPrivateAVFObjC();
 
+    void clearMediaSource() { m_mediaSource = nullptr; }
+
     // AVStreamDataParser delegate methods
     void didParseStreamDataAsAsset(AVAsset*);
     void didFailToParseStreamDataWithError(NSError*);
@@ -101,7 +103,7 @@ private:
     RetainPtr<AVSampleBufferDisplayLayer> m_displayLayer;
     RetainPtr<NSObject> m_delegate;
 
-    MediaSourcePrivateAVFObjC* m_parent;
+    MediaSourcePrivateAVFObjC* m_mediaSource;
     SourceBufferPrivateClient* m_client;
 
     bool m_parsingSucceeded;
index dd74136..44a593d 100644 (file)
@@ -284,7 +284,7 @@ RefPtr<SourceBufferPrivateAVFObjC> SourceBufferPrivateAVFObjC::create(MediaSourc
 SourceBufferPrivateAVFObjC::SourceBufferPrivateAVFObjC(MediaSourcePrivateAVFObjC* parent)
     : m_parser(adoptNS([[getAVStreamDataParserClass() alloc] init]))
     , m_delegate(adoptNS([[WebAVStreamDataParserListener alloc] initWithParser:m_parser.get() parent:this]))
-    , m_parent(parent)
+    , m_mediaSource(parent)
     , m_client(0)
     , m_parsingSucceeded(true)
 {
@@ -293,7 +293,8 @@ SourceBufferPrivateAVFObjC::SourceBufferPrivateAVFObjC(MediaSourcePrivateAVFObjC
 SourceBufferPrivateAVFObjC::~SourceBufferPrivateAVFObjC()
 {
     if (m_displayLayer) {
-        m_parent->player()->removeDisplayLayer(m_displayLayer.get());
+        if (m_mediaSource)
+            m_mediaSource->player()->removeDisplayLayer(m_displayLayer.get());
         [m_displayLayer flushAndRemoveImage];
         [m_displayLayer stopRequestingMediaData];
         m_displayLayer = nullptr;
@@ -386,8 +387,8 @@ SourceBufferPrivate::AppendResult SourceBufferPrivateAVFObjC::append(const unsig
     LOG(Media, "SourceBufferPrivateAVFObjC::append(%p) - data:%p, length:%d", this, data, length);
     [m_parser appendStreamData:[NSData dataWithBytes:data length:length]];
 
-    if (m_parsingSucceeded)
-        m_parent->player()->setLoadingProgresssed(true);
+    if (m_parsingSucceeded && m_mediaSource)
+        m_mediaSource->player()->setLoadingProgresssed(true);
 
     return m_parsingSucceeded ? AppendSucceeded : ParsingFailed;
 }
@@ -400,23 +401,26 @@ void SourceBufferPrivateAVFObjC::abort()
 void SourceBufferPrivateAVFObjC::removedFromMediaSource()
 {
     if (m_displayLayer) {
-        m_parent->player()->removeDisplayLayer(m_displayLayer.get());
+        if (m_mediaSource)
+            m_mediaSource->player()->removeDisplayLayer(m_displayLayer.get());
         [m_displayLayer flush];
         [m_displayLayer stopRequestingMediaData];
         m_displayLayer = nullptr;
     }
 
-    m_parent->removeSourceBuffer(this);
+    if (m_mediaSource)
+        m_mediaSource->removeSourceBuffer(this);
 }
 
 MediaPlayer::ReadyState SourceBufferPrivateAVFObjC::readyState() const
 {
-    return m_parent->player()->readyState();
+    return m_mediaSource ? m_mediaSource->player()->readyState() : MediaPlayer::HaveNothing;
 }
 
 void SourceBufferPrivateAVFObjC::setReadyState(MediaPlayer::ReadyState readyState)
 {
-    m_parent->player()->setReadyState(readyState);
+    if (m_mediaSource)
+        m_mediaSource->player()->setReadyState(readyState);
 }
 
 void SourceBufferPrivateAVFObjC::evictCodedFrames()
@@ -453,6 +457,8 @@ void SourceBufferPrivateAVFObjC::trackDidChangeEnabled(VideoTrackPrivateMediaSou
     if (!track->selected() && m_enabledVideoTrackID == trackID) {
         m_enabledVideoTrackID = -1;
         [m_parser setShouldProvideMediaData:NO forTrackID:trackID];
+        if (m_mediaSource)
+            m_mediaSource->player()->removeDisplayLayer(m_displayLayer.get());
     } else if (track->selected()) {
         m_enabledVideoTrackID = trackID;
         [m_parser setShouldProvideMediaData:YES forTrackID:trackID];
@@ -462,7 +468,8 @@ void SourceBufferPrivateAVFObjC::trackDidChangeEnabled(VideoTrackPrivateMediaSou
                 if (m_client)
                     m_client->sourceBufferPrivateDidBecomeReadyForMoreSamples(this);
             }];
-            m_parent->player()->addDisplayLayer(m_displayLayer.get());
+            if (m_mediaSource)
+                m_mediaSource->player()->addDisplayLayer(m_displayLayer.get());
         }
     }
 }
@@ -508,7 +515,8 @@ void SourceBufferPrivateAVFObjC::flushAndEnqueueNonDisplayingSamples(Vector<RefP
         [m_displayLayer enqueueSampleBuffer:sampleBuffer.get()];
     }
 
-    m_parent->player()->setHasAvailableVideoFrame(false);
+    if (m_mediaSource)
+        m_mediaSource->player()->setHasAvailableVideoFrame(false);
 }
 
 void SourceBufferPrivateAVFObjC::enqueueSample(PassRefPtr<MediaSample> prpMediaSample, AtomicString trackID)
@@ -523,7 +531,8 @@ void SourceBufferPrivateAVFObjC::enqueueSample(PassRefPtr<MediaSample> prpMediaS
         return;
 
     [m_displayLayer enqueueSampleBuffer:platformSample.sample.cmSampleBuffer];
-    m_parent->player()->setHasAvailableVideoFrame(true);
+    if (m_mediaSource)
+        m_mediaSource->player()->setHasAvailableVideoFrame(true);
 }
 
 bool SourceBufferPrivateAVFObjC::isReadyForMoreSamples()
@@ -533,7 +542,8 @@ bool SourceBufferPrivateAVFObjC::isReadyForMoreSamples()
 
 void SourceBufferPrivateAVFObjC::setActive(bool isActive)
 {
-    m_parent->sourceBufferPrivateDidChangeActiveState(this, isActive);
+    if (m_mediaSource)
+        m_mediaSource->sourceBufferPrivateDidChangeActiveState(this, isActive);
 }
 
 MediaTime SourceBufferPrivateAVFObjC::fastSeekTimeForMediaTime(MediaTime time, MediaTime negativeThreshold, MediaTime positiveThreshold)
index f3878dd..e9f5208 100644 (file)
@@ -49,6 +49,8 @@ MockMediaSourcePrivate::MockMediaSourcePrivate(MockMediaPlayerMediaSource* paren
 
 MockMediaSourcePrivate::~MockMediaSourcePrivate()
 {
+    for (auto it = m_sourceBuffers.begin(), end = m_sourceBuffers.end(); it != end; ++it)
+        (*it)->clearMediaSource();
 }
 
 MediaSourcePrivate::AddStatus MockMediaSourcePrivate::addSourceBuffer(const ContentType& contentType, RefPtr<SourceBufferPrivate>& outPrivate)
index eeb1de6..87670da 100644 (file)
@@ -100,7 +100,7 @@ RefPtr<MockSourceBufferPrivate> MockSourceBufferPrivate::create(MockMediaSourceP
 }
 
 MockSourceBufferPrivate::MockSourceBufferPrivate(MockMediaSourcePrivate* parent)
-    : m_parent(parent)
+    : m_mediaSource(parent)
     , m_client(0)
 {
 }
@@ -187,17 +187,19 @@ void MockSourceBufferPrivate::abort()
 
 void MockSourceBufferPrivate::removedFromMediaSource()
 {
-    m_parent->removeSourceBuffer(this);
+    if (m_mediaSource)
+        m_mediaSource->removeSourceBuffer(this);
 }
 
 MediaPlayer::ReadyState MockSourceBufferPrivate::readyState() const
 {
-    return m_parent->player()->readyState();
+    return m_mediaSource ? m_mediaSource->player()->readyState() : MediaPlayer::HaveNothing;
 }
 
 void MockSourceBufferPrivate::setReadyState(MediaPlayer::ReadyState readyState)
 {
-    m_parent->player()->setReadyState(readyState);
+    if (m_mediaSource)
+        m_mediaSource->player()->setReadyState(readyState);
 }
 
 void MockSourceBufferPrivate::evictCodedFrames()
@@ -212,7 +214,8 @@ bool MockSourceBufferPrivate::isFull()
 
 void MockSourceBufferPrivate::setActive(bool isActive)
 {
-    m_parent->sourceBufferPrivateDidChangeActiveState(this, isActive);
+    if (m_mediaSource)
+        m_mediaSource->sourceBufferPrivateDidChangeActiveState(this, isActive);
 }
 
 bool MockSourceBufferPrivate::hasVideo() const
index 9b8b857..c540972 100644 (file)
@@ -50,6 +50,8 @@ public:
     static RefPtr<MockSourceBufferPrivate> create(MockMediaSourcePrivate*);
     virtual ~MockSourceBufferPrivate();
 
+    void clearMediaSource() { m_mediaSource = nullptr; }
+
     bool hasVideo() const;
     bool hasAudio() const;
 
@@ -77,7 +79,7 @@ private:
     void didReceiveInitializationSegment(const MockInitializationBox&);
     void didReceiveSample(const MockSampleBox&);
 
-    MockMediaSourcePrivate* m_parent;
+    MockMediaSourcePrivate* m_mediaSource;
     SourceBufferPrivateClient* m_client;
 
     Vector<char> m_inputBuffer;