Bug 224090 – [CoreIPC] Encoding/decoding of WebCore::CapabilityValueOrRange transmits padding bytes

• Component: WebKit2
• Status: NEW

[David Kilzer (:ddkilzer)]

[CoreIPC] Encoding/decoding of WebCore::CapabilityValueOrRange transmits padding bytes.

Currently ValueUnion objects are encoded as a block of memory instead of as individual values.

<rdar://problem/75981660>

[David Kilzer (:ddkilzer)]

Note for potential reviewers (especially Alex):

WebCore::CapabilityValueOrRange could be changed to be a WTF::Variant, but to keep this patch simple, I started by fixing the issue of sending padding bytes first.

Also, it's been long enough since I've worked on CoreIPC that I'm not sure if this is a step forward to the "new" way of writing IPC decoders, but it keeps RealtimeMediaSourceCapabilities::decode() very simple to read by hiding the complexity in the CapabilityValueOrRange::decoder() method and in IPC/ArgumentCoders.h:

template<class Decoder>
bool RealtimeMediaSourceCapabilities::decode(Decoder& decoder, RealtimeMediaSourceCapabilities& capabilities)
{
    return decoder.decode(capabilities.m_width)
        && decoder.decode(capabilities.m_height)
        && decoder.decode(capabilities.m_aspectRatio)
        && decoder.decode(capabilities.m_frameRate)
        && decoder.decode(capabilities.m_facingMode)
        && decoder.decode(capabilities.m_volume)
        && decoder.decode(capabilities.m_sampleRate)
        && decoder.decode(capabilities.m_sampleSize)
        && decoder.decode(capabilities.m_echoCancellation)
        && decoder.decode(capabilities.m_deviceId)
        && decoder.decode(capabilities.m_groupId)
        && decoder.decode(capabilities.m_supportedConstraints);
}

[David Kilzer (:ddkilzer)]

Created attachment 424979 [details]
Patch v1

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #2)
> Created attachment 424979 [details]
> Patch v1

I may have over-used makeOptional() as well.  Should I remove every makeOptional() that's not necessary?

[Alex Christensen]

Comment on attachment 424979 [details]
Patch v1

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

I personally prefer {{ }} over makeOptional.

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:105
> +    ValueUnion m_minOrValue;

Instead of all this, why don't we just use Variant<double, int, std::pair<double, double>, std::pair<int, int>>?

> Source/WebKit/Platform/IPC/ArgumentCoders.h:181
> +        optional = U::decode(decoder);

Are there types where this doesn't work, or could we just remove the is_default_constructible version of this?

[David Kilzer (:ddkilzer)]

Comment on attachment 424979 [details]
Patch v1

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

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:105
>> +    ValueUnion m_minOrValue;
> 
> Instead of all this, why don't we just use Variant<double, int, std::pair<double, double>, std::pair<int, int>>?

See Comment #1.  Switching to use a Variant<> was more complexity than I wanted to tackle in this patch.

BTW, any advantage to using std::pair<double, double> instead of Vector<double>[2]?  I though std::pair<> was most commonly used for two different types.

>> Source/WebKit/Platform/IPC/ArgumentCoders.h:181
>> +        optional = U::decode(decoder);
> 
> Are there types where this doesn't work, or could we just remove the is_default_constructible version of this?

I wouldn't have guessed this, but the is_default_constructible<> version is not used!  Will post a new patch.

[Alex Christensen]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Comment on attachment 424979 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424979&action=review
> 
> >> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:105
> >> +    ValueUnion m_minOrValue;
> > 
> > Instead of all this, why don't we just use Variant<double, int, std::pair<double, double>, std::pair<int, int>>?
> 
> See Comment #1.  Switching to use a Variant<> was more complexity than I
> wanted to tackle in this patch.
I think it's exactly as much complexity as should be tackled in this patch.
> 
> BTW, any advantage to using std::pair<double, double> instead of
> Vector<double>[2]?  I though std::pair<> was most commonly used for two
> different types.
std::pair keeps enough space for two things.  Vector keeps a size and a pointer to a dynamically allocated buffer.

[David Kilzer (:ddkilzer)]

