Bug 230714 – [WebKit2] Refactor some IPC argument encoder logic to work with StreamConnectionEncoder

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

Work towards using StreamConnectionEncoder for concurrent display list item processing between web/GPU processes.

[Wenson Hsieh]

Created attachment 439086 [details]
For EWS

[Wenson Hsieh]

Created attachment 439089 [details]
For EWS

[Wenson Hsieh]

Created attachment 439099 [details]
Try to fix WPE build

[Wenson Hsieh]

Created attachment 439104 [details]
Try to fix WPE build again

[Simon Fraser (smfr)]

Comment on attachment 439104 [details]
Try to fix WPE build again

Nice!

[Wenson Hsieh]

Comment on attachment 439104 [details]
Try to fix WPE build again

Thanks for the review!

[EWS]

Committed r283024 (242085@main): <https://commits.webkit.org/242085@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/83477591>

[Kimmo Kinnunen]

What is "POD" is a fuzzy abstraction of "trivially copyable", so it's sometimes thought wrong.
I think originally the intention of the IPC encoding was that encoder wouldn't read padding bits that would alert ASAN/MSAN.

[Kimmo Kinnunen]

David, do you remember if there's a problem with IPC encoding and memcpy reading arbitrary types? E.g. the types might have fields padded and the padding would be uninitialised and some sanitisers would report read of uninitialised memory?

[Wenson Hsieh]

