Skip to content

Conversation

kkinnunen-apple
Copy link
Contributor

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

e6b6765

New test: [iOS macOS] TestIPC.StreamConnectionTest.SendAsyncReplyMessage is timing out
https://bugs.webkit.org/show_bug.cgi?id=249780
rdar://103640558

Reviewed by Matt Woodrow.

The test would time out mainly because the communication semaphores were not
set up between client and server connection.
If the server would be slow enough, the initial processing wakeup due to
starting message listening would make the test pass.
If the server would be fast enough, the initial processing wakeup would
not process all messages that the test would send, and thus it would
time out. Fix by setting the semaphores, so new messages will wake up
the server work queue.

Secondarily make the test more consistent by running all the server
related code in the server work queue.

The first point needs IPCSemaphore to have copy operations, so implement
these. Add tests for IPCSemaphore, test also the new operations.
These semaphores are typically sent as part of the initialization message
exchange between client and server (CreateXXX, DidCreateXXX messages).
The unit tests run in single process, there is no message exchange
to setup the connection.

The second point needs a fix for StreamConnectionWorkQueue, where
the tasks that would be run during shutdown would crash if the
tasks would assert that the task is run on correct queue. Fix by
not removing the m_processingThread reference until the thread
has exited. Add a test for this.

* Source/WebKit/Platform/IPC/StreamConnectionWorkQueue.cpp:
(IPC::StreamConnectionWorkQueue::stopAndWaitForCompletion):
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/IPC/IPCTestUtilities.h:
(TestWebKitAPI::copyViaEncoder):
* Tools/TestWebKitAPI/Tests/IPC/StreamConnectionTests.cpp:
(TestWebKitAPI::StreamConnectionTest::localReferenceBarrier):
(TestWebKitAPI::StreamConnectionTest::serverQueue):
(TestWebKitAPI::TEST_F):
(TestWebKitAPI::StreamConnectionTest::StreamConnectionTest): Deleted.
* Tools/TestWebKitAPI/Tests/IPC/StreamConnectionWorkQueueTests.cpp: Added.
(TestWebKitAPI::TEST_F):

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

e436a82

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 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 requested review from a team and cdumez as code owners January 12, 2023 10:12
@kkinnunen-apple kkinnunen-apple self-assigned this Jan 12, 2023
@kkinnunen-apple kkinnunen-apple added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Jan 12, 2023
@kkinnunen-apple kkinnunen-apple requested review from achristensen07 and zdobersek and removed request for cdumez January 12, 2023 10:15
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 12, 2023
@zdobersek
Copy link
Contributor

diff --git a/Source/WebKit/Platform/IPC/unix/IPCSemaphoreUnix.cpp b/Source/WebKit/Platform/IPC/unix/IPCSemaphoreUnix.cpp
index 1aaa88d8bba9..b7af4d381845 100644
--- a/Source/WebKit/Platform/IPC/unix/IPCSemaphoreUnix.cpp
+++ b/Source/WebKit/Platform/IPC/unix/IPCSemaphoreUnix.cpp
@@ -50,10 +50,19 @@ Semaphore::Semaphore(UnixFileDescriptor&& fd)
 { }
 
 Semaphore::Semaphore(Semaphore&&) = default;
-Semaphore::Semaphore(const Semaphore&) = default;
-Semaphore::~Semaphore() = default;
 Semaphore& Semaphore::operator=(Semaphore&&) = default;