(In reply to Alex Christensen from comment #6)
> (In reply to David Kilzer (:ddkilzer) from comment #5)
> > Comment on attachment 424979 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=424979&action=review
> > 
> > >> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:105
> > >> +    ValueUnion m_minOrValue;
> > > 
> > > Instead of all this, why don't we just use Variant<double, int, std::pair<double, double>, std::pair<int, int>>?
> > 
> > See Comment #1.  Switching to use a Variant<> was more complexity than I
> > wanted to tackle in this patch.
> 
> I think it's exactly as much complexity as should be tackled in this patch.

I wrote the patch to switch to Variant, but it turned out a bit bigger than I would have liked (though not too big to review).

> > BTW, any advantage to using std::pair<double, double> instead of
> > Vector<double>[2]?  I though std::pair<> was most commonly used for two
> > different types.
> 
> std::pair keeps enough space for two things.  Vector keeps a size and a
> pointer to a dynamically allocated buffer.

Great point.  I also used C++17 strutted bindings in one source file to hide the need to use .first and .second.

<https://en.cppreference.com/w/cpp/language/structured_binding>

[David Kilzer (:ddkilzer)]

Created attachment 425546 [details]
Patch v2

[Darin Adler]

Comment on attachment 425546 [details]
Patch v2

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

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:44
> +    using ULong = int;

What does the "U" in "ULong" mean? If it means "unsigned" then why is the type "signed int"?

[David Kilzer (:ddkilzer)]

Created attachment 425557 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Comment on attachment 425546 [details]
Patch v2

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

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:44
>> +    using ULong = int;
> 
> What does the "U" in "ULong" mean? If it means "unsigned" then why is the type "signed int"?

I don't know.  One would imagine it means "unsigned" as in "unsigned long", but one would be mistaken because LongRange.idl defines the value as an int.

Guess I'll change it to Long/LongRange then!  (Not sure why the original enums used "ULong", though.)

[David Kilzer (:ddkilzer)]

Comment on attachment 425546 [details]
Patch v2

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

>>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:44
>>> +    using ULong = int;
>> 
>> What does the "U" in "ULong" mean? If it means "unsigned" then why is the type "signed int"?
> 
> I don't know.  One would imagine it means "unsigned" as in "unsigned long", but one would be mistaken because LongRange.idl defines the value as an int.
> 
> Guess I'll change it to Long/LongRange then!  (Not sure why the original enums used "ULong", though.)

I also wondered if I should change "Double" and "Long" to "DoubleValue" and "LongValue" just so they aren't so generic.

Feedback welcome.

[David Kilzer (:ddkilzer)]

Created attachment 425558 [details]
Patch v4

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #10)
> Created attachment 425557 [details]
> Patch v3

Build fix for GTK port.

(In reply to David Kilzer (:ddkilzer) from comment #13)
> Created attachment 425558 [details]
> Patch v4

Rename ULong/ULongRange to Long/LongRange.

[youenn fablet]

Comment on attachment 425558 [details]
Patch v4

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

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:102
> +    const CapabilityValueOrRange& width() const { return *m_width; }

There is no guarantee that m_width might not be null here.

[Eric Carlson]

(In reply to David Kilzer (:ddkilzer) from comment #11)
> Comment on attachment 425546 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425546&action=review
> 
> >> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:44
> >> +    using ULong = int;
> > 
> > What does the "U" in "ULong" mean? If it means "unsigned" then why is the type "signed int"?
> 
> I don't know.  One would imagine it means "unsigned" as in "unsigned long",
> but one would be mistaken because LongRange.idl defines the value as an int.
> 
> Guess I'll change it to Long/LongRange then!  (Not sure why the original
> enums used "ULong", though.)

Looks like that is my fault. The value was `unsigned long` when I added ValueUnion in r193389:

    union ValueUnion {
        unsigned long asULong;
        float asFloat;
    };

but when I changed it to `int` in r205348 I neglected to update the enum.

[Alex Christensen]

Comment on attachment 425558 [details]
Patch v4

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

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:319
> +            WTFLogAlways("RealtimeMediaSource::supportsSizeAndFrameRate failed width constraint, capabilities are [%d, %d]", *range.min, *range.max);

This also looks like it's unchecked dereferencing something.  Maybe an Optional.  I don't think it should.  It should be 0 or the dereferenced value like it was.

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:156
> +    Optional<CapabilityValueOrRange> m_width;

Why were these changed to Optional?