Bug 237730 – Allow modification of content of StreamClientConnection.streamBuffer in IPCTestingAPI

• Component: WebKit2
• Status: RESOLVED FIXED

[Simon Lewis]

When the IPCTestingAPI is enabled we can access the streamBuffer object of a StreamClientConnection:

streamConnection = IPC.createStreamClientConnection('GPU', 0x2000);
streamConnection.streamBuffer

However there are currently no methods exposed that allow for reading or writing to the memory content of the StreamBuffer.

[Simon Lewis]

rdar://problem/89676460

[Simon Lewis]

Created attachment 454413 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 454413 [details]
Patch

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

looks great, some maintenance improvement aspects to consider

> Source/WebKit/Platform/IPC/StreamClientConnection.h:81
> +    friend class WebKit::IPCTestingAPI::JSIPCStreamConnectionBuffer;

could you instead add:
StreamConnectionBuffer& bufferForTesting()

for StreamConnectionBuffer, add function:
Span<uint8_t> headerForTesting()

This way if StreamConnectionBuffer changes a bit, it doesn't neccarily need knowing that the ipc testing .cpp needs change.
This way the access chain is not so long and

> Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:909
> +    uint8_t* data = static_cast<uint8_t*>(jsStreamBuffer->m_streamConnection->m_streamConnection.m_buffer.m_sharedMemory->data());

so this brittle access chain to multiple internal structures we want to break with controlled ...ForTesting() functions.
(this enables the developers to understand that private variables are accessed with ForTesting methods. Understanding  and remembering that via friend declarations is a bit harder)

> LayoutTests/ipc/stream-buffer-read-write.html:17
> +    let initialData = new Uint8Array(streamBuffer.readBytes(0, 32));

I suggest add two modalities:
modify the header
modify the data

this way if the header changes position or is detached from the data area, all the tests related to corrupting the header will still be valid. it's also easier to understand what the test does.

[Simon Lewis]

Created attachment 454753 [details]
Patch

[Simon Lewis]

Created attachment 454773 [details]
Patch

[Simon Lewis]

Created attachment 454796 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 454796 [details]
Patch

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

Looks great still.
Feel free to consider the suggestion. If you do the change, you can also replace the "Reviewed by OOOPS" with Reviewed by Kimmo Kinnunen", and set the cq+ to commit.
for such patch, you can upload it by doing:

EDITOR=true tools/Scripts/webkit-patch upload -g HEAD   --no-confirm --no-review -m "For landing" --check-oops

> Source/WebKit/Platform/IPC/StreamConnectionBuffer.cpp:110
> +    return { static_cast<uint8_t*>((uint8_t*)m_sharedMemory->data() + headerSize()), m_sharedMemory->size() - headerSize() };

this contains a c-style cast.
I think you could write this as

  return { data(), dataSize() };

or if you want to be pedantic about possibly stuffing stuff into the (potential) page rounding leftover space and test that it's not used wrong:

 return { data(), m_sharedMemory->size() - headerSize() };

[Simon Lewis]

Created attachment 454988 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 454988 [details]
Patch

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

> Source/WebKit/Platform/IPC/StreamConnectionBuffer.cpp:105
> +    return { static_cast<uint8_t*>(m_sharedMemory->data()), m_sharedMemory->size() - headerSize() };

Hmm, I may have missed this one:
this should be just headerSize(), right?
return { static_cast<uint8_t*>(m_sharedMemory->data()), headerSize() };

[Simon Lewis]

Created attachment 454995 [details]
Patch

[EWS]

Committed r291428 (248554@main): <https://commits.webkit.org/248554@main>

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