Skip to content

Commit

Permalink
Fix global state leak in gl_tests.
Browse files Browse the repository at this point in the history
There is a global CommandBufferTaskExecutor that gets created if an
instance isn't provided. This task executor picks up the current command
line to create GpuPreferences on first use, which caused problems where
depending on what test ran first the GpuPreferences would be different
and later tests would fail.

Don't use the global task executor in gl_tests. Move and rename
InProcessGpuThreadHolder so that each test which needs it can
instantiate the object itself. Also enable
GLInProcessCommandBufferTest.CreateImage again.

Bug: 897456
Change-Id: Icdc307bd68453fdc2b898a9765a3f903b21fdfe8
Reviewed-on: https://chromium-review.googlesource.com/c/1298214
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603143}
  • Loading branch information
kylechar authored and Commit Bot committed Oct 26, 2018
1 parent f33f9de commit d6d5893
Show file tree
Hide file tree
Showing 8 changed files with 194 additions and 96 deletions.
25 changes: 5 additions & 20 deletions gpu/command_buffer/tests/gl_tests_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,17 @@
#include "base/command_line.h"
#include "base/feature_list.h"
#include "base/message_loop/message_loop.h"
#if defined(OS_MACOSX)
#include "base/mac/scoped_nsautorelease_pool.h"
#endif
#include "base/test/launcher/unit_test_launcher.h"
#include "base/test/test_suite.h"
#include "build/build_config.h"
#include "gpu/command_buffer/client/gles2_lib.h"
#include "gpu/command_buffer/tests/gl_test_utils.h"
#include "gpu/config/gpu_info_collector.h"
#include "gpu/config/gpu_preferences.h"
#include "gpu/config/gpu_util.h"
#include "gpu/ipc/in_process_command_buffer.h"
#include "testing/gmock/include/gmock/gmock.h"

#if defined(OS_MACOSX)
#include "base/mac/scoped_nsautorelease_pool.h"
#endif

