Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REGRESSION(258484@main): StreamClientConnection sends destination id in wrong format #8465

Conversation

kkinnunen-apple
Copy link
Contributor

@kkinnunen-apple kkinnunen-apple commented Jan 10, 2023

ce70a7f

REGRESSION(258484@main): StreamClientConnection sends destination id in wrong format
https://bugs.webkit.org/show_bug.cgi?id=250386
rdar://104076868

Reviewed by Alex Christensen.

258484@main would change the IPC message destination ID to UInt128.
The commit changed also stream IPC destinations as UInt128.

Stream{Client,Server}Connection public API has only ObjectIdentifier<T> instances
as the destination ID. These are uint64_t. Thus sending UInt128 when the
input is only uint64_t is redundant.

Revert back to uint64_t. The stream IPC memory format expects certain layout and alignment
for messages and their fields. The UInt128 change didn't contain these.

It is better to make the 128 bit change in isolation, should the need arise. Given
that 64 bits is relatively big id space, try to preserve the perf benefit of using
this smaller space until needed.

* Source/WebKit/Platform/IPC/StreamClientConnection.h:
(IPC::StreamClientConnection::trySendDestinationIDIfNeeded):
* Source/WebKit/Platform/IPC/StreamServerConnection.cpp:
(IPC::StreamServerConnection::startReceivingMessages):
(IPC::StreamServerConnection::stopReceivingMessages):
(IPC::StreamServerConnection::processSetStreamDestinationID):
* Source/WebKit/Platform/IPC/StreamServerConnection.h:
* Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:
(WebKit::IPCTestingAPI::JSIPCStreamClientConnection::prepareToSendOutOfStreamMessage):

Canonical link: https://commits.webkit.org/258782@main

7ec77d5

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2   πŸ§ͺ api-mac βœ… πŸ§ͺ gtk-wk2
  πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1   πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim

@kkinnunen-apple kkinnunen-apple self-assigned this Jan 10, 2023
@kkinnunen-apple kkinnunen-apple added the WebKit2 Bugs relating to the WebKit2 API layer label Jan 10, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 10, 2023
@kkinnunen-apple kkinnunen-apple removed the merging-blocked Applied to prevent a change from being merged label Jan 11, 2023
@kkinnunen-apple kkinnunen-apple force-pushed the ipc-stream-revert-128bit-destination-1 branch from c57abdb to 7ec77d5 Compare January 11, 2023 07:59
@kkinnunen-apple kkinnunen-apple added the merge-queue Applied to send a pull request to merge-queue label Jan 11, 2023
…in wrong format

https://bugs.webkit.org/show_bug.cgi?id=250386
rdar://104076868

Reviewed by Alex Christensen.

258484@main would change the IPC message destination ID to UInt128.
The commit changed also stream IPC destinations as UInt128.

Stream{Client,Server}Connection public API has only ObjectIdentifier<T> instances
as the destination ID. These are uint64_t. Thus sending UInt128 when the
input is only uint64_t is redundant.

Revert back to uint64_t. The stream IPC memory format expects certain layout and alignment
for messages and their fields. The UInt128 change didn't contain these.

It is better to make the 128 bit change in isolation, should the need arise. Given
that 64 bits is relatively big id space, try to preserve the perf benefit of using
this smaller space until needed.

* Source/WebKit/Platform/IPC/StreamClientConnection.h:
(IPC::StreamClientConnection::trySendDestinationIDIfNeeded):
* Source/WebKit/Platform/IPC/StreamServerConnection.cpp:
(IPC::StreamServerConnection::startReceivingMessages):
(IPC::StreamServerConnection::stopReceivingMessages):
(IPC::StreamServerConnection::processSetStreamDestinationID):
* Source/WebKit/Platform/IPC/StreamServerConnection.h:
* Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:
(WebKit::IPCTestingAPI::JSIPCStreamClientConnection::prepareToSendOutOfStreamMessage):

Canonical link: https://commits.webkit.org/258782@main
@webkit-commit-queue
Copy link
Collaborator

Committed 258782@main (ce70a7f): https://commits.webkit.org/258782@main

Reviewed commits have been landed. Closing PR #8465 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit ce70a7f into WebKit:main Jan 11, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit2 Bugs relating to the WebKit2 API layer
Projects
None yet
5 participants