Bug 156904 – Drop [UsePointersEvenForNonNullableObjectArguments] from MediaSource interfaces

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

[youenn fablet]

Drop [UsePointersEvenForNonNullableObjectArguments] from MediaSource interfaces

[youenn fablet]

Created attachment 277035 [details]
Patch

[Chris Dumez]

Comment on attachment 277035 [details]
Patch

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

> Source/WebCore/Modules/mediasource/MediaSource.cpp:522
> +    // FIXME: In case type is null, we should probably throw a TypeError.

Yes, this is what the spec says:
https://w3c.github.io/media-source/#widl-MediaSource-addSourceBuffer-SourceBuffer-DOMString-type

Could you please make the fix in this patch? Changing the exception type should be safe compatibility-size.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:526
>      if (type.isNull() || type.isEmpty()) {

if (type.isEmpty()) would suffice
Then
ec = TypeError;

> Source/WebCore/Modules/mediasource/MediaSource.cpp:576
> +    return buffer.ptr();

This function should return a RefPtr<> and here we would just return WTFMove(buffer);

> Source/WebCore/Modules/mediasource/SourceBuffer.h:90
> +    void appendBuffer(ArrayBufferView*, ExceptionCode&);

We really need to fix the bindings generator.

> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:52
> +void SourceBufferList::add(SourceBuffer& buffer)

Should be a Ref<>&&

> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:54
> +    m_list.append(&buffer);

WTFMove(buffer)

Also, could you look to see if making m_list a Vector<Ref<SourceBuffer>> could be done in this patch without too much effort? It would be much nicer.

[Darin Adler]

Comment on attachment 277035 [details]
Patch

I’m going to set review- because I’d like to see another patch with the fixes Chris suggested.

[youenn fablet]

Created attachment 277240 [details]
Patch

[youenn fablet]

(In reply to comment #2)
> Comment on attachment 277035 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277035&action=review
> 
> > Source/WebCore/Modules/mediasource/MediaSource.cpp:522
> > +    // FIXME: In case type is null, we should probably throw a TypeError.
> 
> Yes, this is what the spec says:
> https://w3c.github.io/media-source/#widl-MediaSource-addSourceBuffer-
> SourceBuffer-DOMString-type
> 
> Could you please make the fix in this patch? Changing the exception type
> should be safe compatibility-size.

The issue is that binding generator should handle this case (null is not a valid value for DOMString).
I fixed that by making type parameter nullable.

> > Source/WebCore/Modules/mediasource/MediaSource.cpp:576
> > +    return buffer.ptr();
> 
> This function should return a RefPtr<> and here we would just return
> WTFMove(buffer);

buffer is now moved before returning.
I kept returning the raw pointer.

> > Source/WebCore/Modules/mediasource/SourceBuffer.h:90
> > +    void appendBuffer(ArrayBufferView*, ExceptionCode&);
> 
> We really need to fix the bindings generator.

Agreed.

> > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:52
> > +void SourceBufferList::add(SourceBuffer& buffer)
> 
> Should be a Ref<>&&

FIXED.

> > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:54
> > +    m_list.append(&buffer);
> 
> WTFMove(buffer)
> 
> Also, could you look to see if making m_list a Vector<Ref<SourceBuffer>>
> could be done in this patch without too much effort? It would be much nicer.

Done.
There are some HashMap<..., RefPtr<>> and Vector<RefPtr<>> that might also be changed to using Ref<> in mediasource/mediastream if possible.

[Build Bot]

Comment on attachment 277240 [details]
Patch

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

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

[Build Bot]

Created attachment 277244 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 277240 [details]
Patch

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

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

[Build Bot]

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

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

[Build Bot]

Comment on attachment 277240 [details]
Patch

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

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

[Build Bot]

Created attachment 277246 [details]
Archive of layout-test-results from ews115 for mac-yosemite

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

[youenn fablet]

Created attachment 277248 [details]
Rebasing some tests

[Chris Dumez]

Comment on attachment 277248 [details]
Rebasing some tests

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

> Source/WTF/wtf/Ref.h:189
> +template<typename T, typename U> inline bool operator==(const Ref<T>& a, const Ref<U>& b)

Antti and Andreas do not like this one. They think if there were an operator== it should forward to the underlying type's operator== since this is a reference, not a pointer type.

[Chris Dumez]

Comment on attachment 277248 [details]
Rebasing some tests

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

> Source/WebCore/Modules/mediasource/MediaSource.cpp:575
> +    return result;

This method should be updated to return a RefPtr<> since we transfer ownership to the caller.

[youenn fablet]

(In reply to comment #14)
> Comment on attachment 277248 [details]
> Rebasing some tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277248&action=review
> 
> > Source/WebCore/Modules/mediasource/MediaSource.cpp:575
> > +    return result;
> 
> This method should be updated to return a RefPtr<> since we transfer
> ownership to the caller.

Can you explain the issue?
A "result" ref is kept by m_sourceBuffers.
I wonder whether returning a RefPtr<> will not create unneeded refcounting at the JS binding code?

> Antti and Andreas do not like this one. They think if there were an
> operator== it should forward to the underlying type's operator== since this
> is a reference, not a pointer type.

Good point, I'll check this.

[Chris Dumez]

Comment on attachment 277248 [details]
Rebasing some tests

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

>>> Source/WebCore/Modules/mediasource/MediaSource.cpp:575
>>> +    return result;
>> 
>> This method should be updated to return a RefPtr<> since we transfer ownership to the caller.
> 
> Can you explain the issue?
> A "result" ref is kept by m_sourceBuffers.
> I wonder whether returning a RefPtr<> will not create unneeded refcounting at the JS binding code?

Hmm, given that m_sourceBuffers refs it, I don't feel strongly about it anymore.

[youenn fablet]

Created attachment 277367 [details]
Patch

[WebKit Commit Bot]

Attachment 277367 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Ref.h:200:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Ref.h:205:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Ref.cpp:165:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Ref.cpp:170:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Ref.cpp:175:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Ref.cpp:180:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 6 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 277367 [details]
Patch

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

> Source/WebCore/Modules/mediasource/SourceBuffer.h:90
> +    void appendBuffer(ArrayBufferView*, ExceptionCode&);

Why does this take a pointer instead of a reference?

> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:60
>      size_t index = m_list.find(buffer);

This does not seem to be what we want.

We want to find the pointer, not find the value. And Vector::find will use == to compare Ref<SourceBuffer> with SourceBuffer&, right? That’s bad in two ways. One, it will churn the reference count on the passed in buffer every time it does a comparison. Two, it will compare the SourceBuffer objects rather than comparing the pointers.

While this patch does add the correct behavior for Ref==Ref comparisons, I don’t think we actually want to use that algorithm in the code here from Vector::find for both of those reasons. And maybe we should not be adding the Ref==Ref operation at this time.

Instead I think we need to write out a loop of our own to do the finding, or add another Vector function that is flexible enough that we can use something other than operator==.

> Source/WebCore/Modules/mediasource/SourceBufferList.h:59
> +    bool contains(SourceBuffer& buffer) { return m_list.find(buffer) != notFound; }

Same issue here.

[Darin Adler]

Comment on attachment 277367 [details]
Patch

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

> Source/WTF/wtf/Ref.h:207
> +template<typename T, typename U> inline auto operator==(const Ref<T>& a, const U& b) -> typename std::enable_if<!CanUseEqual<T, U>::value, bool>::type
> +{
> +    return a.ptr() == &b;
> +}

This is *not* a good idea. We don’t want Ref to silently do a pointer comparison if there is no == operator available to compare the values of the two objects. This is unprecedented behavior that might be convenient, but is subtle and dangerous.

I suggest not adding this to Ref at all, and doing something different to deal with the Vector::find that we want to use to find a pointer in a vector of Ref.

>> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:60
>>      size_t index = m_list.find(buffer);
> 
> This does not seem to be what we want.
> 
> We want to find the pointer, not find the value. And Vector::find will use == to compare Ref<SourceBuffer> with SourceBuffer&, right? That’s bad in two ways. One, it will churn the reference count on the passed in buffer every time it does a comparison. Two, it will compare the SourceBuffer objects rather than comparing the pointers.
> 
> While this patch does add the correct behavior for Ref==Ref comparisons, I don’t think we actually want to use that algorithm in the code here from Vector::find for both of those reasons. And maybe we should not be adding the Ref==Ref operation at this time.
> 
> Instead I think we need to write out a loop of our own to do the finding, or add another Vector function that is flexible enough that we can use something other than operator==.

OK, my comment was wrong. Clearly your override of Ref does a pointer comparison with no reference count churn for SourceBuffer&, so the error lies elsewhere.

[Chris Dumez]

(In reply to comment #20)
> Comment on attachment 277367 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277367&action=review
> 
> > Source/WTF/wtf/Ref.h:207
> > +template<typename T, typename U> inline auto operator==(const Ref<T>& a, const U& b) -> typename std::enable_if<!CanUseEqual<T, U>::value, bool>::type
> > +{
> > +    return a.ptr() == &b;
> > +}
> 
> This is *not* a good idea. We don’t want Ref to silently do a pointer
> comparison if there is no == operator available to compare the values of the
> two objects. This is unprecedented behavior that might be convenient, but is
> subtle and dangerous.
> 
> I suggest not adding this to Ref at all, and doing something different to
> deal with the Vector::find that we want to use to find a pointer in a vector
> of Ref.
> 
> >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:60
> >>      size_t index = m_list.find(buffer);
> > 
> > This does not seem to be what we want.
> > 
> > We want to find the pointer, not find the value. And Vector::find will use == to compare Ref<SourceBuffer> with SourceBuffer&, right? That’s bad in two ways. One, it will churn the reference count on the passed in buffer every time it does a comparison. Two, it will compare the SourceBuffer objects rather than comparing the pointers.
> > 
> > While this patch does add the correct behavior for Ref==Ref comparisons, I don’t think we actually want to use that algorithm in the code here from Vector::find for both of those reasons. And maybe we should not be adding the Ref==Ref operation at this time.
> > 
> > Instead I think we need to write out a loop of our own to do the finding, or add another Vector function that is flexible enough that we can use something other than operator==.
> 
> OK, my comment was wrong. Clearly your override of Ref does a pointer
> comparison with no reference count churn for SourceBuffer&, so the error
> lies elsewhere.

Honestly, short-term, I would just keep using RefPtr<> in this Vector.

[Darin Adler]

(In reply to comment #21)
> Honestly, short-term, I would just keep using RefPtr<> in this Vector.

Agreed, I think that’s the best thing to do for now.

[Chris Dumez]

(In reply to comment #21)
> (In reply to comment #20)
> > Comment on attachment 277367 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=277367&action=review
> > 
> > > Source/WTF/wtf/Ref.h:207
> > > +template<typename T, typename U> inline auto operator==(const Ref<T>& a, const U& b) -> typename std::enable_if<!CanUseEqual<T, U>::value, bool>::type
> > > +{
> > > +    return a.ptr() == &b;
> > > +}
> > 
> > This is *not* a good idea. We don’t want Ref to silently do a pointer
> > comparison if there is no == operator available to compare the values of the
> > two objects. This is unprecedented behavior that might be convenient, but is
> > subtle and dangerous.
> > 
> > I suggest not adding this to Ref at all, and doing something different to
> > deal with the Vector::find that we want to use to find a pointer in a vector
> > of Ref.
> > 
> > >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:60
> > >>      size_t index = m_list.find(buffer);
> > > 
> > > This does not seem to be what we want.
> > > 
> > > We want to find the pointer, not find the value. And Vector::find will use == to compare Ref<SourceBuffer> with SourceBuffer&, right? That’s bad in two ways. One, it will churn the reference count on the passed in buffer every time it does a comparison. Two, it will compare the SourceBuffer objects rather than comparing the pointers.
> > > 
> > > While this patch does add the correct behavior for Ref==Ref comparisons, I don’t think we actually want to use that algorithm in the code here from Vector::find for both of those reasons. And maybe we should not be adding the Ref==Ref operation at this time.
> > > 
> > > Instead I think we need to write out a loop of our own to do the finding, or add another Vector function that is flexible enough that we can use something other than operator==.
> > 
> > OK, my comment was wrong. Clearly your override of Ref does a pointer
> > comparison with no reference count churn for SourceBuffer&, so the error
> > lies elsewhere.
> 
> Honestly, short-term, I would just keep using RefPtr<> in this Vector.

Longer term, I think VectorTraits are meant for things like these?

[Darin Adler]

(In reply to comment #23)
> > Honestly, short-term, I would just keep using RefPtr<> in this Vector.
> 
> Longer term, I think VectorTraits are meant for things like these?

I don’t agree. That defines what every Vector does for a given type. In this case I think it’s an issue of we want to do with the particular Vector that’s the issue, not how all Vector<Ref> should work. Unless we let you allocate a Vector with custom traits.

[Chris Dumez]

(In reply to comment #24)
> (In reply to comment #23)
> > > Honestly, short-term, I would just keep using RefPtr<> in this Vector.
> > 
> > Longer term, I think VectorTraits are meant for things like these?
> 
> I don’t agree. That defines what every Vector does for a given type. In this
> case I think it’s an issue of we want to do with the particular Vector
> that’s the issue, not how all Vector<Ref> should work. Unless we let you
> allocate a Vector with custom traits.

Oh right, I assumed we would let you allocate a Vector with custom traits (like we do for HashMap), but it looks like you're right.

[youenn fablet]

(In reply to comment #20)
> Comment on attachment 277367 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277367&action=review
> 
> > Source/WTF/wtf/Ref.h:207
> > +template<typename T, typename U> inline auto operator==(const Ref<T>& a, const U& b) -> typename std::enable_if<!CanUseEqual<T, U>::value, bool>::type
> > +{
> > +    return a.ptr() == &b;
> > +}
> 
> This is *not* a good idea. We don’t want Ref to silently do a pointer
> comparison if there is no == operator available to compare the values of the
> two objects. This is unprecedented behavior that might be convenient, but is
> subtle and dangerous.
> 

Agreed, Ref should remain a reference and not try to be more intelligent.

> Honestly, short-term, I would just keep using RefPtr<> in this Vector.

I'll update the patch accordingly.

[youenn fablet]

Created attachment 277463 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 277463 [details]
Patch

Clearing flags on attachment: 277463

Committed r200198: <http://trac.webkit.org/changeset/200198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.