From d9f47951394778e8fc1a8e5bc16f3ce3b71e5ab0 Mon Sep 17 00:00:00 2001 From: Jelle Foks Date: Wed, 13 Nov 2024 16:09:10 -0800 Subject: [PATCH 1/5] Fix socket watching read and write conflict. --- .../message_loop/message_pump_io_starboard.cc | 86 ++++++++++++++----- base/message_loop/message_pump_io_starboard.h | 13 ++- .../message_pump_io_starboard_unittest.cc | 26 +++--- base/task/current_thread.cc | 10 ++- base/task/current_thread.h | 9 +- net/socket/tcp_socket_starboard.cc | 48 ++++++----- net/socket/tcp_socket_starboard.h | 3 +- net/socket/udp_socket_starboard.cc | 54 ++++++------ net/socket/udp_socket_starboard.h | 8 +- 9 files changed, 154 insertions(+), 103 deletions(-) diff --git a/base/message_loop/message_pump_io_starboard.cc b/base/message_loop/message_pump_io_starboard.cc index 714012b8c455..ef33f601344d 100644 --- a/base/message_loop/message_pump_io_starboard.cc +++ b/base/message_loop/message_pump_io_starboard.cc @@ -42,24 +42,17 @@ MessagePumpIOStarboard::SocketWatcher::~SocketWatcher() { bool MessagePumpIOStarboard::SocketWatcher::StopWatchingSocket() { watcher_ = nullptr; interests_ = kSbSocketWaiterInterestNone; - if (!SbSocketIsValid(socket_)) { - pump_ = nullptr; - // If this watcher is not watching anything, no-op and return success. - return true; - } SbSocket socket = Release(); bool result = true; if (SbSocketIsValid(socket)) { DCHECK(pump_); -#if defined(STARBOARD) // This may get called multiple times from TCPSocketStarboard. if (pump_) { - result = pump_->StopWatching(socket); + result = pump_->UnregisterInterest( + socket, kSbSocketWaiterInterestRead || kSbSocketWaiterInterestWrite, + this); } -#else - result = pump_->StopWatching(socket); -#endif } pump_ = nullptr; return result; @@ -109,27 +102,75 @@ MessagePumpIOStarboard::~MessagePumpIOStarboard() { SbSocketWaiterDestroy(waiter_); } +bool MessagePumpIOStarboard::UnregisterInterest(SbSocket socket, + int dropped_interests, + SocketWatcher* controller) { + DCHECK(SbSocketIsValid(socket)); + DCHECK(controller); + DCHECK(dropped_interests == kSbSocketWaiterInterestRead || + dropped_interests == kSbSocketWaiterInterestWrite || + dropped_interests == + (kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite)); + DCHECK_CALLED_ON_VALID_THREAD(watch_socket_caller_checker_); + + // Make sure we don't pick up any funky internal masks. + int old_interest_mask = + controller->interests() & + (kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite); + int interests = old_interest_mask & (~dropped_interests); + if (interests == old_interest_mask) { + // Interests didn't change, return. + return true; + } + + SbSocket old_socket = controller->Release(); + if (SbSocketIsValid(old_socket)) { + // It's illegal to use this function to listen on 2 separate fds with the + // same |controller|. + if (old_socket != socket) { + NOTREACHED() << "Sockets don't match" << old_socket << "!=" << socket; + return false; + } + + // Must disarm the event before we can reuse it. + SbSocketWaiterRemove(waiter_, old_socket); + } else { + interests = kSbSocketWaiterInterestNone; + } + + if (!SbSocketIsValid(socket)) { + NOTREACHED() << "Invalid socket" << socket; + return false; + } + + if (interests) { + // Set current interest mask and waiter for this event. + if (!SbSocketWaiterAdd(waiter_, socket, controller, + OnSocketWaiterNotification, interests, + controller->persistent())) { + return false; + } + controller->Init(socket, controller->persistent()); + } + return true; +} + bool MessagePumpIOStarboard::Watch(SbSocket socket, bool persistent, - int mode, + int interests, SocketWatcher* controller, Watcher* delegate) { DCHECK(SbSocketIsValid(socket)); DCHECK(controller); DCHECK(delegate); - DCHECK(mode == WATCH_READ || mode == WATCH_WRITE || mode == WATCH_READ_WRITE); + DCHECK(interests == kSbSocketWaiterInterestRead || + interests == kSbSocketWaiterInterestWrite || + interests == + (kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite)); // Watch should be called on the pump thread. It is not threadsafe, and your // watcher may never be registered. DCHECK_CALLED_ON_VALID_THREAD(watch_socket_caller_checker_); - int interests = kSbSocketWaiterInterestNone; - if (mode & WATCH_READ) { - interests |= kSbSocketWaiterInterestRead; - } - if (mode & WATCH_WRITE) { - interests |= kSbSocketWaiterInterestWrite; - } - SbSocket old_socket = controller->Release(); if (SbSocketIsValid(old_socket)) { // It's illegal to use this function to listen on 2 separate fds with the @@ -151,6 +192,11 @@ bool MessagePumpIOStarboard::Watch(SbSocket socket, SbSocketWaiterRemove(waiter_, old_socket); } + if (!SbSocketIsValid(socket)) { + NOTREACHED() << "Invalid socket" << socket; + return false; + } + // Set current interest mask and waiter for this event. if (!SbSocketWaiterAdd(waiter_, socket, controller, OnSocketWaiterNotification, interests, persistent)) { diff --git a/base/message_loop/message_pump_io_starboard.h b/base/message_loop/message_pump_io_starboard.h index b7d807dec914..132d53addffc 100644 --- a/base/message_loop/message_pump_io_starboard.h +++ b/base/message_loop/message_pump_io_starboard.h @@ -102,12 +102,6 @@ class BASE_EXPORT MessagePumpIOStarboard : public MessagePump { base::WeakPtrFactory weak_factory_; }; - enum Mode { - WATCH_READ = 1 << 0, - WATCH_WRITE = 1 << 1, - WATCH_READ_WRITE = WATCH_READ | WATCH_WRITE - }; - MessagePumpIOStarboard(); virtual ~MessagePumpIOStarboard(); @@ -125,10 +119,15 @@ class BASE_EXPORT MessagePumpIOStarboard : public MessagePump { // success. Must be called on the same thread the message_pump is running on. bool Watch(SbSocket socket, bool persistent, - int mode, + int interests, SocketWatcher* controller, Watcher* delegate); + // Removes an interest from a socket, and stops watching the socket if needed. + bool UnregisterInterest(SbSocket socket, + int dropped_interests, + SocketWatcher* controller); + // Stops watching the socket. bool StopWatching(SbSocket socket); diff --git a/base/message_loop/message_pump_io_starboard_unittest.cc b/base/message_loop/message_pump_io_starboard_unittest.cc index c6231f70d882..65ba8151043f 100644 --- a/base/message_loop/message_pump_io_starboard_unittest.cc +++ b/base/message_loop/message_pump_io_starboard_unittest.cc @@ -137,10 +137,9 @@ TEST_F(MessagePumpIOStarboardTest, DISABLED_DeleteWatcher) { std::make_unique(FROM_HERE)); std::unique_ptr pump = CreateMessagePump(); pump->Watch(socket(), - /*persistent=*/false, - MessagePumpIOStarboard::WATCH_READ_WRITE, - delegate.controller(), - &delegate); + /*persistent=*/false, + (kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite), + delegate.controller(), &delegate); SimulateIOEvent(delegate.controller()); } @@ -165,10 +164,9 @@ TEST_F(MessagePumpIOStarboardTest, DISABLED_StopWatcher) { MessagePumpIOStarboard::SocketWatcher controller(FROM_HERE); StopWatcher delegate(&controller); pump->Watch(socket(), - /*persistent=*/false, - MessagePumpIOStarboard::WATCH_READ_WRITE, - &controller, - &delegate); + /*persistent=*/false, + (kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite), + &controller, &delegate); SimulateIOEvent(&controller); } @@ -202,10 +200,8 @@ TEST_F(MessagePumpIOStarboardTest, DISABLED_NestedPumpWatcher) { std::unique_ptr pump = CreateMessagePump(); MessagePumpIOStarboard::SocketWatcher controller(FROM_HERE); pump->Watch(socket(), - /*persistent=*/false, - MessagePumpIOStarboard::WATCH_READ, - &controller, - &delegate); + /*persistent=*/false, kSbSocketWaiterInterestRead, &controller, + &delegate); SimulateIOEvent(&controller); } @@ -253,10 +249,8 @@ TEST_F(MessagePumpIOStarboardTest, DISABLED_QuitWatcher) { // Tell the pump to watch the pipe. pump->Watch(socket(), - /*persistent=*/false, - MessagePumpIOStarboard::WATCH_READ, - &controller, - &delegate); + /*persistent=*/false, kSbSocketWaiterInterestRead, &controller, + &delegate); // Make the IO thread wait for |event| before writing to pipefds[1]. const char buf = 0; diff --git a/base/task/current_thread.cc b/base/task/current_thread.cc index 673e2028d4a2..d7e572fec80f 100644 --- a/base/task/current_thread.cc +++ b/base/task/current_thread.cc @@ -214,11 +214,17 @@ MessagePumpForIO* CurrentIOThread::GetMessagePumpForIO() const { #if defined(STARBOARD) bool CurrentIOThread::Watch(SbSocket socket, bool persistent, - int mode, + SbSocketWaiterInterest interests, SocketWatcher* controller, Watcher* delegate) { return static_cast(GetMessagePumpForIO()) - ->Watch(socket, persistent, mode, controller, delegate); + ->Watch(socket, persistent, interests, controller, delegate); +} +bool CurrentIOThread::UnregisterInterest(SbSocket socket, + int dropped_interests, + SocketWatcher* controller) { + return static_cast(GetMessagePumpForIO()) + ->UnregisterInterest(socket, dropped_interests, controller); } #elif BUILDFLAG(IS_WIN) HRESULT CurrentIOThread::RegisterIOHandler( diff --git a/base/task/current_thread.h b/base/task/current_thread.h index 5106c5e9fd71..c39eb38669bd 100644 --- a/base/task/current_thread.h +++ b/base/task/current_thread.h @@ -275,15 +275,14 @@ class BASE_EXPORT CurrentIOThread : public CurrentThread { typedef base::MessagePumpIOStarboard::SocketWatcher SocketWatcher; typedef base::MessagePumpIOStarboard::IOObserver IOObserver; - enum Mode{WATCH_READ = base::MessagePumpIOStarboard::WATCH_READ, - WATCH_WRITE = base::MessagePumpIOStarboard::WATCH_WRITE, - WATCH_READ_WRITE = base::MessagePumpIOStarboard::WATCH_READ_WRITE}; - bool Watch(SbSocket socket, bool persistent, - int mode, + SbSocketWaiterInterest interests, SocketWatcher* controller, Watcher* delegate); + bool UnregisterInterest(SbSocket socket, + int dropped_interests, + SocketWatcher* controller); #elif BUILDFLAG(IS_WIN) // Please see MessagePumpWin for definitions of these methods. HRESULT RegisterIOHandler(HANDLE file, MessagePumpForIO::IOHandler* handler); diff --git a/net/socket/tcp_socket_starboard.cc b/net/socket/tcp_socket_starboard.cc index 0fdd752ae130..920bc13f69e9 100644 --- a/net/socket/tcp_socket_starboard.cc +++ b/net/socket/tcp_socket_starboard.cc @@ -139,9 +139,9 @@ int TCPSocketStarboard::Accept(std::unique_ptr* socket, int result = AcceptInternal(socket, address); if (result == ERR_IO_PENDING) { - if (!base::CurrentIOThread::Get()->Watch( - socket_, true, base::MessagePumpIOStarboard::WATCH_READ, - &socket_watcher_, this)) { + if (!base::CurrentIOThread::Get()->Watch(socket_, true, + kSbSocketWaiterInterestRead, + &socket_watcher_, this)) { DLOG(ERROR) << "WatchSocket failed on read"; return MapLastSocketError(socket_); } @@ -252,7 +252,9 @@ void TCPSocketStarboard::Close() { } void TCPSocketStarboard::StopWatchingAndCleanUp() { - bool ok = socket_watcher_.StopWatchingSocket(); + bool ok = base::CurrentIOThread::Get()->UnregisterInterest( + socket_, kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite, + &socket_watcher_); DCHECK(ok); if (!accept_callback_.is_null()) { @@ -293,7 +295,7 @@ void TCPSocketStarboard::OnSocketReadyToRead(SbSocket socket) { } else if (read_pending()) { DidCompleteRead(); } else { - ClearWatcherIfOperationsNotPending(); + ClearReadWatcherIfOperationsNotPending(); } } @@ -345,8 +347,7 @@ int TCPSocketStarboard::Connect(const IPEndPoint& address, // When it is ready to write, it will have connected. base::CurrentIOThread::Get()->Watch( - socket_, true, base::MessagePumpIOStarboard::WATCH_WRITE, - &socket_watcher_, this); + socket_, true, kSbSocketWaiterInterestWrite, &socket_watcher_, this); return ERR_IO_PENDING; } @@ -374,7 +375,7 @@ void TCPSocketStarboard::DidCompleteConnect() { waiting_connect_ = false; CompletionOnceCallback callback = std::move(write_callback_); write_callback_.Reset(); - ClearWatcherIfOperationsNotPending(); + ClearReadWatcherIfOperationsNotPending(); std::move(callback).Run(HandleConnectCompleted(rv)); } @@ -460,9 +461,8 @@ int TCPSocketStarboard::ReadIfReady(IOBuffer* buf, } read_if_ready_callback_ = std::move(callback); - base::CurrentIOThread::Get()->Watch( - socket_, true, base::MessagePumpIOStarboard::WATCH_READ, - &socket_watcher_, this); + base::CurrentIOThread::Get()->Watch( + socket_, true, kSbSocketWaiterInterestRead, &socket_watcher_, this); return rv; } @@ -470,7 +470,8 @@ int TCPSocketStarboard::ReadIfReady(IOBuffer* buf, int TCPSocketStarboard::CancelReadIfReady() { DCHECK(read_if_ready_callback_); - bool ok = socket_watcher_.StopWatchingSocket(); + bool ok = base::CurrentIOThread::Get()->UnregisterInterest( + socket_, kSbSocketWaiterInterestRead, &socket_watcher_); DCHECK(ok); read_if_ready_callback_.Reset(); @@ -522,7 +523,7 @@ void TCPSocketStarboard::DidCompleteRead() { CompletionOnceCallback callback = std::move(read_if_ready_callback_); read_if_ready_callback_.Reset(); - ClearWatcherIfOperationsNotPending(); + ClearReadWatcherIfOperationsNotPending(); std::move(callback).Run(OK); } @@ -545,8 +546,7 @@ int TCPSocketStarboard::Write( write_buf_len_ = buf_len; write_callback_ = std::move(callback); base::CurrentIOThread::Get()->Watch( - socket_, true, base::MessagePumpIOStarboard::WATCH_WRITE, - &socket_watcher_, this); + socket_, true, kSbSocketWaiterInterestWrite, &socket_watcher_, this); } return rv; @@ -586,7 +586,7 @@ void TCPSocketStarboard::DidCompleteWrite() { CompletionOnceCallback callback = std::move(write_callback_); write_callback_.Reset(); - ClearWatcherIfOperationsNotPending(); + ClearWriteWatcherIfOperationsNotPending(); std::move(callback).Run(rv); } } @@ -667,10 +667,18 @@ void TCPSocketStarboard::ApplySocketTag(const SocketTag& tag) { tag_ = tag; } -void TCPSocketStarboard::ClearWatcherIfOperationsNotPending() { - if (!read_pending() && !write_pending() && !accept_pending() && - !connect_pending()) { - bool ok = socket_watcher_.StopWatchingSocket(); +void TCPSocketStarboard::ClearReadWatcherIfOperationsNotPending() { + if (!read_pending() && !accept_pending() && !connect_pending()) { + bool ok = base::CurrentIOThread::Get()->UnregisterInterest( + socket_, kSbSocketWaiterInterestRead, &socket_watcher_); + DCHECK(ok); + } +} + +void TCPSocketStarboard::ClearWriteWatcherIfOperationsNotPending() { + if (!write_pending()) { + bool ok = base::CurrentIOThread::Get()->UnregisterInterest( + socket_, kSbSocketWaiterInterestWrite, &socket_watcher_); DCHECK(ok); } } diff --git a/net/socket/tcp_socket_starboard.h b/net/socket/tcp_socket_starboard.h index 876dac02d8a5..f23cc4ad2417 100644 --- a/net/socket/tcp_socket_starboard.h +++ b/net/socket/tcp_socket_starboard.h @@ -158,7 +158,8 @@ class NET_EXPORT TCPSocketStarboard : public base::MessagePumpIOStarboard::Watch int DoWrite(IOBuffer* buf, int buf_len); void StopWatchingAndCleanUp(); - void ClearWatcherIfOperationsNotPending(); + void ClearReadWatcherIfOperationsNotPending(); + void ClearWriteWatcherIfOperationsNotPending(); bool read_pending() const { return !read_if_ready_callback_.is_null(); } bool write_pending() const { diff --git a/net/socket/udp_socket_starboard.cc b/net/socket/udp_socket_starboard.cc index 6cf1f737f691..e3bd7a4c1b41 100644 --- a/net/socket/udp_socket_starboard.cc +++ b/net/socket/udp_socket_starboard.cc @@ -132,7 +132,9 @@ void UDPSocketStarboard::Close() { write_callback_.Reset(); send_to_address_.reset(); - bool ok = socket_watcher_.StopWatchingSocket(); + bool ok = base::CurrentIOThread::Get()->UnregisterInterest( + socket_, kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite, + &socket_watcher_); DCHECK(ok); is_connected_ = false; @@ -224,8 +226,7 @@ int UDPSocketStarboard::ReadMultiplePackets(Socket::ReadPacketResults* results, } if (!base::CurrentIOThread::Get()->Watch( - socket_, true, base::MessagePumpIOStarboard::WATCH_READ, - &socket_watcher_, this)) { + socket_, true, kSbSocketWaiterInterestRead, &socket_watcher_, this)) { PLOG(ERROR) << "WatchSocket failed on read"; Error result = MapLastSocketError(socket_); if (result == ERR_IO_PENDING) { @@ -266,8 +267,7 @@ int UDPSocketStarboard::RecvFrom(IOBuffer* buf, return nread; if (!base::CurrentIOThread::Get()->Watch( - socket_, true, base::MessagePumpIOStarboard::WATCH_READ, - &socket_watcher_, this)) { + socket_, true, kSbSocketWaiterInterestRead, &socket_watcher_, this)) { PLOG(ERROR) << "WatchSocket failed on read"; Error result = MapLastSocketError(socket_); if (result == ERR_IO_PENDING) { @@ -315,9 +315,9 @@ int UDPSocketStarboard::SendToOrWrite(IOBuffer* buf, if (result != ERR_IO_PENDING) return result; - if (!base::CurrentIOThread::Get()->Watch( - socket_, true, base::MessagePumpIOStarboard::WATCH_WRITE, - &socket_watcher_, this)) { + if (!base::CurrentIOThread::Get()->Watch(socket_, true, + kSbSocketWaiterInterestWrite, + &socket_watcher_, this)) { DVLOG(1) << "Watch failed on write, error " << SbSocketGetLastError(socket_); Error result = MapLastSocketError(socket_); @@ -476,7 +476,7 @@ void UDPSocketStarboard::WriteAsyncWatcher::OnSocketReadyToWrite( SbSocket /*socket*/) { DVLOG(1) << __func__ << " queue " << socket_->pending_writes_.size() << " out of " << socket_->write_async_outstanding_ << " total"; - socket_->StopWatchingSocket(); + socket_->StopWatchingSocketForWriting(); socket_->FlushPending(); } @@ -504,7 +504,7 @@ void UDPSocketStarboard::DidCompleteRead() { read_buf_ = NULL; read_buf_len_ = 0; recv_from_address_ = NULL; - InternalStopWatchingSocket(); + StopWatchingSocketForReading(); DoReadCallback(result); } } @@ -513,7 +513,7 @@ void UDPSocketStarboard::DidCompleteMultiplePacketRead() { int result = InternalReadMultiplePackets(results_); if (result != ERR_IO_PENDING) { results_ = nullptr; - InternalStopWatchingSocket(); + StopWatchingSocketForReading(); DoReadCallback(result); } } @@ -543,7 +543,7 @@ void UDPSocketStarboard::DidCompleteWrite() { write_buf_ = NULL; write_buf_len_ = 0; send_to_address_.reset(); - InternalStopWatchingSocket(); + StopWatchingSocketForWriting(); DoWriteCallback(result); } } @@ -959,7 +959,7 @@ void UDPSocketStarboard::DidSendBuffers(SendResult send_result) { last_async_result_ = send_result.rv; if (last_async_result_ == ERR_IO_PENDING) { DVLOG(2) << __func__ << " WatchSocket start"; - if (!WatchSocket()) { + if (!WatchSocketForWriting()) { last_async_result_ = MapLastSocketError(socket_); DVLOG(1) << "WatchSocket failed on write, error: " << last_async_result_; LogWrite(last_async_result_, NULL, NULL); @@ -970,7 +970,7 @@ void UDPSocketStarboard::DidSendBuffers(SendResult send_result) { DVLOG(2) << __func__ << " WatchSocket stop: result " << ErrorToShortString(last_async_result_) << " pending_writes " << pending_writes_.size(); - StopWatchingSocket(); + StopWatchingSocketForWriting(); } DCHECK(last_async_result_ != ERR_IO_PENDING); @@ -1002,32 +1002,32 @@ void UDPSocketStarboard::SetMsgConfirm(bool confirm) { NOTIMPLEMENTED(); } -bool UDPSocketStarboard::WatchSocket() { +bool UDPSocketStarboard::WatchSocketForWriting() { if (write_async_watcher_->watching()) return true; - bool result = InternalWatchSocket(); + bool result = base::CurrentIOThread::Get()->Watch( + socket_, true, kSbSocketWaiterInterestWrite, &socket_watcher_, this); if (result) { write_async_watcher_->set_watching(true); } return result; } -void UDPSocketStarboard::StopWatchingSocket() { +void UDPSocketStarboard::StopWatchingSocketForWriting() { if (!write_async_watcher_->watching()) return; write_async_watcher_->set_watching(false); - InternalStopWatchingSocket(); -} - -bool UDPSocketStarboard::InternalWatchSocket() { - return base::CurrentIOThread::Get()->Watch( - socket_, true, base::MessagePumpIOStarboard::WATCH_WRITE, - &socket_watcher_, this); + if (!write_buf_) { + bool ok = base::CurrentIOThread::Get()->UnregisterInterest( + socket_, kSbSocketWaiterInterestWrite, &socket_watcher_); + DCHECK(ok); + } } -void UDPSocketStarboard::InternalStopWatchingSocket() { - if (!read_buf_ && !write_buf_ && !write_async_watcher_->watching()) { - bool ok = socket_watcher_.StopWatchingSocket(); +void UDPSocketStarboard::StopWatchingSocketForReading() { + if (!read_buf_) { + bool ok = base::CurrentIOThread::Get()->UnregisterInterest( + socket_, kSbSocketWaiterInterestRead, &socket_watcher_); DCHECK(ok); } } diff --git a/net/socket/udp_socket_starboard.h b/net/socket/udp_socket_starboard.h index 080d778ba9c5..ae4430c5cf95 100644 --- a/net/socket/udp_socket_starboard.h +++ b/net/socket/udp_socket_starboard.h @@ -361,9 +361,6 @@ class NET_EXPORT UDPSocketStarboard write_async_outstanding_ += increment; } - virtual bool InternalWatchSocket(); - virtual void InternalStopWatchingSocket(); - void SetWriteCallback(CompletionOnceCallback callback) { write_callback_ = std::move(callback); } @@ -385,8 +382,9 @@ class NET_EXPORT UDPSocketStarboard int InternalWriteAsync(CompletionOnceCallback callback, const NetworkTrafficAnnotationTag& traffic_annotation); - bool WatchSocket(); - void StopWatchingSocket(); + bool WatchSocketForWriting(); + void StopWatchingSocketForReading(); + void StopWatchingSocketForWriting(); void DoReadCallback(int rv); void DoWriteCallback(int rv); From 3567d189a13697ae9b8ce34c8509f90130362201 Mon Sep 17 00:00:00 2001 From: Jelle Foks Date: Thu, 8 Aug 2024 11:43:29 -0700 Subject: [PATCH 2/5] doNotStrip and profileable --- starboard/android/apk/app/build.gradle | 4 ++++ starboard/android/apk/app/src/app/AndroidManifest.xml | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/starboard/android/apk/app/build.gradle b/starboard/android/apk/app/build.gradle index 55ba2b964ea1..e8f1b24f93cc 100644 --- a/starboard/android/apk/app/build.gradle +++ b/starboard/android/apk/app/build.gradle @@ -178,6 +178,10 @@ android { } } packagingOptions { + doNotStrip "*/arm64-v8a/*.so" + doNotStrip "*/armeabi-v7a/*.so" + doNotStrip "*/x86/*.so" + doNotStrip "*/x86_64/*.so" jniLibs { // extractNativeLibs, which we set, based on evergreenCompatible, in the manifest file, // is deprecated. Gradle asks that useLegacyPackaging be set if extractNativeLibs is diff --git a/starboard/android/apk/app/src/app/AndroidManifest.xml b/starboard/android/apk/app/src/app/AndroidManifest.xml index 5fd31967bc0b..c34d24be5992 100644 --- a/starboard/android/apk/app/src/app/AndroidManifest.xml +++ b/starboard/android/apk/app/src/app/AndroidManifest.xml @@ -70,7 +70,8 @@ - + + From e4093f89007d3caf09ca4c0c890af3dae3446854 Mon Sep 17 00:00:00 2001 From: Jelle Foks Date: Wed, 13 Nov 2024 17:42:59 -0800 Subject: [PATCH 3/5] fix --- .../message_loop/message_pump_io_starboard.cc | 24 ++++++++------- base/message_loop/message_pump_io_starboard.h | 4 +++ .../message_pump_io_starboard_unittest.cc | 29 +++++++++++++------ net/socket/tcp_socket_starboard.cc | 14 ++++----- net/socket/udp_socket_starboard.cc | 11 +++---- 5 files changed, 46 insertions(+), 36 deletions(-) diff --git a/base/message_loop/message_pump_io_starboard.cc b/base/message_loop/message_pump_io_starboard.cc index ef33f601344d..04490a49cd78 100644 --- a/base/message_loop/message_pump_io_starboard.cc +++ b/base/message_loop/message_pump_io_starboard.cc @@ -39,25 +39,26 @@ MessagePumpIOStarboard::SocketWatcher::~SocketWatcher() { } } -bool MessagePumpIOStarboard::SocketWatcher::StopWatchingSocket() { - watcher_ = nullptr; - interests_ = kSbSocketWaiterInterestNone; - - SbSocket socket = Release(); +bool MessagePumpIOStarboard::SocketWatcher::UnregisterInterest(int interests) { bool result = true; - if (SbSocketIsValid(socket)) { - DCHECK(pump_); + if (SbSocketIsValid(socket_)) { // This may get called multiple times from TCPSocketStarboard. if (pump_) { - result = pump_->UnregisterInterest( - socket, kSbSocketWaiterInterestRead || kSbSocketWaiterInterestWrite, - this); + result = pump_->UnregisterInterest(socket_, interests, this); } } - pump_ = nullptr; + if (!interests_) { + pump_ = nullptr; + watcher_ = nullptr; + } return result; } +bool MessagePumpIOStarboard::SocketWatcher::StopWatchingSocket() { + return UnregisterInterest(kSbSocketWaiterInterestRead || + kSbSocketWaiterInterestWrite); +} + void MessagePumpIOStarboard::SocketWatcher::Init(SbSocket socket, bool persistent) { DCHECK(socket); @@ -137,6 +138,7 @@ bool MessagePumpIOStarboard::UnregisterInterest(SbSocket socket, } else { interests = kSbSocketWaiterInterestNone; } + controller->set_interests(interests); if (!SbSocketIsValid(socket)) { NOTREACHED() << "Invalid socket" << socket; diff --git a/base/message_loop/message_pump_io_starboard.h b/base/message_loop/message_pump_io_starboard.h index 132d53addffc..547f9b760cab 100644 --- a/base/message_loop/message_pump_io_starboard.h +++ b/base/message_loop/message_pump_io_starboard.h @@ -68,6 +68,10 @@ class BASE_EXPORT MessagePumpIOStarboard : public MessagePump { // NOTE: These methods aren't called StartWatching()/StopWatching() to avoid // confusion with the win32 ObjectWatcher class. + // Unregisters the interests for watching the socket, always safe to call. + // No-op if there's nothing to do. + bool UnregisterInterest(int interests); + // Stops watching the socket, always safe to call. No-op if there's nothing // to do. bool StopWatchingSocket(); diff --git a/base/message_loop/message_pump_io_starboard_unittest.cc b/base/message_loop/message_pump_io_starboard_unittest.cc index 65ba8151043f..e6e056387311 100644 --- a/base/message_loop/message_pump_io_starboard_unittest.cc +++ b/base/message_loop/message_pump_io_starboard_unittest.cc @@ -68,10 +68,21 @@ class MessagePumpIOStarboardTest : public testing::Test { } void SimulateIOEvent(MessagePumpIOStarboard::SocketWatcher* controller) { - MessagePumpIOStarboard::OnSocketWaiterNotification(nullptr, - nullptr, - controller, - (kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite)); + MessagePumpIOStarboard::OnSocketWaiterNotification( + nullptr, nullptr, controller, + (kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite)); + } + + void SimulateIOReadEvent(MessagePumpIOStarboard::SocketWatcher* controller) { + MessagePumpIOStarboard::OnSocketWaiterNotification( + nullptr, nullptr, controller, + kSbSocketWaiterInterestRead); + } + + void SimulateIOWriteEvent(MessagePumpIOStarboard::SocketWatcher* controller) { + MessagePumpIOStarboard::OnSocketWaiterNotification( + nullptr, nullptr, controller, + kSbSocketWaiterInterestWrite); } std::unique_ptr task_environment_; @@ -95,7 +106,7 @@ class StupidWatcher : public MessagePumpIOStarboard::Watcher { }; // Death tests not supported. -TEST_F(MessagePumpIOStarboardTest, DISABLED_QuitOutsideOfRun) { +TEST_F(MessagePumpIOStarboardTest, QuitOutsideOfRun) { std::unique_ptr pump = CreateMessagePump(); ASSERT_DCHECK_DEATH(pump->Quit()); } @@ -132,7 +143,7 @@ class DeleteWatcher : public BaseWatcher { }; // Fails on some platforms. -TEST_F(MessagePumpIOStarboardTest, DISABLED_DeleteWatcher) { +TEST_F(MessagePumpIOStarboardTest, DeleteWatcher) { DeleteWatcher delegate( std::make_unique(FROM_HERE)); std::unique_ptr pump = CreateMessagePump(); @@ -159,7 +170,7 @@ class StopWatcher : public BaseWatcher { }; // Fails on some platforms. -TEST_F(MessagePumpIOStarboardTest, DISABLED_StopWatcher) { +TEST_F(MessagePumpIOStarboardTest, StopWatcher) { std::unique_ptr pump = CreateMessagePump(); MessagePumpIOStarboard::SocketWatcher controller(FROM_HERE); StopWatcher delegate(&controller); @@ -195,7 +206,7 @@ class NestedPumpWatcher : public MessagePumpIOStarboard::Watcher { }; // Fails on some platforms. -TEST_F(MessagePumpIOStarboardTest, DISABLED_NestedPumpWatcher) { +TEST_F(MessagePumpIOStarboardTest, NestedPumpWatcher) { NestedPumpWatcher delegate; std::unique_ptr pump = CreateMessagePump(); MessagePumpIOStarboard::SocketWatcher controller(FROM_HERE); @@ -233,7 +244,7 @@ void WriteSocketWrapper(MessagePumpIOStarboard* pump, } // Fails on some platforms. -TEST_F(MessagePumpIOStarboardTest, DISABLED_QuitWatcher) { +TEST_F(MessagePumpIOStarboardTest, QuitWatcher) { // Delete the old TaskEnvironment so that we can manage our own one here. task_environment_.reset(); diff --git a/net/socket/tcp_socket_starboard.cc b/net/socket/tcp_socket_starboard.cc index 920bc13f69e9..73a97d32db9d 100644 --- a/net/socket/tcp_socket_starboard.cc +++ b/net/socket/tcp_socket_starboard.cc @@ -252,9 +252,8 @@ void TCPSocketStarboard::Close() { } void TCPSocketStarboard::StopWatchingAndCleanUp() { - bool ok = base::CurrentIOThread::Get()->UnregisterInterest( - socket_, kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite, - &socket_watcher_); + bool ok = socket_watcher_.UnregisterInterest(kSbSocketWaiterInterestRead | + kSbSocketWaiterInterestWrite); DCHECK(ok); if (!accept_callback_.is_null()) { @@ -470,8 +469,7 @@ int TCPSocketStarboard::ReadIfReady(IOBuffer* buf, int TCPSocketStarboard::CancelReadIfReady() { DCHECK(read_if_ready_callback_); - bool ok = base::CurrentIOThread::Get()->UnregisterInterest( - socket_, kSbSocketWaiterInterestRead, &socket_watcher_); + bool ok = socket_watcher_.UnregisterInterest(kSbSocketWaiterInterestRead); DCHECK(ok); read_if_ready_callback_.Reset(); @@ -669,16 +667,14 @@ void TCPSocketStarboard::ApplySocketTag(const SocketTag& tag) { void TCPSocketStarboard::ClearReadWatcherIfOperationsNotPending() { if (!read_pending() && !accept_pending() && !connect_pending()) { - bool ok = base::CurrentIOThread::Get()->UnregisterInterest( - socket_, kSbSocketWaiterInterestRead, &socket_watcher_); + bool ok = socket_watcher_.UnregisterInterest(kSbSocketWaiterInterestRead); DCHECK(ok); } } void TCPSocketStarboard::ClearWriteWatcherIfOperationsNotPending() { if (!write_pending()) { - bool ok = base::CurrentIOThread::Get()->UnregisterInterest( - socket_, kSbSocketWaiterInterestWrite, &socket_watcher_); + bool ok = socket_watcher_.UnregisterInterest(kSbSocketWaiterInterestWrite); DCHECK(ok); } } diff --git a/net/socket/udp_socket_starboard.cc b/net/socket/udp_socket_starboard.cc index e3bd7a4c1b41..1bfd609bcc0b 100644 --- a/net/socket/udp_socket_starboard.cc +++ b/net/socket/udp_socket_starboard.cc @@ -132,9 +132,8 @@ void UDPSocketStarboard::Close() { write_callback_.Reset(); send_to_address_.reset(); - bool ok = base::CurrentIOThread::Get()->UnregisterInterest( - socket_, kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite, - &socket_watcher_); + bool ok = socket_watcher_.UnregisterInterest(kSbSocketWaiterInterestRead | + kSbSocketWaiterInterestWrite); DCHECK(ok); is_connected_ = false; @@ -1018,16 +1017,14 @@ void UDPSocketStarboard::StopWatchingSocketForWriting() { return; write_async_watcher_->set_watching(false); if (!write_buf_) { - bool ok = base::CurrentIOThread::Get()->UnregisterInterest( - socket_, kSbSocketWaiterInterestWrite, &socket_watcher_); + bool ok = socket_watcher_.UnregisterInterest(kSbSocketWaiterInterestWrite); DCHECK(ok); } } void UDPSocketStarboard::StopWatchingSocketForReading() { if (!read_buf_) { - bool ok = base::CurrentIOThread::Get()->UnregisterInterest( - socket_, kSbSocketWaiterInterestRead, &socket_watcher_); + bool ok = socket_watcher_.UnregisterInterest(kSbSocketWaiterInterestRead); DCHECK(ok); } } From ebbfb05f3fd746e34272fcbeddb432d3612bb743 Mon Sep 17 00:00:00 2001 From: Jelle Foks Date: Tue, 19 Nov 2024 15:39:30 -0800 Subject: [PATCH 4/5] Fix the tests --- base/message_loop/message_pump_io_starboard.cc | 4 ++++ base/message_loop/message_pump_io_starboard.h | 3 --- .../message_pump_io_starboard_unittest.cc | 15 +++++++++------ 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/base/message_loop/message_pump_io_starboard.cc b/base/message_loop/message_pump_io_starboard.cc index 04490a49cd78..dbd571a12284 100644 --- a/base/message_loop/message_pump_io_starboard.cc +++ b/base/message_loop/message_pump_io_starboard.cc @@ -46,6 +46,8 @@ bool MessagePumpIOStarboard::SocketWatcher::UnregisterInterest(int interests) { if (pump_) { result = pump_->UnregisterInterest(socket_, interests, this); } + } else { + interests_ = 0; } if (!interests_) { pump_ = nullptr; @@ -229,6 +231,8 @@ void MessagePumpIOStarboard::Run(Delegate* delegate) { AutoReset auto_reset_keep_running(&keep_running_, true); for (;;) { + if (should_quit()) + break; Delegate::NextWorkInfo next_work_info = delegate->DoWork(); bool immediate_work_available = next_work_info.is_immediate(); diff --git a/base/message_loop/message_pump_io_starboard.h b/base/message_loop/message_pump_io_starboard.h index 547f9b760cab..8b1dc277c4eb 100644 --- a/base/message_loop/message_pump_io_starboard.h +++ b/base/message_loop/message_pump_io_starboard.h @@ -65,9 +65,6 @@ class BASE_EXPORT MessagePumpIOStarboard : public MessagePump { SocketWatcher(const SocketWatcher&) = delete; SocketWatcher& operator=(const SocketWatcher&) = delete; - // NOTE: These methods aren't called StartWatching()/StopWatching() to avoid - // confusion with the win32 ObjectWatcher class. - // Unregisters the interests for watching the socket, always safe to call. // No-op if there's nothing to do. bool UnregisterInterest(int interests); diff --git a/base/message_loop/message_pump_io_starboard_unittest.cc b/base/message_loop/message_pump_io_starboard_unittest.cc index e6e056387311..1b2cef2ef32d 100644 --- a/base/message_loop/message_pump_io_starboard_unittest.cc +++ b/base/message_loop/message_pump_io_starboard_unittest.cc @@ -36,6 +36,8 @@ class MessagePumpIOStarboardTest : public testing::Test { MessagePumpIOStarboardTest() : task_environment_(std::make_unique( test::SingleThreadTaskEnvironment::MainThreadType::DEFAULT)), + event_(WaitableEvent::ResetPolicy::AUTOMATIC, + WaitableEvent::InitialState::NOT_SIGNALED), io_thread_("MessagePumpIOStarboardTestIOThread") {} ~MessagePumpIOStarboardTest() override = default; @@ -87,6 +89,8 @@ class MessagePumpIOStarboardTest : public testing::Test { std::unique_ptr task_environment_; + WaitableEvent event_; + private: Thread io_thread_; SbSocket socket_; @@ -217,6 +221,7 @@ TEST_F(MessagePumpIOStarboardTest, NestedPumpWatcher) { } void FatalClosure() { + LOG(INFO) << __FUNCTION__ << " Fatal?"; FAIL() << "Reached fatal closure."; } @@ -254,8 +259,6 @@ TEST_F(MessagePumpIOStarboardTest, QuitWatcher) { RunLoop run_loop; QuitWatcher delegate(run_loop.QuitClosure()); MessagePumpIOStarboard::SocketWatcher controller(FROM_HERE); - WaitableEvent event(WaitableEvent::ResetPolicy::AUTOMATIC, - WaitableEvent::InitialState::NOT_SIGNALED); std::unique_ptr watcher(new WaitableEventWatcher); // Tell the pump to watch the pipe. @@ -263,18 +266,18 @@ TEST_F(MessagePumpIOStarboardTest, QuitWatcher) { /*persistent=*/false, kSbSocketWaiterInterestRead, &controller, &delegate); - // Make the IO thread wait for |event| before writing to pipefds[1]. + // Make the IO thread wait for |event_| before writing to pipefds[1]. const char buf = 0; WaitableEventWatcher::EventCallback write_socket_task = BindOnce(&WriteSocketWrapper, base::Unretained(pump)); io_runner()->PostTask( FROM_HERE, BindOnce(IgnoreResult(&WaitableEventWatcher::StartWatching), - Unretained(watcher.get()), &event, + Unretained(watcher.get()), &event_, std::move(write_socket_task), io_runner())); - // Queue |event| to signal on |sequence_manager|. + // Queue task to signal |event_|. SingleThreadTaskRunner::GetCurrentDefault()->PostTask( - FROM_HERE, BindOnce(&WaitableEvent::Signal, Unretained(&event))); + FROM_HERE, BindOnce(&WaitableEvent::Signal, Unretained(&event_))); // Now run the MessageLoop. run_loop.Run(); From 25d6343415970d90bec4edfc52a02890bcceaa8f Mon Sep 17 00:00:00 2001 From: Jelle Foks Date: Tue, 19 Nov 2024 11:24:17 -0800 Subject: [PATCH 5/5] temp logging --- net/socket/ssl_client_socket_impl.cc | 2 ++ net/socket/ssl_client_socket_unittest.cc | 7 +++++++ net/socket/tcp_socket_starboard.cc | 3 +++ net/socket/udp_socket_starboard.cc | 9 +++++++++ 4 files changed, 21 insertions(+) diff --git a/net/socket/ssl_client_socket_impl.cc b/net/socket/ssl_client_socket_impl.cc index 31256e7910ad..a665bac708dd 100644 --- a/net/socket/ssl_client_socket_impl.cc +++ b/net/socket/ssl_client_socket_impl.cc @@ -672,6 +672,7 @@ int SSLClientSocketImpl::ReadIfReady(IOBuffer* buf, } int SSLClientSocketImpl::CancelReadIfReady() { + LOG(INFO) << __FUNCTION__; DCHECK(user_read_callback_); DCHECK(!user_read_buf_); @@ -686,6 +687,7 @@ int SSLClientSocketImpl::CancelReadIfReady() { // will signal OnReadReady(), which will notice there is no read operation to // progress and skip it. user_read_callback_.Reset(); + LOG(INFO) << __FUNCTION__; return OK; } diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc index 8fd32cd7d426..4f5da1637fc8 100644 --- a/net/socket/ssl_client_socket_unittest.cc +++ b/net/socket/ssl_client_socket_unittest.cc @@ -436,14 +436,19 @@ bool FakeBlockingStreamSocket::ReplaceReadResult(const std::string& data) { } void FakeBlockingStreamSocket::WaitForReadResult() { + LOG(INFO) << __FUNCTION__; DCHECK(should_block_read_); DCHECK(!read_loop_); + LOG(INFO) << __FUNCTION__; if (pending_read_result_ != ERR_IO_PENDING) return; read_loop_ = std::make_unique(); + LOG(INFO) << __FUNCTION__; read_loop_->Run(); + LOG(INFO) << __FUNCTION__; read_loop_.reset(); + LOG(INFO) << __FUNCTION__; DCHECK_NE(ERR_IO_PENDING, pending_read_result_); } @@ -5554,7 +5559,9 @@ TEST_F(SSLClientSocketTest, CancelReadIfReady) { ASSERT_THAT(sock->CancelReadIfReady(), IsOk()); raw_transport->WaitForReadResult(); raw_transport->UnblockReadResult(); + LOG(INFO) << __FUNCTION__; base::RunLoop().RunUntilIdle(); + LOG(INFO) << __FUNCTION__; // Although data is now available, the callback should not have been called. EXPECT_FALSE(callback_called); diff --git a/net/socket/tcp_socket_starboard.cc b/net/socket/tcp_socket_starboard.cc index 73a97d32db9d..9f6053881b0b 100644 --- a/net/socket/tcp_socket_starboard.cc +++ b/net/socket/tcp_socket_starboard.cc @@ -468,11 +468,14 @@ int TCPSocketStarboard::ReadIfReady(IOBuffer* buf, int TCPSocketStarboard::CancelReadIfReady() { DCHECK(read_if_ready_callback_); + LOG(INFO) << __FUNCTION__; bool ok = socket_watcher_.UnregisterInterest(kSbSocketWaiterInterestRead); DCHECK(ok); + LOG(INFO) << __FUNCTION__; read_if_ready_callback_.Reset(); + LOG(INFO) << __FUNCTION__; return net::OK; } diff --git a/net/socket/udp_socket_starboard.cc b/net/socket/udp_socket_starboard.cc index 1bfd609bcc0b..8cd75fc5de9e 100644 --- a/net/socket/udp_socket_starboard.cc +++ b/net/socket/udp_socket_starboard.cc @@ -115,12 +115,16 @@ int UDPSocketStarboard::AdoptOpenedSocket(AddressFamily address_family, } void UDPSocketStarboard::Close() { + DLOG(INFO) << __FUNCTION__ << "Mark begin"; DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); + DLOG(INFO) << __FUNCTION__ << "Mark 1"; owned_socket_count_.Reset(); + DLOG(INFO) << __FUNCTION__ << "Mark 2"; if (socket_ == kSbSocketInvalid) return; + DLOG(INFO) << __FUNCTION__ << "Mark 3"; // Zero out any pending read/write callback state. read_buf_ = NULL; @@ -132,16 +136,21 @@ void UDPSocketStarboard::Close() { write_callback_.Reset(); send_to_address_.reset(); + DLOG(INFO) << __FUNCTION__ << "Mark 4"; bool ok = socket_watcher_.UnregisterInterest(kSbSocketWaiterInterestRead | kSbSocketWaiterInterestWrite); + DLOG(INFO) << __FUNCTION__ << "Mark 5"; DCHECK(ok); + DLOG(INFO) << __FUNCTION__ << "Mark 6"; is_connected_ = false; if (!SbSocketDestroy(socket_)) { DPLOG(ERROR) << "SbSocketDestroy"; } + DLOG(INFO) << __FUNCTION__ << "Mark 7"; socket_ = kSbSocketInvalid; + DLOG(INFO) << __FUNCTION__ << "Mark end"; } int UDPSocketStarboard::GetPeerAddress(IPEndPoint* address) const {