MediaStream should not be an ActiveDOMObject
authoradam.bergkvist@ericsson.com <adam.bergkvist@ericsson.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 May 2012 15:02:35 +0000 (15:02 +0000)
committeradam.bergkvist@ericsson.com <adam.bergkvist@ericsson.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 May 2012 15:02:35 +0000 (15:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=85191

Reviewed by Adam Barth.

The model with MediaStreamDescriptor and MediaStream (and LocalMediaStream)
allows the JavaScript objects (MediaStream and LocalMediaStream) to be
cleaned up while the MediaStreamDescriptor lives on to manage the stream in
the platform. This happens for example when a URL is created to represent
a MediaStream (using createObjectURL()). In that case, the MediaStreamDescriptor
is put into the MediaStreamRegistry and even though the MediaStream object is
lost, the URL still works since the descriptor is kept in the registry.

The changes introduced in r113460 (http://webkit.org/b/83143) turned
MediaStream and LocalMediaStream into ActiveDOMObjects. For example on page
reload, LocalMediaStream calls MediaStreamCenter::didStopLocalMediaStream()
via its ActiveDOMObject::stop() method. However, when a page reload occurs,
the LocalMediaStream object may have been cleaned up already and
MediaStreamCenter::didStopLocalMediaStream() will not be called.

One way to make the behavior consistent would be to call
MediaStreamCenter::didStopLocalMediaStream() when the descriptor is cleaned up,
cause then we wouldn't be dependent on the LocalMediaStream object being alive.
However, calling MediaStreamCenter::didStopLocalMediaStream() might not be the
correct thing to do when all references to the descriptor are lost since there
can be MediaStream objects constructed from the tracks of the LocalMediaStream
that should continue to work. MediaStreamCenter::didStopLocalMediaStream() was
intended for LocalMediaStream.stop() which is used to revoke access to devices;
that should not necessarily happen when the descriptor of a LocalMediaStream is
cleaned up. If it's necessary for some ports to signal to the platform that a
MediaStreamDescriptor is cleaned up, then I would suggest adding a new function,
willDestroyMediaStreamDescriptor(), to the MediaStreamCenter interface.

The current resolution is to make MediaStream a ContextDestructionObserver
instead of an ActiveDOMObject.

Currently not testable.

* Modules/mediastream/LocalMediaStream.cpp:
(WebCore::LocalMediaStream::create):
* Modules/mediastream/LocalMediaStream.h:
(LocalMediaStream):
* Modules/mediastream/LocalMediaStream.idl:
* Modules/mediastream/MediaStream.cpp:
(WebCore::MediaStream::create):
(WebCore::MediaStream::MediaStream):
(WebCore::MediaStream::scriptExecutionContext):
* Modules/mediastream/MediaStream.h:

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

Source/WebCore/ChangeLog
Source/WebCore/Modules/mediastream/LocalMediaStream.cpp
Source/WebCore/Modules/mediastream/LocalMediaStream.h
Source/WebCore/Modules/mediastream/LocalMediaStream.idl
Source/WebCore/Modules/mediastream/MediaStream.cpp
Source/WebCore/Modules/mediastream/MediaStream.h

index 9cd4a03..b85d049 100644 (file)
@@ -1,3 +1,54 @@
+2012-05-07  Adam Bergkvist  <adam.bergkvist@ericsson.com>
+
+        MediaStream should not be an ActiveDOMObject
+        https://bugs.webkit.org/show_bug.cgi?id=85191
+
+        Reviewed by Adam Barth.
+
+        The model with MediaStreamDescriptor and MediaStream (and LocalMediaStream)
+        allows the JavaScript objects (MediaStream and LocalMediaStream) to be
+        cleaned up while the MediaStreamDescriptor lives on to manage the stream in
+        the platform. This happens for example when a URL is created to represent
+        a MediaStream (using createObjectURL()). In that case, the MediaStreamDescriptor
+        is put into the MediaStreamRegistry and even though the MediaStream object is
+        lost, the URL still works since the descriptor is kept in the registry.
+
+        The changes introduced in r113460 (http://webkit.org/b/83143) turned
+        MediaStream and LocalMediaStream into ActiveDOMObjects. For example on page
+        reload, LocalMediaStream calls MediaStreamCenter::didStopLocalMediaStream()
+        via its ActiveDOMObject::stop() method. However, when a page reload occurs,
+        the LocalMediaStream object may have been cleaned up already and
+        MediaStreamCenter::didStopLocalMediaStream() will not be called.
+
+        One way to make the behavior consistent would be to call
+        MediaStreamCenter::didStopLocalMediaStream() when the descriptor is cleaned up,
+        cause then we wouldn't be dependent on the LocalMediaStream object being alive.
+        However, calling MediaStreamCenter::didStopLocalMediaStream() might not be the
+        correct thing to do when all references to the descriptor are lost since there
+        can be MediaStream objects constructed from the tracks of the LocalMediaStream
+        that should continue to work. MediaStreamCenter::didStopLocalMediaStream() was
+        intended for LocalMediaStream.stop() which is used to revoke access to devices;
+        that should not necessarily happen when the descriptor of a LocalMediaStream is
+        cleaned up. If it's necessary for some ports to signal to the platform that a
+        MediaStreamDescriptor is cleaned up, then I would suggest adding a new function,
+        willDestroyMediaStreamDescriptor(), to the MediaStreamCenter interface.
+
+        The current resolution is to make MediaStream a ContextDestructionObserver
+        instead of an ActiveDOMObject.
+
+        Currently not testable.
+
+        * Modules/mediastream/LocalMediaStream.cpp:
+        (WebCore::LocalMediaStream::create):
+        * Modules/mediastream/LocalMediaStream.h:
+        (LocalMediaStream):
+        * Modules/mediastream/LocalMediaStream.idl:
+        * Modules/mediastream/MediaStream.cpp:
+        (WebCore::MediaStream::create):
+        (WebCore::MediaStream::MediaStream):
+        (WebCore::MediaStream::scriptExecutionContext):
+        * Modules/mediastream/MediaStream.h:
+
 2012-05-07  Liam Quinn  <lquinn@rim.com>
 
         [BlackBerry] WWW-Authenticate header on 200 response pops up authentication dialog
index 7626519..ccb0dd0 100644 (file)
@@ -35,9 +35,7 @@ namespace WebCore {
 
 PassRefPtr<LocalMediaStream> LocalMediaStream::create(ScriptExecutionContext* context, const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources)
 {
-    RefPtr<LocalMediaStream> stream = adoptRef(new LocalMediaStream(context, audioSources, videoSources));
-    stream->suspendIfNeeded();
-    return stream.release();
+    return adoptRef(new LocalMediaStream(context, audioSources, videoSources));
 }
 
 LocalMediaStream::LocalMediaStream(ScriptExecutionContext* context, const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources)
@@ -45,11 +43,6 @@ LocalMediaStream::LocalMediaStream(ScriptExecutionContext* context, const MediaS
 {
 }
 
-void LocalMediaStream::stopFunction()
-{
-    stop();
-}
-
 void LocalMediaStream::stop()
 {
     if (readyState() == ENDED)
index 7770320..386ab1b 100644 (file)
@@ -37,10 +37,7 @@ public:
     static PassRefPtr<LocalMediaStream> create(ScriptExecutionContext*, const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources);
     virtual ~LocalMediaStream();
 
-    void stopFunction();
-
-    // ActiveDOMObject
-    virtual void stop() OVERRIDE;
+    void stop();
 
     // EventTarget
     virtual const AtomicString& interfaceName() const OVERRIDE;
index 7aaddc3..e9944cf 100644 (file)
@@ -29,7 +29,7 @@ module core {
         JSGenerateToNativeObject,
         JSGenerateToJSObject
     ] LocalMediaStream : MediaStream {
-        [ImplementedAs=stopFunction] void stop();
+        void stop();
     };
 
 }
index 1227a7e..79e3f4f 100644 (file)
@@ -32,7 +32,6 @@
 #include "ExceptionCode.h"
 #include "MediaStreamCenter.h"
 #include "MediaStreamSource.h"
-#include "ScriptExecutionContext.h"
 #include "UUID.h"
 
 namespace WebCore {
@@ -77,20 +76,16 @@ PassRefPtr<MediaStream> MediaStream::create(ScriptExecutionContext* context, Pas
     RefPtr<MediaStreamDescriptor> descriptor = MediaStreamDescriptor::create(createCanonicalUUIDString(), audioSources, videoSources);
     MediaStreamCenter::instance().didConstructMediaStream(descriptor.get());
 
-    RefPtr<MediaStream> stream = adoptRef(new MediaStream(context, descriptor.release()));
-    stream->suspendIfNeeded();
-    return stream.release();
+    return adoptRef(new MediaStream(context, descriptor.release()));
 }
 
 PassRefPtr<MediaStream> MediaStream::create(ScriptExecutionContext* context, PassRefPtr<MediaStreamDescriptor> streamDescriptor)
 {
-    RefPtr<MediaStream> stream = adoptRef(new MediaStream(context, streamDescriptor));
-    stream->suspendIfNeeded();
-    return stream.release();
+    return adoptRef(new MediaStream(context, streamDescriptor));
 }
 
 MediaStream::MediaStream(ScriptExecutionContext* context, PassRefPtr<MediaStreamDescriptor> streamDescriptor)
-    : ActiveDOMObject(context, this)
+    : ContextDestructionObserver(context)
     , m_descriptor(streamDescriptor)
 {
     m_descriptor->setOwner(this);
@@ -137,7 +132,7 @@ const AtomicString& MediaStream::interfaceName() const
 
 ScriptExecutionContext* MediaStream::scriptExecutionContext() const
 {
-    return ActiveDOMObject::scriptExecutionContext();
+    return ContextDestructionObserver::scriptExecutionContext();
 }
 
 EventTargetData* MediaStream::eventTargetData()
index b493286..f6a98de 100644 (file)
@@ -28,7 +28,7 @@
 
 #if ENABLE(MEDIA_STREAM)
 
-#include "ActiveDOMObject.h"
+#include "ContextDestructionObserver.h"
 #include "EventTarget.h"
 #include "MediaStreamDescriptor.h"
 #include "MediaStreamTrackList.h"
@@ -37,9 +37,7 @@
 
 namespace WebCore {
 
-class ScriptExecutionContext;
-
-class MediaStream : public RefCounted<MediaStream>, public MediaStreamDescriptorOwner, public EventTarget, public ActiveDOMObject {
+class MediaStream : public RefCounted<MediaStream>, public MediaStreamDescriptorOwner, public EventTarget, public ContextDestructionObserver {
 public:
     // Must match the constants in the .idl file.
     enum ReadyState {