Skip to content

Commit

Permalink
memlog: Basic end-to-end test framework.
Browse files Browse the repository at this point in the history
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 <brettw@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499103}
  • Loading branch information
Brett Wilson authored and Commit Bot committed Sep 1, 2017
1 parent c5484d8 commit 69cf473
Show file tree
Hide file tree
Showing 9 changed files with 94 additions and 7 deletions.
1 change: 1 addition & 0 deletions .gn
Original file line number Diff line number Diff line change
Expand Up @@ -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/*",
Expand Down
24 changes: 24 additions & 0 deletions chrome/browser/profiling_host/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
49 changes: 49 additions & 0 deletions chrome/browser/profiling_host/memlog_browsertest.cc
Original file line number Diff line number Diff line change
@@ -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<const char*> {
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<const char*>(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)
6 changes: 5 additions & 1 deletion chrome/browser/profiling_host/profiling_process_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
9 changes: 7 additions & 2 deletions chrome/browser/profiling_host/profiling_process_host.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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);
};

Expand Down
6 changes: 5 additions & 1 deletion chrome/common/chrome_content_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -713,5 +715,7 @@ media::MediaDrmBridgeClient* ChromeContentClient::GetMediaDrmBridgeClient() {

void ChromeContentClient::OnServiceManagerConnected(
content::ServiceManagerConnection* connection) {
memlog_client_.OnServiceManagerConnected(connection);
static base::LazyInstance<profiling::MemlogClient>::Leaky memlog_client =
LAZY_INSTANCE_INITIALIZER;
memlog_client.Get().OnServiceManagerConnected(connection);
}
2 changes: 0 additions & 2 deletions chrome/common/chrome_content_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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<ChromeOriginTrialPolicy> origin_trial_policy_;
profiling::MemlogClient memlog_client_;
};

#endif // CHROME_COMMON_CHROME_CONTENT_CLIENT_H_
2 changes: 2 additions & 0 deletions chrome/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ if (!is_android) {

public_deps = [
"//chrome/browser:test_support_ui",
"//content/public/browser",
]
deps = [
"//components/metrics:test_support",
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 69cf473

Please sign in to comment.