Bug 237730

Created attachment 454413 [details] Patch

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.

Created attachment 454753 [details] Patch

Created attachment 454773 [details] Patch

Created attachment 454796 [details] Patch

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() };

Created attachment 454988 [details] Patch

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() };

Created attachment 454995 [details] Patch

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].