From ffd616de4b7636ee7b4eeba642c0f4384832e649 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Tue, 12 Mar 2024 18:53:20 +0000 Subject: [PATCH] Fix some races in the platform isolate tests * Do not check IsolateIsShutdown after CreateAndRegisterIsolate in the MultithreadedCreation test. The test's main thread could run ShutdownPlatformIsolates between a worker thread's calls to CreateAndRegisterIsolate and IsolateIsShutdown. If that happens, IsolateIsShutdown will return true even though CreateAndRegisterIsolate succeeded. * Change the is_registered flag to was_registered and do not clear it during isolate shutdown Tests like MultithreadedCreation need to know whether RegisterPlatformIsolate succeeded so they can determine if the isolate will be shut down by ShutdownPlatformIsolates or if it needs to be shut down explicitly. If the flag is cleared during shutdown, then the test may see an isolate that was successfully registered and shut down and think that it was never registered. --- runtime/platform_isolate_manager_unittests.cc | 34 +++++++------------ 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/runtime/platform_isolate_manager_unittests.cc b/runtime/platform_isolate_manager_unittests.cc index e786819fdc858..312beff516999 100644 --- a/runtime/platform_isolate_manager_unittests.cc +++ b/runtime/platform_isolate_manager_unittests.cc @@ -15,7 +15,7 @@ struct IsolateData { PlatformIsolateManager* mgr; Dart_Isolate isolate = nullptr; bool is_shutdown = false; - bool is_registered = false; + bool was_registered = false; explicit IsolateData(PlatformIsolateManager* _mgr) : mgr(_mgr) {} }; @@ -96,7 +96,7 @@ class PlatformIsolateManagerTest : public FixtureTest { (*isolate_data_map_.get())[isolate].push_back( std::unique_ptr(isolate_data)); - isolate_data->is_registered = mgr->RegisterPlatformIsolate(isolate); + isolate_data->was_registered = mgr->RegisterPlatformIsolate(isolate); return isolate; } @@ -107,10 +107,10 @@ class PlatformIsolateManagerTest : public FixtureTest { return (*isolate_data_map_.get())[isolate].back()->is_shutdown; } - bool IsolateIsRegistered(Dart_Isolate isolate) { + bool IsolateWasRegistered(Dart_Isolate isolate) { EXPECT_EQ(1u, isolate_data_map_.get()->count(isolate)); EXPECT_LT(0u, (*isolate_data_map_.get())[isolate].size()); - return (*isolate_data_map_.get())[isolate].back()->is_registered; + return (*isolate_data_map_.get())[isolate].back()->was_registered; } private: @@ -122,9 +122,8 @@ class PlatformIsolateManagerTest : public FixtureTest { EXPECT_TRUE(isolate_data->isolate); EXPECT_FALSE(isolate_data->is_shutdown); isolate_data->is_shutdown = true; - if (isolate_data->is_registered) { + if (isolate_data->was_registered) { isolate_data->mgr->RemovePlatformIsolate(isolate_data->isolate); - isolate_data->is_registered = false; } } @@ -140,13 +139,13 @@ TEST_F(PlatformIsolateManagerTest, OrdinaryFlow) { Dart_Isolate isolateA = CreateAndRegisterIsolate(&mgr); ASSERT_TRUE(isolateA); EXPECT_FALSE(IsolateIsShutdown(isolateA)); - EXPECT_TRUE(IsolateIsRegistered(isolateA)); + EXPECT_TRUE(IsolateWasRegistered(isolateA)); EXPECT_TRUE(mgr.IsRegisteredForTestingOnly(isolateA)); Dart_Isolate isolateB = CreateAndRegisterIsolate(&mgr); ASSERT_TRUE(isolateB); EXPECT_FALSE(IsolateIsShutdown(isolateB)); - EXPECT_TRUE(IsolateIsRegistered(isolateB)); + EXPECT_TRUE(IsolateWasRegistered(isolateB)); EXPECT_TRUE(mgr.IsRegisteredForTestingOnly(isolateB)); mgr.ShutdownPlatformIsolates(); @@ -154,10 +153,8 @@ TEST_F(PlatformIsolateManagerTest, OrdinaryFlow) { EXPECT_TRUE(mgr.HasShutdownMaybeFalseNegative()); EXPECT_TRUE(IsolateIsShutdown(isolateA)); - EXPECT_FALSE(IsolateIsRegistered(isolateA)); EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateA)); EXPECT_TRUE(IsolateIsShutdown(isolateB)); - EXPECT_FALSE(IsolateIsRegistered(isolateB)); EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateB)); }); } @@ -170,35 +167,31 @@ TEST_F(PlatformIsolateManagerTest, EarlyShutdown) { Dart_Isolate isolateA = CreateAndRegisterIsolate(&mgr); ASSERT_TRUE(isolateA); EXPECT_FALSE(IsolateIsShutdown(isolateA)); - EXPECT_TRUE(IsolateIsRegistered(isolateA)); + EXPECT_TRUE(IsolateWasRegistered(isolateA)); EXPECT_TRUE(mgr.IsRegisteredForTestingOnly(isolateA)); Dart_Isolate isolateB = CreateAndRegisterIsolate(&mgr); ASSERT_TRUE(isolateB); EXPECT_FALSE(IsolateIsShutdown(isolateB)); - EXPECT_TRUE(IsolateIsRegistered(isolateB)); + EXPECT_TRUE(IsolateWasRegistered(isolateB)); EXPECT_TRUE(mgr.IsRegisteredForTestingOnly(isolateB)); Dart_EnterIsolate(isolateA); Dart_ShutdownIsolate(); EXPECT_TRUE(IsolateIsShutdown(isolateA)); - EXPECT_FALSE(IsolateIsRegistered(isolateA)); EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateA)); Dart_EnterIsolate(isolateB); Dart_ShutdownIsolate(); EXPECT_TRUE(IsolateIsShutdown(isolateB)); - EXPECT_FALSE(IsolateIsRegistered(isolateB)); EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateB)); mgr.ShutdownPlatformIsolates(); EXPECT_TRUE(mgr.HasShutdown()); EXPECT_TRUE(IsolateIsShutdown(isolateA)); - EXPECT_FALSE(IsolateIsRegistered(isolateA)); EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateA)); EXPECT_TRUE(IsolateIsShutdown(isolateB)); - EXPECT_FALSE(IsolateIsRegistered(isolateB)); EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateB)); }); } @@ -211,26 +204,24 @@ TEST_F(PlatformIsolateManagerTest, RegistrationAfterShutdown) { Dart_Isolate isolateA = CreateAndRegisterIsolate(&mgr); ASSERT_TRUE(isolateA); EXPECT_FALSE(IsolateIsShutdown(isolateA)); - EXPECT_TRUE(IsolateIsRegistered(isolateA)); + EXPECT_TRUE(IsolateWasRegistered(isolateA)); EXPECT_TRUE(mgr.IsRegisteredForTestingOnly(isolateA)); mgr.ShutdownPlatformIsolates(); EXPECT_TRUE(mgr.HasShutdown()); EXPECT_TRUE(IsolateIsShutdown(isolateA)); - EXPECT_FALSE(IsolateIsRegistered(isolateA)); EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateA)); Dart_Isolate isolateB = CreateAndRegisterIsolate(&mgr); ASSERT_TRUE(isolateB); EXPECT_FALSE(IsolateIsShutdown(isolateB)); - EXPECT_FALSE(IsolateIsRegistered(isolateB)); + EXPECT_FALSE(IsolateWasRegistered(isolateB)); EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateB)); Dart_EnterIsolate(isolateB); Dart_ShutdownIsolate(); EXPECT_TRUE(IsolateIsShutdown(isolateB)); - EXPECT_FALSE(IsolateIsRegistered(isolateB)); EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateB)); }); } @@ -250,9 +241,8 @@ TEST_F(PlatformIsolateManagerTest, MultithreadedCreation) { for (int j = 0; j < 100; ++j) { Dart_Isolate isolate = CreateAndRegisterIsolate(&mgr); ASSERT_TRUE(isolate); - EXPECT_FALSE(IsolateIsShutdown(isolate)); - if (!IsolateIsRegistered(isolate)) { + if (!IsolateWasRegistered(isolate)) { Dart_EnterIsolate(isolate); Dart_ShutdownIsolate(); }