Bug 211861 – [IPC] Use templates to reduce duplicate code in IPC::Decoder and IPC::Encoder classes

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Use templates to reduce duplicate code in IPC::Decoder and IPC::Encoder classes.

<rdar://problem/62360390>

[David Kilzer (:ddkilzer)]

Created attachment 399299 [details]
Patch v1

[Alex Christensen]

Comment on attachment 399299 [details]
Patch v1

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

> Source/WebKit/ChangeLog:3
> +        [IPC] Use templates to reduce duplicate code in IPC::Decoder and IPC::Encoder classes

This was intentionally not done earlier before because we wanted to avoid serializing size_t because we supported 32-bit plugins in 64-bit hosts.  Now that macOS doesn't support 32-bit executables, I think we can do this.

Do we want to do the same template code deduplication with PersistentEncoder and PersistentDecoder?

> Source/WebKit/Platform/IPC/Decoder.h:127
> +        // FIXME: Check that this is a valid enum value.

I believe decodeEnum is only used for decoding legacy enum types without EnumTraits.  I think this FIXME should read: migrate all uses of this function to operator>> with proper value checks and remove this.

> Source/WebKit/Platform/IPC/Encoder.h:67
> +        // FIXME: Check that this is a valid enum value.

We typically don't do enum validity checking on the encoding side.  It's unnecessary slowdown.  If it's a compromised process, they won't check anyways.

> Source/WebKit/Platform/IPC/Encoder.h:93
> +    template<typename T>
> +    auto encode(T value) -> std::enable_if_t<std::is_arithmetic<T>::value>

This syntax makes it very hard to see what is returned.  Could you use the same syntax as everything else in this file and have two template parameters, the second with std::enable_if_t?

[David Kilzer (:ddkilzer)]

Comment on attachment 399299 [details]
Patch v1

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

>> Source/WebKit/ChangeLog:3
>> +        [IPC] Use templates to reduce duplicate code in IPC::Decoder and IPC::Encoder classes
> 
> This was intentionally not done earlier before because we wanted to avoid serializing size_t because we supported 32-bit plugins in 64-bit hosts.  Now that macOS doesn't support 32-bit executables, I think we can do this.
> 
> Do we want to do the same template code deduplication with PersistentEncoder and PersistentDecoder?

Good to know the history here.

Yes, we could do this to PersistentEncoder and PersistentDecoder in a separate patch.

>> Source/WebKit/Platform/IPC/Decoder.h:127
>> +        // FIXME: Check that this is a valid enum value.
> 
> I believe decodeEnum is only used for decoding legacy enum types without EnumTraits.  I think this FIXME should read: migrate all uses of this function to operator>> with proper value checks and remove this.

Sure, I can change this.  I wanted to use WTF::isValidEnum<EnumType>() to check the type, and I assume that would have lead me down the enum class/EnumTraits path, but I didn't know how to say it.

>> Source/WebKit/Platform/IPC/Encoder.h:67
>> +        // FIXME: Check that this is a valid enum value.
> 
> We typically don't do enum validity checking on the encoding side.  It's unnecessary slowdown.  If it's a compromised process, they won't check anyways.

It's more about catching our own mistakes.  This method has a Debug assertion:

    template<typename E>
    auto encode(E value) -> std::enable_if_t<std::is_enum<E>::value>
    {
        ASSERT(WTF::isValidEnum<E>(static_cast<typename std::underlying_type<E>::type>(value)));
        encode(static_cast<typename std::underlying_type<E>::type>(value));
    }

I assume that's okay to add?  So change "Check" to "Add Debug assert"?

Do we eventually want to get rid of encodeEnum() (just like decodeEnum()) and just use the templated encode() (same method pasted above)?

> Source/WebKit/Platform/IPC/Encoder.h:80
> +        // FIXME: Check that this is a valid enum value.

Here, too.

>> Source/WebKit/Platform/IPC/Encoder.h:93
>> +    auto encode(T value) -> std::enable_if_t<std::is_arithmetic<T>::value>
> 
> This syntax makes it very hard to see what is returned.  Could you use the same syntax as everything else in this file and have two template parameters, the second with std::enable_if_t?

Clearly it's `void`.  (There are a lot of places where I think `auto` is not clear, but I keep my mouth shut because I don't care enough to spend time discussing it.)  Happy to change this one, though.

Should I change this one as well?  It has the same issue.

    template<typename E>
    auto encode(E value) -> std::enable_if_t<std::is_enum<E>::value>
    {
        ASSERT(WTF::isValidEnum<E>(static_cast<typename std::underlying_type<E>::type>(value)));
        encode(static_cast<typename std::underlying_type<E>::type>(value));
    }

