Bug 230714 - [WebKit2] Refactor some IPC argument encoder logic to work with StreamConnectionEncoder
Summary: [WebKit2] Refactor some IPC argument encoder logic to work with StreamConnect...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 230860
  Show dependency treegraph
 
Reported: 2021-09-23 12:55 PDT by Wenson Hsieh
Modified: 2021-10-07 13:02 PDT (History)
12 users (show)

See Also:


Attachments
For EWS (45.18 KB, patch)
2021-09-23 13:26 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (44.20 KB, patch)
2021-09-23 14:00 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix WPE build (46.74 KB, patch)
2021-09-23 15:39 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Try to fix WPE build again (36.74 KB, patch)
2021-09-23 16:25 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Related Crash Log (131.02 KB, text/plain)
2021-09-24 10:53 PDT, Eric Hutchison
no flags Details
Take two (34.20 KB, patch)
2021-09-24 16:16 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Actual take two (32.74 KB, patch)
2021-09-24 16:44 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix WPE (again) (33.30 KB, patch)
2021-09-24 17:46 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix WPE? (33.09 KB, patch)
2021-09-24 18:42 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Test WPE fix? (33.29 KB, patch)
2021-09-24 18:53 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Test WPE fix? (33.22 KB, patch)
2021-09-24 22:24 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix WPE build (33.11 KB, patch)
2021-09-24 23:14 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebase on trunk (35.05 KB, patch)
2021-10-04 21:01 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix WPE again? (34.33 KB, patch)
2021-10-05 08:41 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix WPE again? (34.34 KB, patch)
2021-10-05 09:03 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Does this build on WPE? (3.02 KB, patch)
2021-10-05 11:09 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Does *this* build on WPE? (2.17 KB, patch)
2021-10-05 11:51 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Does *this* build on WPE? (2.25 KB, patch)
2021-10-05 12:56 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix WPE again? (35.63 KB, patch)
2021-10-05 14:13 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Does this build on WPE?? (4.81 KB, patch)
2021-10-05 15:28 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix the WPE build again? (36.18 KB, patch)
2021-10-05 15:45 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Another test patch for WPE (3.95 KB, patch)
2021-10-05 16:14 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Another test patch for WPE (3.62 KB, patch)
2021-10-05 17:00 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Try to fix the WPE build again? (1) (36.20 KB, patch)
2021-10-05 17:22 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Try to fix the WPE build again? (2) (36.20 KB, patch)
2021-10-05 17:23 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Try to fix the WPE build again? (3) (36.20 KB, patch)
2021-10-05 17:24 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Try to fix the WPE build again? (4) (36.20 KB, patch)
2021-10-05 17:27 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix the WPE build again? (5) (36.20 KB, patch)
2021-10-05 17:29 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Try to fix the WPE build again? (6) (36.20 KB, patch)
2021-10-05 17:34 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix the WPE build again? (7) (36.20 KB, patch)
2021-10-05 17:34 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix the WPE build again? (8) (36.20 KB, patch)
2021-10-05 17:35 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix the WPE build again? (9) (36.20 KB, patch)
2021-10-05 17:36 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-09-23 12:55:05 PDT
Work towards using StreamConnectionEncoder for concurrent display list item processing between web/GPU processes.
Comment 1 Wenson Hsieh 2021-09-23 13:26:22 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-09-23 14:00:09 PDT Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-09-23 15:39:42 PDT Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2021-09-23 16:25:50 PDT
Created attachment 439104 [details]
Try to fix WPE build again
Comment 5 Simon Fraser (smfr) 2021-09-23 17:28:27 PDT
Comment on attachment 439104 [details]
Try to fix WPE build again

Nice!
Comment 6 Wenson Hsieh 2021-09-23 19:40:20 PDT
Comment on attachment 439104 [details]
Try to fix WPE build again

