Bug 156905 – Drop [UsePointersEvenForNonNullableObjectArguments] from MediaStream interfaces

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Drop [UsePointersEvenForNonNullableObjectArguments] from MediaStream interfaces

[youenn fablet]

Created attachment 277047 [details]
Patch

[Build Bot]

Comment on attachment 277047 [details]
Patch

Attachment 277047 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1201762

New failing tests:
http/tests/media/media-source/mediasource-append-buffer.html

[Build Bot]

Created attachment 277052 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277047 [details]
Patch

Attachment 277047 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1201769

New failing tests:
http/tests/media/media-source/mediasource-append-buffer.html

[Build Bot]

Created attachment 277053 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 277055 [details]
Removing not related test

[Chris Dumez]

Comment on attachment 277055 [details]
Removing not related test

View in context: https://bugs.webkit.org/attachment.cgi?id=277055&action=review

> Source/WebCore/Modules/mediastream/MediaStream.cpp:129
> +void MediaStream::addTrack(MediaStreamTrack& track)

Should probably be a Ref<MediaStreamTrack>&& since we pass ownership.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:209
> +bool MediaStream::internalAddTrack(MediaStreamTrack& track, StreamModifier streamModifier)

Should probably be a Ref<MediaStreamTrack>&&

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:104
> +RefPtr<RTCRtpSender> RTCPeerConnection::addTrack(MediaStreamTrack& track, Vector<MediaStream*> streams, ExceptionCode& ec)

Should probably be a Ref<>&&

> Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:42
> +    static Ref<RTCRtpReceiver> create(MediaStreamTrack& track)

Should probably be a Ref<>&&

> Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:48
> +    explicit RTCRtpReceiver(MediaStreamTrack&);

Ditto

> Source/WebCore/Modules/mediastream/RTCRtpSender.h:51
> +    static Ref<RTCRtpSender> create(MediaStreamTrack& track, Vector<String>&& mediaStreamIds, RTCRtpSenderClient& client)

Should probably be a Ref<>&&

> Source/WebCore/Modules/mediastream/RTCRtpSender.h:64
> +    RTCRtpSender(MediaStreamTrack&, Vector<String>&& mediaStreamIds, RTCRtpSenderClient&);

ditto.

> Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:49
> +    MediaStreamTrack& track() {  return m_track.get(); }

No need for the .get()

> Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:-52
> -    RTCRtpSenderReceiverBase(RefPtr<MediaStreamTrack>&& track)

Ref<>&&

> LayoutTests/fast/mediastream/MediaStream-add-remove-tracks.html:105
> +                tryThrowing(function() { stream1.addTrack(null); });

Can we use the pre-existing shouldThrow() rather than writing our own?