(In reply to Kimmo Kinnunen from comment #10)
> David, do you remember if there's a problem with IPC encoding and memcpy
> reading arbitrary types? E.g. the types might have fields padded and the
> padding would be uninitialised and some sanitisers would report read of
> uninitialised memory?

Note that we were already using SimpleArgumentCoder for most of the WebCore types that I modified here (just hard-coded to use `IPC::Encoder`) — the only new ones are some CG structs (namely CGRect, CGPoint, CGSize, CGAffineTransform), which are only comprised of CGFloats that *should* be safe to memcpy.

That said, it does look like a couple of webanimation tests are failing.. I'm looking into these to see if they're actually related to the changes here.

[Kimmo Kinnunen]

(In reply to Wenson Hsieh from comment #11)
> That said, it does look like a couple of webanimation tests are failing..
> I'm looking into these to see if they're actually related to the changes
> here.

Only Length fails is_trivially_constructible, so everything this patch adds should be memcpyable

[Eric Hutchison]

Created attachment 439163 [details]
Related Crash Log

[Eric Hutchison]

Reverted r283024 for reason:

Causes slowdown and crash on EWS

Committed r283048 (242108@main): <https://commits.webkit.org/242108@main>

[Wenson Hsieh]

Comment on attachment 439104 [details]
Try to fix WPE build again

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

> Source/WebKit/Shared/WebCoreArgumentCoders.h:241
> +DEFINE_SIMPLE_ARGUMENT_CODER(WebCore::TransformationMatrix)

Using SimpleArgumentCoder for WebCore::TransformationMatrix seems to be the (immediate) cause of the crashes, though it's not obvious to me why.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #15)
> Comment on attachment 439104 [details]
> Try to fix WPE build again
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439104&action=review
> 
> > Source/WebKit/Shared/WebCoreArgumentCoders.h:241
> > +DEFINE_SIMPLE_ARGUMENT_CODER(WebCore::TransformationMatrix)
> 
> Using SimpleArgumentCoder for WebCore::TransformationMatrix seems to be the
> (immediate) cause of the crashes, though it's not obvious to me why.

Alright, I think I've figured it out.

Some logging demonstrates that we sometimes fail to properly decode transformation matrices, due to alignment adjustment logic in IPC::Encoder and IPC::Decoder being inconsistent:

```
Web process:

  Encoding TransformationMatrix #214
    [1.00 0.00 0.00 0.00]
    [0.00 1.00 0.00 0.00]
    [0.00 0.00 1.00 0.00]
    [50.00 0.00 0.00 1.00]
  Rounding up size: 56 to 64 (for alignment: 16)

UI process:

  Decoding TransformationMatrix #214
  Rounding up offset: 0x1373f8500 to 0x1373f8500 (for alignment: 16)
      got
    [0.00 1.00 0.00 0.00]
    [0.00 0.00 1.00 0.00]
    [0.00 0.00 0.00 1.00]
    [0.00 50.00 0.00 0.00]
```

On the Encoder side, we round the buffer size (in this particular case, 56) up to 64 to match the 16-byte align of TransformationMatrix:

```
uint8_t* Encoder::grow(size_t alignment, size_t size)
{
    size_t alignedSize = roundUpToAlignment(m_bufferSize, alignment);
```

..on the Decoder side, however, we don't end up rounding the read offset at all, since we round the _buffer pointer address_ up to the alignment of 16 rather than the offset relative to the start of the buffer.

```
bool Decoder::alignBufferPosition(size_t alignment, size_t size)
{
    const uint8_t* alignedPosition = roundUpToAlignment(m_bufferPos, alignment);
    if (UNLIKELY(!alignedBufferIsLargeEnoughToContain(alignedPosition, m_buffer, m_bufferEnd, size))) {

```

This diff "fixes" this discrepancy by making Decoder-side rounding logic consistent with Encoder-side rounding logic:

```
diff --git a/Source/WebKit/Platform/IPC/Decoder.cpp b/Source/WebKit/Platform/IPC/Decoder.cpp
index 743cb481561b..45a0e7d43cb5 100644
--- a/Source/WebKit/Platform/IPC/Decoder.cpp
+++ b/Source/WebKit/Platform/IPC/Decoder.cpp
@@ -179,6 +179,11 @@ static inline const uint8_t* roundUpToAlignment(const uint8_t* ptr, size_t align
     return reinterpret_cast<uint8_t*>((reinterpret_cast<uintptr_t>(ptr) + alignmentMask) & ~alignmentMask);
 }

+static inline size_t roundUpToAlignment(size_t value, size_t alignment)
+{
+    return ((value + alignment - 1) / alignment) * alignment;
+}
+
 static inline bool alignedBufferIsLargeEnoughToContain(const uint8_t* alignedPosition, const uint8_t* bufferStart, const uint8_t* bufferEnd, size_t size)
 {
     // When size == 0 for the last argument and it's a variable length byte array,
@@ -190,7 +195,9 @@ static inline bool alignedBufferIsLargeEnoughToContain(const uint8_t* alignedPos

 bool Decoder::alignBufferPosition(size_t alignment, size_t size)
 {
-    const uint8_t* alignedPosition = roundUpToAlignment(m_bufferPos, alignment);
+    size_t currentSize = m_bufferPos - m_buffer;
+    auto alignedSize = roundUpToAlignment(currentSize, alignment);
+    auto* alignedPosition = m_buffer + alignedSize;
     if (UNLIKELY(!alignedBufferIsLargeEnoughToContain(alignedPosition, m_buffer, m_bufferEnd, size))) {
         // We've walked off the end of this buffer.
         markInvalid();
```

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #16)
> (In reply to Wenson Hsieh from comment #15)
> > Comment on attachment 439104 [details]
> > Try to fix WPE build again
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=439104&action=review
> > 
> > > Source/WebKit/Shared/WebCoreArgumentCoders.h:241
> > > +DEFINE_SIMPLE_ARGUMENT_CODER(WebCore::TransformationMatrix)
> > 
> > Using SimpleArgumentCoder for WebCore::TransformationMatrix seems to be the
> > (immediate) cause of the crashes, though it's not obvious to me why.
> 
> Alright, I think I've figured it out.
> 
> Some logging demonstrates that we sometimes fail to properly decode
> transformation matrices, due to alignment adjustment logic in IPC::Encoder
> and IPC::Decoder being inconsistent:
> […]

...let's deal with this separately: filed https://bugs.webkit.org/show_bug.cgi?id=230776

[Wenson Hsieh]

Created attachment 439205 [details]
Take two

[Wenson Hsieh]

Created attachment 439208 [details]
Actual take two

[Wenson Hsieh]

Created attachment 439222 [details]
Try to fix WPE (again)

[Wenson Hsieh]

Created attachment 439226 [details]
Try to fix WPE?

[Wenson Hsieh]

Created attachment 439227 [details]
Test WPE fix?

[Wenson Hsieh]

Created attachment 439235 [details]
Test WPE fix?

[Wenson Hsieh]

Created attachment 439238 [details]
Fix WPE build

[Wenson Hsieh]

Created attachment 440153 [details]
Rebase on trunk

[Wenson Hsieh]

Created attachment 440218 [details]
Try to fix WPE again?

[Wenson Hsieh]

Created attachment 440219 [details]
Try to fix WPE again?

[Wenson Hsieh]

Created attachment 440237 [details]
Does this build on WPE?

[Wenson Hsieh]

(Adding a few folks who might be familiar with WPE)

Would anyone happen to know why attempts to templatize the `encode(Encoder&)` function several WebCore structs (e.g. FloatRect, IntSize, and a few others) could cause linker errors on WPE only?

Link to build failure: <https://ews-build.webkit.org/#/builders/8/builds/63759>.

[Michael Catanzaro]

(In reply to Wenson Hsieh from comment #29)
> Would anyone happen to know why attempts to templatize the
> `encode(Encoder&)` function several WebCore structs (e.g. FloatRect,
> IntSize, and a few others) could cause linker errors on WPE only?

Guess: it's probably because the code is defining template functions in the C++ source file, which requires special care. If the functions are called from outside the translation unit, they have to be either (a) defined in the header file instead, so they're guaranteed to be available in the translation unit they are called from, or else (b) explicitly instantiated using 'extern template'. This is documented here: https://gcc.gnu.org/onlinedocs/gcc/Template-Instantiation.html

[Wenson Hsieh]

Created attachment 440242 [details]
Does *this* build on WPE?

[Wenson Hsieh]

Created attachment 440253 [details]
Does *this* build on WPE?

[Wenson Hsieh]

(In reply to Michael Catanzaro from comment #30)
> (In reply to Wenson Hsieh from comment #29)
> > Would anyone happen to know why attempts to templatize the
> > `encode(Encoder&)` function several WebCore structs (e.g. FloatRect,
> > IntSize, and a few others) could cause linker errors on WPE only?
> 
> Guess: it's probably because the code is defining template functions in the
> C++ source file, which requires special care. If the functions are called
> from outside the translation unit, they have to be either (a) defined in the
> header file instead, so they're guaranteed to be available in the
> translation unit they are called from, or else (b) explicitly instantiated
> using 'extern template'. This is documented here:
> https://gcc.gnu.org/onlinedocs/gcc/Template-Instantiation.html

Hm...so my patch moves the definitions of the template functions to the header file (WebCoreArgumentCoders.h), which I thought would be okay because the header file is included in all the places that currently try to encode or decode these objects. That said, I also tried explicit template instantiation (with and without the `extern` keyword), but that didn't seem to have any effect :/

Maybe the template instantiation needs to be in the CPP source instead of the header, though? Going to try this next, and see if it makes any difference.

[Wenson Hsieh]

Created attachment 440263 [details]
Try to fix WPE again?

[Wenson Hsieh]

Created attachment 440281 [details]
Does this build on WPE??

[Wenson Hsieh]

Created attachment 440284 [details]
Try to fix the WPE build again?

[Wenson Hsieh]

Created attachment 440288 [details]
Another test patch for WPE

[Wenson Hsieh]

Created attachment 440300 [details]
Another test patch for WPE

[Wenson Hsieh]

Created attachment 440306 [details]
Try to fix the WPE build again? (1)

[Wenson Hsieh]

Created attachment 440307 [details]
Try to fix the WPE build again? (2)

[Wenson Hsieh]

Created attachment 440308 [details]
Try to fix the WPE build again? (3)

[Wenson Hsieh]

Created attachment 440309 [details]
Try to fix the WPE build again? (4)

[Wenson Hsieh]

Created attachment 440311 [details]
Try to fix the WPE build again? (5)

[Wenson Hsieh]

Created attachment 440312 [details]
Try to fix the WPE build again? (6)

[Wenson Hsieh]

Created attachment 440313 [details]
Try to fix the WPE build again? (7)

[Wenson Hsieh]

Created attachment 440314 [details]
Try to fix the WPE build again? (8)

[Wenson Hsieh]

Created attachment 440315 [details]
Try to fix the WPE build again? (9)

[EWS]

Committed r283617 (242569@main): <https://commits.webkit.org/242569@main>

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

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #42)
> Created attachment 440309 [details]
> Try to fix the WPE build again? (4)

After some trial and error yesterday, I figured out that this patch fails for WPE in EWS when the bot used to build the patch is `aperez-wpe-ews`; however, it does not fail when `igalia-wpe-ews` is used instead. As far as I can tell, these both use incremental builds, so it's a bit of a mystery to me why it fails for `aperez-wpe-ews`.

Tim Horton also helped confirm that all of the patches labeled "Try to fix the WPE build again? (1-9)" build successfully (locally) against WPE on an Arch Linux machine.

[Adrian Perez]

(In reply to Wenson Hsieh from comment #49)
> (In reply to Wenson Hsieh from comment #42)
> > Created attachment 440309 [details]
> > Try to fix the WPE build again? (4)
> 
> After some trial and error yesterday, I figured out that this patch fails
> for WPE in EWS when the bot used to build the patch is `aperez-wpe-ews`;
> however, it does not fail when `igalia-wpe-ews` is used instead. As far as I
> can tell, these both use incremental builds, so it's a bit of a mystery to
> me why it fails for `aperez-wpe-ews`.

I also am not sure why the build failed there, but I forced a fresh full
build by removing the build directory for the EWS and it seems to be
chugging along nicely now.