Thanks for the review!
Comment 7 EWS 2021-09-23 20:02:21 PDT
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].
Comment 8 Radar WebKit Bug Importer 2021-09-23 20:03:18 PDT
<rdar://problem/83477591>
Comment 9 Kimmo Kinnunen 2021-09-24 01:46:31 PDT
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.
Comment 10 Kimmo Kinnunen 2021-09-24 01:53:59 PDT
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?
Comment 11 Wenson Hsieh 2021-09-24 08:58:58 PDT
(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.
Comment 12 Kimmo Kinnunen 2021-09-24 09:00:37 PDT
(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
Comment 13 Eric Hutchison 2021-09-24 10:53:30 PDT
Created attachment 439163 [details]
Related Crash Log
Comment 14 Eric Hutchison 2021-09-24 11:02:02 PDT
Reverted r283024 for reason:

Causes slowdown and crash on EWS

Committed r283048 (242108@main): <https://commits.webkit.org/242108@main>
Comment 15 Wenson Hsieh 2021-09-24 12:14:33 PDT
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.
Comment 16 Wenson Hsieh 2021-09-24 15:59:22 PDT
(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();
```
Comment 17 Wenson Hsieh 2021-09-24 16:04:21 PDT
(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
Comment 18 Wenson Hsieh 2021-09-24 16:16:57 PDT Comment hidden (obsolete)
Comment 19 Wenson Hsieh 2021-09-24 16:44:59 PDT Comment hidden (obsolete)
Comment 20 Wenson Hsieh 2021-09-24 17:46:41 PDT Comment hidden (obsolete)
Comment 21 Wenson Hsieh 2021-09-24 18:42:39 PDT Comment hidden (obsolete)
Comment 22 Wenson Hsieh 2021-09-24 18:53:14 PDT Comment hidden (obsolete)
Comment 23 Wenson Hsieh 2021-09-24 22:24:27 PDT Comment hidden (obsolete)
Comment 24 Wenson Hsieh 2021-09-24 23:14:15 PDT Comment hidden (obsolete)
Comment 25 Wenson Hsieh 2021-10-04 21:01:43 PDT Comment hidden (obsolete)
Comment 26 Wenson Hsieh 2021-10-05 08:41:56 PDT Comment hidden (obsolete)
Comment 27 Wenson Hsieh 2021-10-05 09:03:24 PDT Comment hidden (obsolete)
Comment 28 Wenson Hsieh 2021-10-05 11:09:42 PDT Comment hidden (obsolete)
Comment 29 Wenson Hsieh 2021-10-05 11:13:22 PDT
(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>.
Comment 30 Michael Catanzaro 2021-10-05 11:35:58 PDT
(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
Comment 31 Wenson Hsieh 2021-10-05 11:51:46 PDT Comment hidden (obsolete)
Comment 32 Wenson Hsieh 2021-10-05 12:56:25 PDT Comment hidden (obsolete)
Comment 33 Wenson Hsieh 2021-10-05 13:08:57 PDT
(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.
Comment 34 Wenson Hsieh 2021-10-05 14:13:14 PDT Comment hidden (obsolete)
Comment 35 Wenson Hsieh 2021-10-05 15:28:43 PDT Comment hidden (obsolete)
Comment 36 Wenson Hsieh 2021-10-05 15:45:04 PDT Comment hidden (obsolete)
Comment 37 Wenson Hsieh 2021-10-05 16:14:01 PDT Comment hidden (obsolete)
Comment 38 Wenson Hsieh 2021-10-05 17:00:07 PDT Comment hidden (obsolete)
Comment 39 Wenson Hsieh 2021-10-05 17:22:09 PDT Comment hidden (obsolete)
Comment 40 Wenson Hsieh 2021-10-05 17:23:29 PDT Comment hidden (obsolete)
Comment 41 Wenson Hsieh 2021-10-05 17:24:06 PDT Comment hidden (obsolete)
Comment 42 Wenson Hsieh 2021-10-05 17:27:31 PDT
Created attachment 440309 [details]
Try to fix the WPE build again? (4)
Comment 43 Wenson Hsieh 2021-10-05 17:29:52 PDT Comment hidden (obsolete)
Comment 44 Wenson Hsieh 2021-10-05 17:34:10 PDT Comment hidden (obsolete)
Comment 45 Wenson Hsieh 2021-10-05 17:34:42 PDT Comment hidden (obsolete)
Comment 46 Wenson Hsieh 2021-10-05 17:35:29 PDT Comment hidden (obsolete)
Comment 47 Wenson Hsieh 2021-10-05 17:36:02 PDT Comment hidden (obsolete)
Comment 48 EWS 2021-10-06 07:56:41 PDT
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].
Comment 49 Wenson Hsieh 2021-10-06 07:57:34 PDT
(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.
Comment 50 Adrian Perez 2021-10-07 13:02:45 PDT
(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.