namespace {

int RunHelper(base::TestSuite* testSuite) {
Expand All @@ -31,19 +29,6 @@ int RunHelper(base::TestSuite* testSuite) {
base::FeatureList::InitializeInstance(std::string(), std::string());
gpu::GLTestHelper::InitializeGLDefault();

base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
gpu::GPUInfo gpu_info;
gpu::CollectGraphicsInfoForTesting(&gpu_info);
gpu::GpuFeatureInfo gpu_feature_info = gpu::ComputeGpuFeatureInfo(
gpu_info, gpu::GpuPreferences(), command_line, nullptr);
// Always enable gpu and oop raster, regardless of platform and blacklist.
gpu_feature_info.status_values[gpu::GPU_FEATURE_TYPE_GPU_RASTERIZATION] =
gpu::kGpuFeatureStatusEnabled;
gpu_feature_info.status_values[gpu::GPU_FEATURE_TYPE_OOP_RASTERIZATION] =
gpu::kGpuFeatureStatusEnabled;
gpu::InProcessCommandBuffer::InitializeDefaultServiceForTesting(
gpu_feature_info);

::gles2::Initialize();
return testSuite->Run();
}
Expand Down
2 changes: 2 additions & 0 deletions gpu/ipc/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ component("gl_in_process_context") {
"gpu_in_process_thread_service.h",
"in_process_command_buffer.cc",
"in_process_command_buffer.h",
"in_process_gpu_thread_holder.cc",
"in_process_gpu_thread_holder.h",
]

defines = [ "GL_IN_PROCESS_CONTEXT_IMPLEMENTATION" ]
Expand Down
18 changes: 8 additions & 10 deletions gpu/ipc/client/gpu_in_process_context_tests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "gpu/command_buffer/client/shared_memory_limits.h"
#include "gpu/ipc/common/surface_handle.h"
#include "gpu/ipc/gl_in_process_context.h"
#include "gpu/ipc/in_process_gpu_thread_holder.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace {
Expand All @@ -36,13 +37,11 @@ class ContextTestBase : public testing::Test {

auto context = std::make_unique<gpu::GLInProcessContext>();
auto result = context->Initialize(
nullptr, /* service */
nullptr, /* surface */
true, /* offscreen */
gpu::kNullSurfaceHandle, /* window */
attributes, gpu::SharedMemoryLimits(), gpu_memory_buffer_manager_.get(),
nullptr, /* image_factory */
base::ThreadTaskRunnerHandle::Get());
gpu_thread_holder_.GetTaskExecutor(),
/*surface=*/nullptr, /*offscreen=*/true,
/*window=*/gpu::kNullSurfaceHandle, attributes,
gpu::SharedMemoryLimits(), gpu_memory_buffer_manager_.get(),
/*image_factory=*/nullptr, base::ThreadTaskRunnerHandle::Get());
DCHECK_EQ(result, gpu::ContextResult::kSuccess);
return context;
}
Expand All @@ -66,6 +65,7 @@ class ContextTestBase : public testing::Test {
std::unique_ptr<gpu::GpuMemoryBufferManager> gpu_memory_buffer_manager_;

private:
gpu::InProcessGpuThreadHolder gpu_thread_holder_;
std::unique_ptr<gpu::GLInProcessContext> context_;
};

Expand All @@ -77,9 +77,7 @@ class ContextTestBase : public testing::Test {

using GLInProcessCommandBufferTest = ContextTestBase;

// TODO(https://crbug.com/897456): Test leaks state, causing the
// WebGPUInProcessCommandBufferTest suite to fail if run in the same process.
TEST_F(GLInProcessCommandBufferTest, DISABLED_CreateImage) {
TEST_F(GLInProcessCommandBufferTest, CreateImage) {
constexpr gfx::BufferFormat kBufferFormat = gfx::BufferFormat::RGBA_8888;
constexpr gfx::BufferUsage kBufferUsage = gfx::BufferUsage::SCANOUT;
constexpr gfx::Size kBufferSize(100, 100);
Expand Down
13 changes: 12 additions & 1 deletion gpu/ipc/client/raster_in_process_context_tests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "gpu/command_buffer/client/shared_memory_limits.h"
#include "gpu/command_buffer/common/shared_image_usage.h"
#include "gpu/ipc/host/gpu_memory_buffer_support.h"
#include "gpu/ipc/in_process_gpu_thread_holder.h"
#include "gpu/ipc/raster_in_process_context.h"
#include "gpu/ipc/service/gpu_memory_buffer_factory.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand All @@ -30,6 +31,15 @@ constexpr gfx::Size kBufferSize(100, 100);

class RasterInProcessCommandBufferTest : public ::testing::Test {
public:
RasterInProcessCommandBufferTest() {
// Always enable gpu and oop raster, regardless of platform and blacklist.
auto* gpu_feature_info = gpu_thread_holder_.GetGpuFeatureInfo();
gpu_feature_info->status_values[gpu::GPU_FEATURE_TYPE_GPU_RASTERIZATION] =
gpu::kGpuFeatureStatusEnabled;
gpu_feature_info->status_values[gpu::GPU_FEATURE_TYPE_OOP_RASTERIZATION] =
gpu::kGpuFeatureStatusEnabled;
}

std::unique_ptr<RasterInProcessContext> CreateRasterInProcessContext() {
if (!RasterInProcessContext::SupportedInTest())
return nullptr;
Expand All @@ -42,7 +52,7 @@ class RasterInProcessCommandBufferTest : public ::testing::Test {

auto context = std::make_unique<RasterInProcessContext>();
auto result = context->Initialize(
/*service=*/nullptr, attributes, SharedMemoryLimits(),
gpu_thread_holder_.GetTaskExecutor(), attributes, SharedMemoryLimits(),
gpu_memory_buffer_manager_.get(),
gpu_memory_buffer_factory_->AsImageFactory(),
/*gpu_channel_manager_delegate=*/nullptr, nullptr, nullptr);
Expand All @@ -67,6 +77,7 @@ class RasterInProcessCommandBufferTest : public ::testing::Test {
}

protected:
InProcessGpuThreadHolder gpu_thread_holder_;
raster::RasterInterface* ri_; // not owned
std::unique_ptr<GpuMemoryBufferFactory> gpu_memory_buffer_factory_;
std::unique_ptr<GpuMemoryBufferManager> gpu_memory_buffer_manager_;
Expand Down
14 changes: 8 additions & 6 deletions gpu/ipc/client/webgpu_in_process_context_tests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "gpu/command_buffer/client/shared_memory_limits.h"
#include "gpu/command_buffer/client/webgpu_implementation.h"
#include "gpu/config/gpu_switches.h"
#include "gpu/ipc/in_process_gpu_thread_holder.h"
#include "gpu/ipc/webgpu_in_process_context.h"
#include "testing/gtest/include/gtest/gtest.h"

Expand All @@ -18,23 +19,23 @@ namespace {

class WebGPUInProcessCommandBufferTest : public ::testing::Test {
public:
std::unique_ptr<WebGPUInProcessContext> CreateWebGPUInProcessContext() {
base::CommandLine::ForCurrentProcess()->AppendSwitch(
switches::kEnableUnsafeWebGPU);
WebGPUInProcessCommandBufferTest() {
gpu_thread_holder_.GetGpuPreferences()->enable_webgpu = true;
}

std::unique_ptr<WebGPUInProcessContext> CreateWebGPUInProcessContext() {
ContextCreationAttribs attributes;
attributes.bind_generates_resource = false;
attributes.enable_gles2_interface = false;
attributes.context_type = CONTEXT_TYPE_WEBGPU;

static scoped_refptr<CommandBufferTaskExecutor> task_executor = nullptr;
static constexpr GpuMemoryBufferManager* memory_buffer_manager = nullptr;
static constexpr ImageFactory* image_factory = nullptr;
static constexpr GpuChannelManagerDelegate* channel_manager = nullptr;
auto context = std::make_unique<WebGPUInProcessContext>();
auto result = context->Initialize(
task_executor, attributes, SharedMemoryLimits(), memory_buffer_manager,
image_factory, channel_manager);
gpu_thread_holder_.GetTaskExecutor(), attributes, SharedMemoryLimits(),
memory_buffer_manager, image_factory, channel_manager);
DCHECK_EQ(result, ContextResult::kSuccess);
return context;
}
Expand All @@ -47,6 +48,7 @@ class WebGPUInProcessCommandBufferTest : public ::testing::Test {
void TearDown() override { context_.reset(); }

protected:
InProcessGpuThreadHolder gpu_thread_holder_;
webgpu::WebGPUInterface* wg_; // not owned
std::unique_ptr<WebGPUInProcessContext> context_;
};
Expand Down
62 changes: 3 additions & 59 deletions gpu/ipc/in_process_command_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@
#include "gpu/ipc/command_buffer_task_executor.h"
#include "gpu/ipc/common/command_buffer_id.h"
#include "gpu/ipc/common/gpu_client_ids.h"
#include "gpu/ipc/gpu_in_process_thread_service.h"
#include "gpu/ipc/host/gpu_memory_buffer_support.h"
#include "gpu/ipc/in_process_gpu_thread_holder.h"
#include "gpu/ipc/service/gpu_channel_manager_delegate.h"
#include "gpu/ipc/service/image_transport_surface.h"
#include "ui/gfx/geometry/size.h"
Expand Down Expand Up @@ -109,63 +109,7 @@ base::OnceClosure WrapTaskWithResult(base::OnceCallback<T(void)> task,
return base::BindOnce(wrapper, std::move(task), result, completion);
}

class GpuInProcessThreadHolder : public base::Thread {
public:
GpuInProcessThreadHolder() : base::Thread("GpuThread") { Start(); }

~GpuInProcessThreadHolder() override {
task_runner()->DeleteSoon(FROM_HERE, std::move(scheduler_));
task_runner()->DeleteSoon(FROM_HERE, std::move(sync_point_manager_));
Stop();
}

void InitializeOnGpuThread(base::WaitableEvent* completion) {
DCHECK(base::CommandLine::InitializedForCurrentProcess());
const base::CommandLine* command_line =
base::CommandLine::ForCurrentProcess();
GpuPreferences gpu_preferences = gles2::ParseGpuPreferences(command_line);
gpu_preferences.texture_target_exception_list =
CreateBufferUsageAndFormatExceptionList();

sync_point_manager_ = std::make_unique<SyncPointManager>();
scheduler_ =
std::make_unique<Scheduler>(task_runner(), sync_point_manager_.get());
gpu_thread_service_ = base::MakeRefCounted<GpuInProcessThreadService>(
task_runner(), scheduler_.get(), sync_point_manager_.get(), nullptr,
nullptr, gl::GLSurfaceFormat(), gpu_feature_info_, gpu_preferences);

completion->Signal();
}

void SetGpuFeatureInfo(const GpuFeatureInfo& gpu_feature_info) {
DCHECK(!gpu_thread_service_.get());
gpu_feature_info_ = gpu_feature_info;
}

scoped_refptr<CommandBufferTaskExecutor> GetGpuThreadService() {
if (!gpu_thread_service_) {
base::WaitableEvent completion(
base::WaitableEvent::ResetPolicy::MANUAL,
base::WaitableEvent::InitialState::NOT_SIGNALED);
task_runner()->PostTask(
FROM_HERE,
base::BindOnce(&GpuInProcessThreadHolder::InitializeOnGpuThread,
base::Unretained(this), &completion));
completion.Wait();
}
return gpu_thread_service_;
}

private:
std::unique_ptr<SyncPointManager> sync_point_manager_;
std::unique_ptr<Scheduler> scheduler_;
scoped_refptr<CommandBufferTaskExecutor> gpu_thread_service_;
GpuFeatureInfo gpu_feature_info_;

DISALLOW_COPY_AND_ASSIGN(GpuInProcessThreadHolder);
};

base::LazyInstance<GpuInProcessThreadHolder>::DestructorAtExit
base::LazyInstance<InProcessGpuThreadHolder>::DestructorAtExit
g_default_task_executer = LAZY_INSTANCE_INITIALIZER;

class ScopedEvent {
Expand All @@ -191,7 +135,7 @@ scoped_refptr<CommandBufferTaskExecutor> MaybeGetDefaultTaskExecutor(
// ThreadTaskRunnerHandle, which will re-add a new task to the, AtExitManager,
// which causes a deadlock because it's already locked.
base::ThreadTaskRunnerHandle::IsSet();
return g_default_task_executer.Get().GetGpuThreadService();
return g_default_task_executer.Get().GetTaskExecutor();
}

} // namespace
Expand Down
91 changes: 91 additions & 0 deletions gpu/ipc/in_process_gpu_thread_holder.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "gpu/ipc/in_process_gpu_thread_holder.h"

#include "base/command_line.h"
#include "base/synchronization/waitable_event.h"
#include "base/threading/thread_task_runner_handle.h"
#include "gpu/command_buffer/service/scheduler.h"
#include "gpu/command_buffer/service/service_utils.h"
#include "gpu/command_buffer/service/sync_point_manager.h"
#include "gpu/config/gpu_info_collector.h"
#include "gpu/config/gpu_util.h"
#include "gpu/ipc/gpu_in_process_thread_service.h"
#include "gpu/ipc/host/gpu_memory_buffer_support.h"

namespace gpu {

InProcessGpuThreadHolder::InProcessGpuThreadHolder()
: base::Thread("GpuThread") {
DCHECK(base::CommandLine::InitializedForCurrentProcess());
auto* command_line = base::CommandLine::ForCurrentProcess();
gpu_preferences_ = gles2::ParseGpuPreferences(command_line);
gpu_preferences_.texture_target_exception_list =
CreateBufferUsageAndFormatExceptionList();

gpu::GPUInfo gpu_info;
gpu::CollectGraphicsInfoForTesting(&gpu_info);
gpu::GpuFeatureInfo gpu_feature_info = gpu::ComputeGpuFeatureInfo(
gpu_info, gpu_preferences_, command_line, nullptr);

Start();
}

InProcessGpuThreadHolder::~InProcessGpuThreadHolder() {
// Ensure members created on GPU thread are destroyed there too.
task_runner()->PostTask(
FROM_HERE, base::BindOnce(&InProcessGpuThreadHolder::DeleteOnGpuThread,
base::Unretained(this)));
Stop();
}

GpuPreferences* InProcessGpuThreadHolder::GetGpuPreferences() {
DCHECK(!task_executor_);
return &gpu_preferences_;
}

GpuFeatureInfo* InProcessGpuThreadHolder::GetGpuFeatureInfo() {
DCHECK(!task_executor_);
return &gpu_feature_info_;
}

void InProcessGpuThreadHolder::SetGpuFeatureInfo(
const GpuFeatureInfo& gpu_feature_info) {
DCHECK(!task_executor_);
gpu_feature_info_ = gpu_feature_info;
}

scoped_refptr<CommandBufferTaskExecutor>
InProcessGpuThreadHolder::GetTaskExecutor() {
if (!task_executor_) {
base::WaitableEvent completion;
task_runner()->PostTask(
FROM_HERE,
base::BindOnce(&InProcessGpuThreadHolder::InitializeOnGpuThread,
base::Unretained(this), &completion));
completion.Wait();
}
return task_executor_;
}

void InProcessGpuThreadHolder::InitializeOnGpuThread(
base::WaitableEvent* completion) {
sync_point_manager_ = std::make_unique<SyncPointManager>();
scheduler_ =
std::make_unique<Scheduler>(task_runner(), sync_point_manager_.get());
task_executor_ = base::MakeRefCounted<GpuInProcessThreadService>(
task_runner(), scheduler_.get(), sync_point_manager_.get(), nullptr,
nullptr, gl::GLSurfaceFormat(), gpu_feature_info_, gpu_preferences_);

completion->Signal();
}

void InProcessGpuThreadHolder::DeleteOnGpuThread() {
task_executor_.reset();
scheduler_.reset();
sync_point_manager_.reset();
}

} // namespace gpu
Loading

0 comments on commit d6d5893

Please sign in to comment.