[Alex Christensen]

Comment on attachment 399299 [details]
Patch v1

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

>>> Source/WebKit/Platform/IPC/Decoder.h:127
>>> +        // FIXME: Check that this is a valid enum value.
>> 
>> I believe decodeEnum is only used for decoding legacy enum types without EnumTraits.  I think this FIXME should read: migrate all uses of this function to operator>> with proper value checks and remove this.
> 
> Sure, I can change this.  I wanted to use WTF::isValidEnum<EnumType>() to check the type, and I assume that would have lead me down the enum class/EnumTraits path, but I didn't know how to say it.

We should also look for places where we just cast enum values to integral types for IPC.  There are still a few of them around from the good ol' days.

>>> Source/WebKit/Platform/IPC/Encoder.h:67
>>> +        // FIXME: Check that this is a valid enum value.
>> 
>> We typically don't do enum validity checking on the encoding side.  It's unnecessary slowdown.  If it's a compromised process, they won't check anyways.
> 
> It's more about catching our own mistakes.  This method has a Debug assertion:
> 
>     template<typename E>
>     auto encode(E value) -> std::enable_if_t<std::is_enum<E>::value>
>     {
>         ASSERT(WTF::isValidEnum<E>(static_cast<typename std::underlying_type<E>::type>(value)));
>         encode(static_cast<typename std::underlying_type<E>::type>(value));
>     }
> 
> I assume that's okay to add?  So change "Check" to "Add Debug assert"?
> 
> Do we eventually want to get rid of encodeEnum() (just like decodeEnum()) and just use the templated encode() (same method pasted above)?

Debug assertion is fine.

>>> Source/WebKit/Platform/IPC/Encoder.h:93
>>> +    auto encode(T value) -> std::enable_if_t<std::is_arithmetic<T>::value>
>> 
>> This syntax makes it very hard to see what is returned.  Could you use the same syntax as everything else in this file and have two template parameters, the second with std::enable_if_t?
> 
> Clearly it's `void`.  (There are a lot of places where I think `auto` is not clear, but I keep my mouth shut because I don't care enough to spend time discussing it.)  Happy to change this one, though.
> 
> Should I change this one as well?  It has the same issue.
> 
>     template<typename E>
>     auto encode(E value) -> std::enable_if_t<std::is_enum<E>::value>
>     {
>         ASSERT(WTF::isValidEnum<E>(static_cast<typename std::underlying_type<E>::type>(value)));
>         encode(static_cast<typename std::underlying_type<E>::type>(value));
>     }

Yes, please change them both.  A template parameter indicating what the template is used for is much better than a mess in the return type, especially with an implicit return type.

[David Kilzer (:ddkilzer)]

Comment on attachment 399299 [details]
Patch v1

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

>>> Source/WebKit/Platform/IPC/Decoder.h:127
>>> +        // FIXME: Check that this is a valid enum value.
>> 
>> I believe decodeEnum is only used for decoding legacy enum types without EnumTraits.  I think this FIXME should read: migrate all uses of this function to operator>> with proper value checks and remove this.
> 
> Sure, I can change this.  I wanted to use WTF::isValidEnum<EnumType>() to check the type, and I assume that would have lead me down the enum class/EnumTraits path, but I didn't know how to say it.

Should this method also have a FIXME to remove it?  If not, I don't understand why.

    template<typename E, std::enable_if_t<std::is_enum<E>::value>* = nullptr>
    WARN_UNUSED_RETURN bool decode(E& enumValue)
    {   
        typename std::underlying_type<E>::type value;
        if (!decode(value))
            return false;
        if (!WTF::isValidEnum<E>(value))
            return false;
        
        enumValue = static_cast<E>(value);
        return true;
    }

[Alex Christensen]

We should remove decodeEnum and rename all of them to decode, and also be pushing towards using operator>>
Same with encodeEnum, which is super easy to replace with operator<<

[David Kilzer (:ddkilzer)]

(In reply to Alex Christensen from comment #6)
> We should remove decodeEnum and rename all of them to decode, and also be
> pushing towards using operator>>
> Same with encodeEnum, which is super easy to replace with operator<<

Okay, got it. Thanks!

[David Kilzer (:ddkilzer)]

Created attachment 399316 [details]
Patch for landing

[EWS]

Committed r261672: <https://trac.webkit.org/changeset/261672>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399316 [details].