From 69cf4732e6fc941622eb70c84869f35b52ef8281 Mon Sep 17 00:00:00 2001 From: Brett Wilson Date: Fri, 1 Sep 2017 01:34:40 +0000 Subject: [PATCH] memlog: Basic end-to-end test framework. This set up test stubs for all 3 major modes of memlog. Currently it only tests that the system doesn't crash, and that the ProfilingProcessHost is only started when the flags are passed. Note, this test is focused on ONLY the memlog interface. ProfilingProcessHost may eventually evolved into hosting other types of profiling services thus the test is named for Memlog and not PPH. Also, with this CL is a shutdown crash fix for MemlogClient being deleted on the wrong thread. Bug: 753218 Change-Id: I37111b7a61d9a4c18b37680b3216975be2d0535c Reviewed-on: https://chromium-review.googlesource.com/644608 Commit-Queue: Brett Wilson Reviewed-by: Erik Chen Reviewed-by: Brett Wilson Reviewed-by: Albert J. Wong Cr-Commit-Position: refs/heads/master@{#499103} --- .gn | 1 + chrome/browser/profiling_host/BUILD.gn | 24 +++++++++ ...rome_browser_main_extra_parts_profiling.cc | 2 +- .../profiling_host/memlog_browsertest.cc | 49 +++++++++++++++++++ .../profiling_host/profiling_process_host.cc | 6 ++- .../profiling_host/profiling_process_host.h | 9 +++- chrome/common/chrome_content_client.cc | 6 ++- chrome/common/chrome_content_client.h | 2 - chrome/test/BUILD.gn | 2 + 9 files changed, 94 insertions(+), 7 deletions(-) create mode 100644 chrome/browser/profiling_host/memlog_browsertest.cc diff --git a/.gn b/.gn index 29d65d771249b3..138645620bc764 100644 --- a/.gn +++ b/.gn @@ -76,6 +76,7 @@ check_targets = [ "//chrome/browser/ui/*", "//chrome/common/*", "//chrome/installer/*", + "//chrome/profiling", "//chrome/third_party/mozilla_security_manager/*", "//chrome/tools/*", "//chrome/utility/*", diff --git a/chrome/browser/profiling_host/BUILD.gn b/chrome/browser/profiling_host/BUILD.gn index 588301f8e21d59..533bbbfb071288 100644 --- a/chrome/browser/profiling_host/BUILD.gn +++ b/chrome/browser/profiling_host/BUILD.gn @@ -19,3 +19,27 @@ static_library("profiling_host") { "//content/public/common", ] } + +if (!is_android) { + source_set("profiling_browsertests") { + testonly = true + + sources = [ + "memlog_browsertest.cc", + ] + + defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ] + + deps = [ + "//base", + "//base/allocator:features", + "//chrome/test:test_support_ui", + "//testing/gmock", + "//testing/gtest", + ] + } +} else { + # In-process browser tests aren't supported on Android. + group("profiling_browsertests") { + } +} diff --git a/chrome/browser/profiling_host/chrome_browser_main_extra_parts_profiling.cc b/chrome/browser/profiling_host/chrome_browser_main_extra_parts_profiling.cc index 7d4fd28c5e3002..933605e5f9f28f 100644 --- a/chrome/browser/profiling_host/chrome_browser_main_extra_parts_profiling.cc +++ b/chrome/browser/profiling_host/chrome_browser_main_extra_parts_profiling.cc @@ -18,5 +18,5 @@ void ChromeBrowserMainExtraPartsProfiling::ServiceManagerConnectionStarted( profiling::ProfilingProcessHost::Mode mode = profiling::ProfilingProcessHost::GetCurrentMode(); if (mode != profiling::ProfilingProcessHost::Mode::kNone) - profiling::ProfilingProcessHost::EnsureStarted(connection, mode); + profiling::ProfilingProcessHost::Start(connection, mode); } diff --git a/chrome/browser/profiling_host/memlog_browsertest.cc b/chrome/browser/profiling_host/memlog_browsertest.cc new file mode 100644 index 00000000000000..2098ad99ebd032 --- /dev/null +++ b/chrome/browser/profiling_host/memlog_browsertest.cc @@ -0,0 +1,49 @@ +// Copyright (c) 2012 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 "base/allocator/features.h" +#include "chrome/browser/profiling_host/profiling_process_host.h" +#include "chrome/common/chrome_switches.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "content/public/test/browser_test.h" +#include "testing/gtest/include/gtest/gtest.h" + +// Some builds don't support the allocator shim in which case the memory long +// won't function. +#if BUILDFLAG(USE_ALLOCATOR_SHIM) + +namespace { + +class MemlogBrowserTest : public InProcessBrowserTest, + public testing::WithParamInterface { + protected: + void SetUpDefaultCommandLine(base::CommandLine* command_line) override { + InProcessBrowserTest::SetUpDefaultCommandLine(command_line); + if (GetParam()) + command_line->AppendSwitchASCII(switches::kMemlog, GetParam()); + } +}; + +IN_PROC_BROWSER_TEST_P(MemlogBrowserTest, EndToEnd) { + if (!GetParam()) { + // Test that nothing has been started if the flag is not passed. + ASSERT_FALSE(profiling::ProfilingProcessHost::has_started()); + } else { + ASSERT_TRUE(profiling::ProfilingProcessHost::has_started()); + } +} + +INSTANTIATE_TEST_CASE_P(NoMemlog, + MemlogBrowserTest, + ::testing::Values(static_cast(nullptr))); +INSTANTIATE_TEST_CASE_P(BrowserOnly, + MemlogBrowserTest, + ::testing::Values(switches::kMemlogModeBrowser)); +INSTANTIATE_TEST_CASE_P(AllProcesses, + MemlogBrowserTest, + ::testing::Values(switches::kMemlogModeAll)); + +} // namespace + +#endif // BUILDFLAG(USE_ALLOCATOR_SHIM) diff --git a/chrome/browser/profiling_host/profiling_process_host.cc b/chrome/browser/profiling_host/profiling_process_host.cc index 6fd08ae73b0c90..82aa9fe5b325b8 100644 --- a/chrome/browser/profiling_host/profiling_process_host.cc +++ b/chrome/browser/profiling_host/profiling_process_host.cc @@ -52,6 +52,8 @@ class StringWrapper : public base::trace_event::ConvertableToTraceFormat { namespace profiling { +bool ProfilingProcessHost::has_started_ = false; + namespace { const size_t kMaxTraceSizeUploadInBytes = 10 * 1024 * 1024; @@ -251,10 +253,12 @@ ProfilingProcessHost::Mode ProfilingProcessHost::GetCurrentMode() { } // static -ProfilingProcessHost* ProfilingProcessHost::EnsureStarted( +ProfilingProcessHost* ProfilingProcessHost::Start( content::ServiceManagerConnection* connection, Mode mode) { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); + CHECK(!has_started_); + has_started_ = true; ProfilingProcessHost* host = GetInstance(); host->SetMode(mode); host->MakeConnector(connection); diff --git a/chrome/browser/profiling_host/profiling_process_host.h b/chrome/browser/profiling_host/profiling_process_host.h index 4a45f34cfd6054..c7953573695bc2 100644 --- a/chrome/browser/profiling_host/profiling_process_host.h +++ b/chrome/browser/profiling_host/profiling_process_host.h @@ -61,11 +61,14 @@ class ProfilingProcessHost : public content::BrowserChildProcessObserver, // Returns the mode set on the current process' command line. static Mode GetCurrentMode(); - // Launches the profiling process if necessary and returns a pointer to it. - static ProfilingProcessHost* EnsureStarted( + // Launches the profiling process and returns a pointer to it. + static ProfilingProcessHost* Start( content::ServiceManagerConnection* connection, Mode mode); + // Returns true if Start() has been called. + static bool has_started() { return has_started_; } + // Returns a pointer to the current global profiling process host. static ProfilingProcessHost* GetInstance(); @@ -138,6 +141,8 @@ class ProfilingProcessHost : public content::BrowserChildProcessObserver, // The mode determines which processes should be profiled. Mode mode_; + static bool has_started_; + DISALLOW_COPY_AND_ASSIGN(ProfilingProcessHost); }; diff --git a/chrome/common/chrome_content_client.cc b/chrome/common/chrome_content_client.cc index 17d9819ad2b0df..3182e96c73fdbd 100644 --- a/chrome/common/chrome_content_client.cc +++ b/chrome/common/chrome_content_client.cc @@ -15,6 +15,7 @@ #include "base/debug/crash_logging.h" #include "base/files/file_util.h" #include "base/json/json_reader.h" +#include "base/lazy_instance.h" #include "base/memory/ptr_util.h" #include "base/native_library.h" #include "base/path_service.h" @@ -31,6 +32,7 @@ #include "chrome/common/chrome_switches.h" #include "chrome/common/crash_keys.h" #include "chrome/common/pepper_flash.h" +#include "chrome/common/profiling/memlog_client.h" #include "chrome/common/secure_origin_whitelist.h" #include "chrome/common/url_constants.h" #include "chrome/grit/common_resources.h" @@ -713,5 +715,7 @@ media::MediaDrmBridgeClient* ChromeContentClient::GetMediaDrmBridgeClient() { void ChromeContentClient::OnServiceManagerConnected( content::ServiceManagerConnection* connection) { - memlog_client_.OnServiceManagerConnected(connection); + static base::LazyInstance::Leaky memlog_client = + LAZY_INSTANCE_INITIALIZER; + memlog_client.Get().OnServiceManagerConnected(connection); } diff --git a/chrome/common/chrome_content_client.h b/chrome/common/chrome_content_client.h index e2790e45703aef..683f4344b08182 100644 --- a/chrome/common/chrome_content_client.h +++ b/chrome/common/chrome_content_client.h @@ -16,7 +16,6 @@ #include "build/build_config.h" #include "chrome/common/features.h" #include "chrome/common/origin_trials/chrome_origin_trial_policy.h" -#include "chrome/common/profiling/memlog_client.h" #include "content/public/common/content_client.h" #include "ppapi/features/features.h" @@ -114,7 +113,6 @@ class ChromeContentClient : public content::ContentClient { // Used to lock when |origin_trial_policy_| is initialized. base::Lock origin_trial_policy_lock_; std::unique_ptr origin_trial_policy_; - profiling::MemlogClient memlog_client_; }; #endif // CHROME_COMMON_CHROME_CONTENT_CLIENT_H_ diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index 345d0bc51239ed..c806e30248823b 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn @@ -329,6 +329,7 @@ if (!is_android) { public_deps = [ "//chrome/browser:test_support_ui", + "//content/public/browser", ] deps = [ "//components/metrics:test_support", @@ -1119,6 +1120,7 @@ test("browser_tests") { ":browser_tests_runner", ":test_support", "//base", + "//chrome/browser/profiling_host:profiling_browsertests", "//components/spellcheck:build_features", "//components/sync:test_support_model", "//extensions/features",