-Semaphore& Semaphore::operator=(const Semaphore&) = default;
+
+Semaphore::Semaphore(const Semaphore& o)
+    : m_fd(o.m_fd.duplicate())
+{ }
+
+Semaphore& Semaphore::operator=(const Semaphore& o)
+{
+    m_fd = o.m_fd.duplicate();
+    return *this;
+}
+
+Semaphore::~Semaphore() = default;
 
 void Semaphore::signal()
 {

@kkinnunen-apple kkinnunen-apple removed the merging-blocked Applied to prevent a change from being merged label Jan 12, 2023
@kkinnunen-apple kkinnunen-apple force-pushed the ipc-stream-tests-consistent-connections-1 branch from b1b7344 to 3f2dd0d Compare January 12, 2023 20:22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you just use this to create same-process copies of the Semaphore for the test code, and then not need to add the copy constructor?

It seems fine either way though, to support both in-process copies as well as IPC ones, just adds a bit more code that's only used for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you just use this to create same-process copies of the Semaphore for the test code, and then not need to add the copy constructor?

While contrary to the goal of bringing order to (my) world, I think your suggestion is unfortunately the better approach.

@kkinnunen-apple kkinnunen-apple force-pushed the ipc-stream-tests-consistent-connections-1 branch from 3f2dd0d to 55bb99e Compare January 13, 2023 07:21
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 13, 2023
@kkinnunen-apple kkinnunen-apple removed the merging-blocked Applied to prevent a change from being merged label Jan 13, 2023
@kkinnunen-apple kkinnunen-apple force-pushed the ipc-stream-tests-consistent-connections-1 branch from 55bb99e to 174aca5 Compare January 13, 2023 07:39
@kkinnunen-apple kkinnunen-apple added merge-queue Applied to send a pull request to merge-queue and removed merge-queue Applied to send a pull request to merge-queue labels Jan 14, 2023
@kkinnunen-apple kkinnunen-apple force-pushed the ipc-stream-tests-consistent-connections-1 branch from 174aca5 to e436a82 Compare January 14, 2023 09:18
@kkinnunen-apple kkinnunen-apple added the merge-queue Applied to send a pull request to merge-queue label Jan 14, 2023
…age is timing out

https://bugs.webkit.org/show_bug.cgi?id=249780
rdar://103640558

Reviewed by Matt Woodrow.

The test would time out mainly because the communication semaphores were not
set up between client and server connection.
If the server would be slow enough, the initial processing wakeup due to
starting message listening would make the test pass.
If the server would be fast enough, the initial processing wakeup would
not process all messages that the test would send, and thus it would
time out. Fix by setting the semaphores, so new messages will wake up
the server work queue.

Secondarily make the test more consistent by running all the server
related code in the server work queue.

The first point needs IPCSemaphore to have copy operations, so implement
these. Add tests for IPCSemaphore, test also the new operations.
These semaphores are typically sent as part of the initialization message
exchange between client and server (CreateXXX, DidCreateXXX messages).
The unit tests run in single process, there is no message exchange
to setup the connection.

The second point needs a fix for StreamConnectionWorkQueue, where
the tasks that would be run during shutdown would crash if the
tasks would assert that the task is run on correct queue. Fix by
not removing the m_processingThread reference until the thread
has exited. Add a test for this.

* Source/WebKit/Platform/IPC/StreamConnectionWorkQueue.cpp:
(IPC::StreamConnectionWorkQueue::stopAndWaitForCompletion):
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/IPC/IPCTestUtilities.h:
(TestWebKitAPI::copyViaEncoder):
* Tools/TestWebKitAPI/Tests/IPC/StreamConnectionTests.cpp:
(TestWebKitAPI::StreamConnectionTest::localReferenceBarrier):
(TestWebKitAPI::StreamConnectionTest::serverQueue):
(TestWebKitAPI::TEST_F):
(TestWebKitAPI::StreamConnectionTest::StreamConnectionTest): Deleted.
* Tools/TestWebKitAPI/Tests/IPC/StreamConnectionWorkQueueTests.cpp: Added.
(TestWebKitAPI::TEST_F):

Canonical link: https://commits.webkit.org/258913@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the ipc-stream-tests-consistent-connections-1 branch from e436a82 to e6b6765 Compare January 14, 2023 15:45
@webkit-commit-queue
Copy link
Collaborator

Committed 258913@main (e6b6765): https://commits.webkit.org/258913@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit e6b6765 into WebKit:main Jan 14, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New Bugs Unclassified bugs are placed in this component until the correct component can be determined.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants