Skip to content

Commit

Permalink
cppgc: Avoid initializing cppgc platform through V8
Browse files Browse the repository at this point in the history
Embedders may use cppgc (or v8::CppHeap) earlier than V8's Isolate and
platform are initialized. Require explicit initialization of cppgc to
avoid recurring init calls with potentially conflicting parameters.

Bug: chromium:1056170
Change-Id: I613452954b322c9a5bf074eefd25107b4579958c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2682648
Reviewed-by: Omer Katz <[email protected]>
Reviewed-by: Ulan Degenbaev <[email protected]>
Commit-Queue: Michael Lippautz <[email protected]>
Cr-Commit-Position: refs/heads/master@{#72573}
  • Loading branch information
mlippautz authored and Commit Bot committed Feb 9, 2021
1 parent da78565 commit 8c99b25
Show file tree
Hide file tree
Showing 9 changed files with 32 additions and 18 deletions.
2 changes: 1 addition & 1 deletion include/cppgc/platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ class V8_EXPORT Platform {

/**
* Process-global initialization of the garbage collector. Must be called before
* creating a Heap.
* creating a Heap. Must only be called once per process.
*/
V8_EXPORT void InitializeProcess(PageAllocator*);

Expand Down
6 changes: 1 addition & 5 deletions samples/cppgc/cppgc-sample.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,7 @@ int main(int argc, char* argv[]) {
// backend allocation.
auto cppgc_platform = std::make_shared<cppgc::DefaultPlatform>();
// Initialize the process. This must happen before any cppgc::Heap::Create()
// calls. cppgc::DefaultPlatform::InitializeProcess initializes both cppgc
// and v8 (if cppgc is not used as a standalone) as needed.
// If using a platform other than cppgc::DefaultPlatform, should call
// cppgc::InitializeProcess (for standalone builds) or
// v8::V8::InitializePlatform (for non-standalone builds) directly.
// calls.
cppgc::DefaultPlatform::InitializeProcess(cppgc_platform.get());
// Create a managed heap.
std::unique_ptr<cppgc::Heap> heap = cppgc::Heap::Create(cppgc_platform);
Expand Down
9 changes: 0 additions & 9 deletions src/heap/cppgc/default-platform.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,11 @@

#include <include/cppgc/default-platform.h>

#if !CPPGC_IS_STANDALONE
#include <v8.h>
#endif // !CPPGC_IS_STANDALONE

namespace cppgc {

// static
void DefaultPlatform::InitializeProcess(DefaultPlatform* platform) {
#if CPPGC_IS_STANDALONE
cppgc::InitializeProcess(platform->GetPageAllocator());
#else
// v8::V8::InitializePlatform transitively calls cppgc::InitializeProcess.
v8::V8::InitializePlatform(platform->v8_platform_.get());
#endif // CPPGC_IS_STANDALONE
}

} // namespace cppgc
3 changes: 3 additions & 0 deletions src/heap/cppgc/platform.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ TracingController* Platform::GetTracingController() {
}

void InitializeProcess(PageAllocator* page_allocator) {
static PageAllocator* allocator = nullptr;
CHECK(!allocator);
internal::GlobalGCInfoTable::Create(page_allocator);
allocator = page_allocator;
}

void ShutdownProcess() {}
Expand Down
1 change: 0 additions & 1 deletion src/init/v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ void V8::InitializePlatform(v8::Platform* platform) {
platform_ = platform;
v8::base::SetPrintStackTrace(platform_->GetStackTracePrinter());
v8::tracing::TracingCategoryObserver::SetUp();
cppgc::InitializeProcess(platform->GetPageAllocator());
}

void V8::ShutdownPlatform() {
Expand Down
2 changes: 2 additions & 0 deletions test/cctest/cctest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

#include "test/cctest/cctest.h"

#include "include/cppgc/platform.h"
#include "include/libplatform/libplatform.h"
#include "include/v8.h"
#include "src/codegen/compiler.h"
Expand Down Expand Up @@ -333,6 +334,7 @@ int main(int argc, char* argv[]) {
v8::V8::InitializeICUDefaultLocation(argv[0]);
std::unique_ptr<v8::Platform> platform(v8::platform::NewDefaultPlatform());
v8::V8::InitializePlatform(platform.get());
cppgc::InitializeProcess(platform->GetPageAllocator());
using HelpOptions = v8::internal::FlagList::HelpOptions;
v8::internal::FlagList::SetFlagsFromCommandLine(
&argc, argv, true, HelpOptions(HelpOptions::kExit, usage.c_str()));
Expand Down
23 changes: 23 additions & 0 deletions test/unittests/heap/cppgc/run-all-unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,30 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "include/cppgc/platform.h"
#include "test/unittests/heap/cppgc/test-platform.h"
#include "testing/gmock/include/gmock/gmock.h"

namespace {

class DefaultPlatformEnvironment final : public ::testing::Environment {
public:
DefaultPlatformEnvironment() = default;

void SetUp() override {
platform_ =
std::make_unique<cppgc::internal::testing::TestPlatform>(nullptr);
cppgc::InitializeProcess(platform_->GetPageAllocator());
}

void TearDown() override { cppgc::ShutdownProcess(); }

private:
std::shared_ptr<cppgc::internal::testing::TestPlatform> platform_;
};

} // namespace

int main(int argc, char** argv) {
// Don't catch SEH exceptions and continue as the following tests might hang
// in an broken environment on windows.
Expand All @@ -13,5 +35,6 @@ int main(int argc, char** argv) {
testing::FLAGS_gtest_death_test_style = "threadsafe";

testing::InitGoogleMock(&argc, argv);
testing::AddGlobalTestEnvironment(new DefaultPlatformEnvironment);
return RUN_ALL_TESTS();
}
2 changes: 0 additions & 2 deletions test/unittests/heap/cppgc/tests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,10 @@ std::shared_ptr<TestPlatform> TestWithPlatform::platform_;
void TestWithPlatform::SetUpTestSuite() {
platform_ = std::make_unique<TestPlatform>(
std::make_unique<DelegatingTracingController>());
cppgc::InitializeProcess(platform_->GetPageAllocator());
}

// static
void TestWithPlatform::TearDownTestSuite() {
cppgc::ShutdownProcess();
platform_.reset();
}

Expand Down
2 changes: 2 additions & 0 deletions test/unittests/run-all-unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "include/cppgc/platform.h"
#include "include/libplatform/libplatform.h"
#include "include/v8.h"
#include "src/base/compiler-specific.h"
Expand All @@ -18,6 +19,7 @@ class DefaultPlatformEnvironment final : public ::testing::Environment {
0, v8::platform::IdleTaskSupport::kEnabled);
ASSERT_TRUE(platform_.get() != nullptr);
v8::V8::InitializePlatform(platform_.get());
cppgc::InitializeProcess(platform_->GetPageAllocator());
ASSERT_TRUE(v8::V8::Initialize());
}

Expand Down

0 comments on commit 8c99b25

Please sign in to comment.