Bug 231447

Summary: DrawImageBuffer, DrawNativeImage and DrawPattern should not be inline items
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebCore Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED WONTFIX    
Severity: Normal CC: mmaxfield, sabouhallawa, sam, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 230860    
Attachments:
Description Flags
For EWS
none
For EWS
none
Fix failing tests
none
Fix ChangeLog typo none

Description Wenson Hsieh 2021-10-08 13:47:57 PDT
.
Comment 1 Wenson Hsieh 2021-10-08 14:04:34 PDT
Created attachment 440670 [details]
For EWS
Comment 2 Wenson Hsieh 2021-10-08 14:35:34 PDT
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)
Comment 3 Wenson Hsieh 2021-10-08 14:36:54 PDT Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2021-10-08 17:57:56 PDT
Created attachment 440696 [details]
Fix failing tests
Comment 5 Myles C. Maxfield 2021-10-09 00:41:52 PDT
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.
Comment 6 Wenson Hsieh 2021-10-09 10:51:20 PDT
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.
Comment 7 Wenson Hsieh 2021-10-09 11:10:37 PDT
Created attachment 440713 [details]
Fix ChangeLog typo
Comment 8 Sam Weinig 2021-10-10 10:21:09 PDT
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?
Comment 9 Wenson Hsieh 2021-10-10 10:41:20 PDT
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.
Comment 10 Wenson Hsieh 2021-10-10 17:02:08 PDT
I think I'm going to adjust my approach here, such that I won't require this patch.