> LayoutTests/fast/mediastream/RTCPeerConnection-add-removeTrack.html:8
> +            function tryThrowing(callback) {

ditto.

> LayoutTests/fast/mediastream/RTCPeerConnection-datachannel.html:10
> +            function tryThrowing(callback) {

ditto.

[youenn fablet]

Created attachment 277242 [details]
Patch

[youenn fablet]

Thanks for the review, I updated the patch accordingly.

[youenn fablet]

Created attachment 277243 [details]
Patch

[Chris Dumez]

Comment on attachment 277243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277243&action=review

> Source/WebCore/Modules/mediastream/MediaStream.cpp:199
> +        internalAddTrack(MediaStreamTrack::create(*context, trackPrivate).get(), StreamModifier::Platform);

I don't believe you need the .get() here.

> Source/WebCore/Modules/mediastream/MediaStream.h:135
> +    // FIXME: m_trackSet should take Ref<MediaStreamTrack>.

We don't support this for HashMaps..

> Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:38
> +RTCRtpReceiver::RTCRtpReceiver(MediaStreamTrack& track)

Should be a Ref<>&&

> Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:42
> +    static Ref<RTCRtpReceiver> create(MediaStreamTrack& track)

Should be a Ref<>&&

> Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:48
> +    explicit RTCRtpReceiver(MediaStreamTrack&);

should be a Ref<>&&

> LayoutTests/fast/mediastream/MediaStream-add-remove-tracks-expected.txt:60
> +FAIL stream2.active should be true. Was false.

New FAIL?

> LayoutTests/fast/mediastream/MediaStream-add-remove-tracks-expected.txt:63
> +FAIL stream2.active should be true. Was false.

New FAIL?

[youenn fablet]

Created attachment 277361 [details]
Patch

[youenn fablet]

Created attachment 277362 [details]
Patch

[youenn fablet]

Thanks for the review, I updated accordingly.
I also made addTrack take a ref and not a Ref<>, following on bug 156901 discussion.

> > Source/WebCore/Modules/mediastream/MediaStream.cpp:199
> > +        internalAddTrack(MediaStreamTrack::create(*context, trackPrivate).get(), StreamModifier::Platform);
> 
> I don't believe you need the .get() here.

Fixed.

> > Source/WebCore/Modules/mediastream/MediaStream.h:135
> > +    // FIXME: m_trackSet should take Ref<MediaStreamTrack>.
> 
> We don't support this for HashMaps..

Do you know the reason?

> > Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:38
> > +RTCRtpReceiver::RTCRtpReceiver(MediaStreamTrack& track)
> 
> Should be a Ref<>&&

Fixed.

> > Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:42
> > +    static Ref<RTCRtpReceiver> create(MediaStreamTrack& track)
> 
> Should be a Ref<>&&

Fixed.

> > Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:48
> > +    explicit RTCRtpReceiver(MediaStreamTrack&);
> 
> should be a Ref<>&&

Fixed.

> > LayoutTests/fast/mediastream/MediaStream-add-remove-tracks-expected.txt:60
> > +FAIL stream2.active should be true. Was false.
> 
> New FAIL?

This is unrelated to this patch changes.
This test is skipped so it does not get rebased.
In the new patch, the testing of null/undefined gets its own test file.

> > LayoutTests/fast/mediastream/MediaStream-add-remove-tracks-expected.txt:63
> > +FAIL stream2.active should be true. Was false.
> 
> New FAIL?

Ditto.

[Darin Adler]

Comment on attachment 277362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277362&action=review

Change is generally quite good, but there is a possible lifetime bug introduced in MediaStream::internalRemoveTrack that we should not land.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:206
> -    RefPtr<MediaStreamTrack> track = getTrackById(trackPrivate.id());
> +    MediaStreamTrack* track = getTrackById(trackPrivate.id());
>      ASSERT(track);
> -    internalRemoveTrack(WTFMove(track), StreamModifier::Platform);
> +    internalRemoveTrack(*track, StreamModifier::Platform);

This is an inefficient algorithm; looks up the track by ID once here, then calls internalRemoveTrack which does the same thing again. Given how it works, the internalRemoveTrack function should take a track ID as its argument rather than taking a track.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:216
> +    MediaStreamTrack& track = *result.iterator->value;

I would have used auto& here.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:232
> +    track.removeObserver(this);

I believe that if the track is not in a Ref or RefPtr, it could have been deleted by the call to remove above; that might have been the last reference to the track. So this change may introduce a bug. I think we do need a Ref inside this function even though the argument doesn’t need to be a Ref. The cleanest way to do this would be to use HashMap::take rather than HashMap::remove in the implementation of this function. That would be a natural thing to do if we changed it to take a track ID as its argument rather than taking a track.

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:224
> +void RTCDataChannel::send(ArrayBufferView* data, ExceptionCode& ec)

Why does this function take a pointer instead of a reference?

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:229
>      RefPtr<ArrayBuffer> arrayBuffer(data->buffer());
> -    send(arrayBuffer.release(), ec);
> +    ASSERT(arrayBuffer);
> +    send(*arrayBuffer, ec);

There is no reason for this local variable. Here’s what I would write:

    ASSERT(data.buffer());
    send(*data.buffer(), ec);

But also, not sure why that assertion is safe. If that function can ever return nullptr, then it’s an incorrect assertion. If it can’t return nullptr, then it should return a & or Ref, not RefPtr.

> Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:49
> +    MediaStreamTrack& track() {  return m_track; }

Should remove the extra space here after the "{".

[Chris Dumez]

Comment on attachment 277362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277362&action=review

> Source/WebCore/Modules/mediastream/MediaStream.cpp:129
> +void MediaStream::addTrack(MediaStreamTrack& track)

I still don't understand why we wouldn't use Ref<>&& here. The caller is clearly passing ownership and internalAddTrack() already takes a Ref<>&&. We anyway move the Ref<> around so there is no ref-counting churn as far as I can tell.

[Darin Adler]

Comment on attachment 277362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277362&action=review

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:129
>> +void MediaStream::addTrack(MediaStreamTrack& track)
> 
> I still don't understand why we wouldn't use Ref<>&& here. The caller is clearly passing ownership and internalAddTrack() already takes a Ref<>&&. We anyway move the Ref<> around so there is no ref-counting churn as far as I can tell.

Lets discuss this in person at some point. Really either way is OK with me but I see two issues:

1) There’s not a good reason to put the reference counting at the call site of addTrack inside the bindings rather than inside the function. And that’s what using Ref&& does.

2) I think we generally should prefer standard references to rvalue references unless the move semantic is an important optimization. I don’t think we should always do it any time we pass a reference counted object even if the semantic is passing ownership. The way you’d see this in the past is massive over-use of PassRefPtr.

But I suppose that point (2) is debatable.

[youenn fablet]

Thanks for the review.
Please see inline for more comments.

