Changeset 116311 in webkit


Ignore:
Timestamp:
May 7, 2012 8:02:35 AM (12 years ago)
Author:
adam.bergkvist@ericsson.com
Message:

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:
Location:
trunk/Source/WebCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r116308 r116311  
     12012-05-07  Adam Bergkvist  <adam.bergkvist@ericsson.com>
     2
     3        MediaStream should not be an ActiveDOMObject
     4        https://bugs.webkit.org/show_bug.cgi?id=85191
     5
     6        Reviewed by Adam Barth.
     7
     8        The model with MediaStreamDescriptor and MediaStream (and LocalMediaStream)
     9        allows the JavaScript objects (MediaStream and LocalMediaStream) to be
     10        cleaned up while the MediaStreamDescriptor lives on to manage the stream in
     11        the platform. This happens for example when a URL is created to represent
     12        a MediaStream (using createObjectURL()). In that case, the MediaStreamDescriptor
     13        is put into the MediaStreamRegistry and even though the MediaStream object is
     14        lost, the URL still works since the descriptor is kept in the registry.
     15
     16        The changes introduced in r113460 (http://webkit.org/b/83143) turned
     17        MediaStream and LocalMediaStream into ActiveDOMObjects. For example on page
     18        reload, LocalMediaStream calls MediaStreamCenter::didStopLocalMediaStream()
     19        via its ActiveDOMObject::stop() method. However, when a page reload occurs,
     20        the LocalMediaStream object may have been cleaned up already and
     21        MediaStreamCenter::didStopLocalMediaStream() will not be called.
     22
     23        One way to make the behavior consistent would be to call
     24        MediaStreamCenter::didStopLocalMediaStream() when the descriptor is cleaned up,
     25        cause then we wouldn't be dependent on the LocalMediaStream object being alive.
     26        However, calling MediaStreamCenter::didStopLocalMediaStream() might not be the
     27        correct thing to do when all references to the descriptor are lost since there
     28        can be MediaStream objects constructed from the tracks of the LocalMediaStream
     29        that should continue to work. MediaStreamCenter::didStopLocalMediaStream() was
     30        intended for LocalMediaStream.stop() which is used to revoke access to devices;
     31        that should not necessarily happen when the descriptor of a LocalMediaStream is
     32        cleaned up. If it's necessary for some ports to signal to the platform that a
     33        MediaStreamDescriptor is cleaned up, then I would suggest adding a new function,
     34        willDestroyMediaStreamDescriptor(), to the MediaStreamCenter interface.
     35
     36        The current resolution is to make MediaStream a ContextDestructionObserver
     37        instead of an ActiveDOMObject.
     38
     39        Currently not testable.
     40
     41        * Modules/mediastream/LocalMediaStream.cpp:
     42        (WebCore::LocalMediaStream::create):
     43        * Modules/mediastream/LocalMediaStream.h:
     44        (LocalMediaStream):
     45        * Modules/mediastream/LocalMediaStream.idl:
     46        * Modules/mediastream/MediaStream.cpp:
     47        (WebCore::MediaStream::create):
     48        (WebCore::MediaStream::MediaStream):
     49        (WebCore::MediaStream::scriptExecutionContext):
     50        * Modules/mediastream/MediaStream.h:
     51
    1522012-05-07  Liam Quinn  <lquinn@rim.com>
    253

  • trunk/Source/WebCore/Modules/mediastream/LocalMediaStream.cpp

    r115649 r116311  
    3636PassRefPtr<LocalMediaStream> LocalMediaStream::create(ScriptExecutionContext* context, const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources)
    3737{
    38     RefPtr<LocalMediaStream> stream = adoptRef(new LocalMediaStream(context, audioSources, videoSources));
    39     stream->suspendIfNeeded();
    40     return stream.release();
     38    return adoptRef(new LocalMediaStream(context, audioSources, videoSources));
    4139}
    4240
    … …  
    4442    : MediaStream(context, MediaStreamDescriptor::create(createCanonicalUUIDString(), audioSources, videoSources))
    4543{
    46 }
    47 
    48 void LocalMediaStream::stopFunction()
    49 {
    50     stop();
    5144}
    5245

  • trunk/Source/WebCore/Modules/mediastream/LocalMediaStream.h

    r115649 r116311  
    3838    virtual ~LocalMediaStream();
    3939
    40     void stopFunction();
    41 
    42     // ActiveDOMObject
    43     virtual void stop() OVERRIDE;
     40    void stop();
    4441
    4542    // EventTarget

  • trunk/Source/WebCore/Modules/mediastream/LocalMediaStream.idl

    r113924 r116311  
    3030        JSGenerateToJSObject
    3131    ] LocalMediaStream : MediaStream {
    32         [ImplementedAs=stopFunction] void stop();
     32        void stop();
    3333    };
    3434

  • trunk/Source/WebCore/Modules/mediastream/MediaStream.cpp

    r113460 r116311  
    3333#include "MediaStreamCenter.h"
    3434#include "MediaStreamSource.h"
    35 #include "ScriptExecutionContext.h"
    3635#include "UUID.h"
    3736
    … …  
    7877    MediaStreamCenter::instance().didConstructMediaStream(descriptor.get());
    7978
    80     RefPtr<MediaStream> stream = adoptRef(new MediaStream(context, descriptor.release()));
    81     stream->suspendIfNeeded();
    82     return stream.release();
     79    return adoptRef(new MediaStream(context, descriptor.release()));
    8380}
    8481
    8582PassRefPtr<MediaStream> MediaStream::create(ScriptExecutionContext* context, PassRefPtr<MediaStreamDescriptor> streamDescriptor)
    8683{
    87     RefPtr<MediaStream> stream = adoptRef(new MediaStream(context, streamDescriptor));
    88     stream->suspendIfNeeded();
    89     return stream.release();
     84    return adoptRef(new MediaStream(context, streamDescriptor));
    9085}
    9186
    9287MediaStream::MediaStream(ScriptExecutionContext* context, PassRefPtr<MediaStreamDescriptor> streamDescriptor)
    93     : ActiveDOMObject(context, this)
     88    : ContextDestructionObserver(context)
    9489    , m_descriptor(streamDescriptor)
    9590{
    … …  
    138133ScriptExecutionContext* MediaStream::scriptExecutionContext() const
    139134{
    140     return ActiveDOMObject::scriptExecutionContext();
     135    return ContextDestructionObserver::scriptExecutionContext();
    141136}
    142137

  • trunk/Source/WebCore/Modules/mediastream/MediaStream.h

    r113460 r116311  
    2929#if ENABLE(MEDIA_STREAM)
    3030
    31 #include "ActiveDOMObject.h"
     31#include "ContextDestructionObserver.h"
    3232#include "EventTarget.h"
    3333#include "MediaStreamDescriptor.h"
    … …  
    3838namespace WebCore {
    3939
    40 class ScriptExecutionContext;
    41 
    42 class MediaStream : public RefCounted<MediaStream>, public MediaStreamDescriptorOwner, public EventTarget, public ActiveDOMObject {
     40class MediaStream : public RefCounted<MediaStream>, public MediaStreamDescriptorOwner, public EventTarget, public ContextDestructionObserver {
    4341public:
    4442    // Must match the constants in the .idl file.
Note: See TracChangeset for help on using the changeset viewer.