Bug 231447 – DrawImageBuffer, DrawNativeImage and DrawPattern should not be inline items

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 440670 [details]
For EWS

[Wenson Hsieh]

Comment on attachment 440670 [details]
For EWS

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

Thanks for the review!

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1165
>      std::optional<FloatRect> globalBounds() const { return std::nullopt; }

(It just occurred to me that I forgot to remove the now-unnecessary `isValid()` method in DrawPattern, so I'll do that before landing)

[Wenson Hsieh]

Created attachment 440674 [details]
For EWS

[Wenson Hsieh]

Created attachment 440696 [details]
Fix failing tests

[Myles C. Maxfield]

Comment on attachment 440696 [details]
Fix failing tests

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

> Source/WebCore/ChangeLog:8
> +        These three display list items all contain ImagePaintingOptions, which (in turn) contains strongly typed enums.

nit: ImagePaintingOptions is plural, so it should use "contain" instead of "contains." Or maybe "Each of which contain"

> Source/WebCore/ChangeLog:13
> +        we should just make them non-inline and implement encoding/decoding logic as static methods on the items

This is unclear. If they're just enums, why should they be out-of-line items? Enums are tiny.

[Wenson Hsieh]

Comment on attachment 440696 [details]
Fix failing tests

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

>> Source/WebCore/ChangeLog:8
>> +        These three display list items all contain ImagePaintingOptions, which (in turn) contains strongly typed enums.
> 
> nit: ImagePaintingOptions is plural, so it should use "contain" instead of "contains." Or maybe "Each of which contain"

Fixed!

>> Source/WebCore/ChangeLog:13
>> +        we should just make them non-inline and implement encoding/decoding logic as static methods on the items
> 
> This is unclear. If they're just enums, why should they be out-of-line items? Enums are tiny.

The issue is that we don't have a mechanism for validating the underlying types of these strongly typed enums when `memcpy`-ing these items cross-process, which triggers UB.

It's technically possible to address this by storing all of these as the underlying type directly (e.g. `uint8_t`) when sending them to the GPUP so we can validate the values, but in the streamable IPC world, this won't really make a difference.

[Wenson Hsieh]

Created attachment 440713 [details]
Fix ChangeLog typo

[Sam Weinig]

Comment on attachment 440713 [details]
Fix ChangeLog typo

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

> Source/WebCore/ChangeLog:14
> +        Instead of adding SimpleArgumentCoder support for these display list item types when implementing streaming IPC,
> +        we should just make them non-inline and implement encoding/decoding logic as static methods on the items
> +        themselves.

Why is it better to make them out of line than add the enum traits / argument coder support for these enums?

[Wenson Hsieh]

Comment on attachment 440713 [details]
Fix ChangeLog typo

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

>> Source/WebCore/ChangeLog:14
>> +        themselves.
> 
> Why is it better to make them out of line than add the enum traits / argument coder support for these enums?

Hm.. not sure what you mean by this — we already have enum traits / argument coder support for these enums (and the options struct).

Making these items "out of line" means making these display list items work with argument coders, as opposed to being memcpy'd directly in and out of shared memory. Currently, we only do the latter, which bypasses validation for these enum types.

[Wenson Hsieh]

I think I'm going to adjust my approach here, such that I won't require this patch.