> > Source/WebCore/Modules/mediastream/MediaStream.cpp:206
> > -    RefPtr<MediaStreamTrack> track = getTrackById(trackPrivate.id());
> > +    MediaStreamTrack* track = getTrackById(trackPrivate.id());
> >      ASSERT(track);
> > -    internalRemoveTrack(WTFMove(track), StreamModifier::Platform);
> > +    internalRemoveTrack(*track, StreamModifier::Platform);
> 
> This is an inefficient algorithm; looks up the track by ID once here, then
> calls internalRemoveTrack which does the same thing again. Given how it
> works, the internalRemoveTrack function should take a track ID as its
> argument rather than taking a track.

OK.

> > Source/WebCore/Modules/mediastream/MediaStream.cpp:216
> > +    MediaStreamTrack& track = *result.iterator->value;
> 
> I would have used auto& here.

OK.

> > Source/WebCore/Modules/mediastream/MediaStream.cpp:232
> > +    track.removeObserver(this);
> 
> I believe that if the track is not in a Ref or RefPtr, it could have been
> deleted by the call to remove above; that might have been the last reference
> to the track. So this change may introduce a bug. I think we do need a Ref
> inside this function even though the argument doesn’t need to be a Ref. The
> cleanest way to do this would be to use HashMap::take rather than
> HashMap::remove in the implementation of this function. That would be a
> natural thing to do if we changed it to take a track ID as its argument
> rather than taking a track.

Right, take sounds good.

> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:224
> > +void RTCDataChannel::send(ArrayBufferView* data, ExceptionCode& ec)
> 
> Why does this function take a pointer instead of a reference?

Binding generator limitation.

> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:229
> >      RefPtr<ArrayBuffer> arrayBuffer(data->buffer());
> > -    send(arrayBuffer.release(), ec);
> > +    ASSERT(arrayBuffer);
> > +    send(*arrayBuffer, ec);
> 
> There is no reason for this local variable. Here’s what I would write:
> 
>     ASSERT(data.buffer());
>     send(*data.buffer(), ec);

ArrayBufferView::buffer() is not const.
It may for instance x that is why I did not want to call it twice.


> But also, not sure why that assertion is safe. If that function can ever
> return nullptr, then it’s an incorrect assertion. If it can’t return
> nullptr, then it should return a & or Ref, not RefPtr.

There is a theoretical case where it can returns null (ArrayBufferView

> 
> > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:49
> > +    MediaStreamTrack& track() {  return m_track; }
> 
> Should remove the extra space here after the "{".

OK

[youenn fablet]

> >     ASSERT(data.buffer());
> >     send(*data.buffer(), ec);
> 
> ArrayBufferView::buffer() is not const.
> It may for instance x that is why I did not want to call it twice.

It may call for instance slowDownAndWasteMemory.
It probably does not have any side-effect calling it twice in debug but I thought it would be better to call it just once.

> > But also, not sure why that assertion is safe. If that function can ever
> > return nullptr, then it’s an incorrect assertion. If it can’t return
> > nullptr, then it should return a & or Ref, not RefPtr.
> 
> There is a theoretical case where it can returns null (ArrayBufferView

... in case ArrayBufferView is a JSDataView.
That said, I am not familiar with these classes.

[youenn fablet]

Created attachment 277465 [details]
Patch

[youenn fablet]

(In reply to comment #20)
> Created attachment 277465 [details]
> Patch

I kept ASSERT() the same as the previous patch.

[Darin Adler]

Comment on attachment 277465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277465&action=review

> Source/WebCore/Modules/mediastream/MediaStream.cpp:98
> +        Ref<MediaStreamTrack> track = MediaStreamTrack::create(context, *trackPrivate);

I would have used auto here rather than repeat the class name twice.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:227
> +    RefPtr<MediaStreamTrack> track = m_trackSet.take(trackId);

I would have used auto here rather than naming the type explicitly.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:236
> -        dispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, WTFMove(track)));
> +        dispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, track));

Why would we make this change? Still seems like an optimization to use WTFMove here.

[youenn fablet]

Created attachment 277674 [details]
Patch for landing

[youenn fablet]

(In reply to comment #22)
> Comment on attachment 277465 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277465&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:98
> > +        Ref<MediaStreamTrack> track = MediaStreamTrack::create(context, *trackPrivate);
> 
> I would have used auto here rather than repeat the class name twice.

I changed it. Fact is we loose that this is a Ref<> and not a RefPtr<>.

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:227
> > +    RefPtr<MediaStreamTrack> track = m_trackSet.take(trackId);
> 
> I would have used auto here rather than naming the type explicitly.

OK

> > Source/WebCore/Modules/mediastream/MediaStream.cpp:236
> > -        dispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, WTFMove(track)));
> > +        dispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, track));
> 
> Why would we make this change? Still seems like an optimization to use
> WTFMove here.

Right, fixed in landing patch.

[WebKit Commit Bot]

Comment on attachment 277674 [details]
Patch for landing

Clearing flags on attachment: 277674

Committed r200231: <http://trac.webkit.org/changeset/200231>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.