From d680e1c25d943c15ab3a5e35a122ab35822ccc4d Mon Sep 17 00:00:00 2001 From: oysteine Date: Fri, 17 Jul 2020 18:27:49 +0000 Subject: [PATCH] Revert "[Clank SSM]: Enable stack sampling in android browsertests." This reverts commit ec645d523a7b97910d9d3622576381829467a869. Reason for revert: crbug.com/1106867 Original change's description: > [Clank SSM]: Enable stack sampling in android browsertests. > > android_browsertests must depend on libunwindstack directly instead of > through DFM, since loading of a partitioned library is not supported in > an APK. > (internal discussion) > https://groups.google.com/a/google.com/g/clank-components/c/ktt285Gtax4/m/yRy8qm8LAQAJ > > This CL also re-enables tracing use of StackSampingProfiler, which > was accidentally disabled in https://crrev.com/784147? > > Bug: 1004855 > Change-Id: I5967bf3815008843ba8a77e926977e39a4ccfbd8 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2293071 > Reviewed-by: Wez > Reviewed-by: Mike Wittman > Reviewed-by: Dirk Pranke > Commit-Queue: Etienne Pierre-Doray > Cr-Commit-Position: refs/heads/master@{#789530} TBR=wez@chromium.org,wittman@chromium.org,dpranke@google.com,etiennep@chromium.org Change-Id: I61309dcf5ceb5591dbf9f40e506ee25ee2ac475c No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 1004855 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2304932 Reviewed-by: oysteine Commit-Queue: oysteine Cr-Commit-Position: refs/heads/master@{#789554} --- base/BUILD.gn | 5 ++++ base/profiler/stack_sampling_profiler.cc | 5 ++-- build/config/compiler/compiler.gni | 5 ---- .../modules/stack_unwinder/internal/BUILD.gn | 13 --------- .../profiler/stack_sampling_browsertest.cc | 21 +++------------ .../profiler/stack_sampling_configuration.cc | 27 ++++++++----------- chrome/test/BUILD.gn | 20 -------------- 7 files changed, 22 insertions(+), 74 deletions(-) diff --git a/base/BUILD.gn b/base/BUILD.gn index f623d6e6d00585..f9e4c5bb72b69b 100644 --- a/base/BUILD.gn +++ b/base/BUILD.gn @@ -91,6 +91,11 @@ dep_libevent = # Determines whether message_pump_libevent should be used. use_libevent = dep_libevent && !is_ios +# Whether or not cfi table should be enabled on arm. +# TODO(crbug.com/1090409): Replace can_unwind_with_cfi_table once sampling +# profiler is enabled on android. +enable_arm_cfi_table = is_android && !is_component_build && current_cpu == "arm" + if (is_android) { import("//build/config/android/rules.gni") } diff --git a/base/profiler/stack_sampling_profiler.cc b/base/profiler/stack_sampling_profiler.cc index 5ffe697b75f3eb..15fbfbd6812c3e 100644 --- a/base/profiler/stack_sampling_profiler.cc +++ b/base/profiler/stack_sampling_profiler.cc @@ -742,9 +742,8 @@ TimeTicks StackSamplingProfiler::TestPeer::GetNextSampleTime( // The profiler is currently only implemented for Windows x64 and MacOSX. // TODO(https://crbug.com/1004855): enable for Android arm. bool StackSamplingProfiler::IsSupported() { -#if (defined(OS_WIN) && defined(ARCH_CPU_X86_64)) || \ - (defined(OS_MACOSX) && defined(ARCH_CPU_X86_64) && !defined(OS_IOS)) || \ - (defined(OS_ANDROID) && BUILDFLAG(ENABLE_ARM_CFI_TABLE)) +#if (defined(OS_WIN) && defined(ARCH_CPU_X86_64)) || \ + (defined(OS_MACOSX) && defined(ARCH_CPU_X86_64) && !defined(OS_IOS)) #if defined(OS_MACOSX) // TODO(https://crbug.com/1098119): Fix unwinding on OS X 10.16. The OS // has moved all system libraries into the dyld shared cache and this diff --git a/build/config/compiler/compiler.gni b/build/config/compiler/compiler.gni index 54cabe66a9de57..0aa9e4759589dd 100644 --- a/build/config/compiler/compiler.gni +++ b/build/config/compiler/compiler.gni @@ -172,11 +172,6 @@ assert(!can_unwind_with_frame_pointers || enable_frame_pointers) can_unwind_with_cfi_table = is_android && !is_component_build && !enable_frame_pointers && current_cpu == "arm" -# Whether or not cfi table should be enabled on arm. -# TODO(crbug.com/1090409): Replace can_unwind_with_cfi_table with this once -# sampling profiler is enabled on android. -enable_arm_cfi_table = is_android && !is_component_build && current_cpu == "arm" - declare_args() { # Set to true to use lld, the LLVM linker. use_lld = is_clang && (!is_ios && !is_mac) diff --git a/chrome/android/modules/stack_unwinder/internal/BUILD.gn b/chrome/android/modules/stack_unwinder/internal/BUILD.gn index 3707a5fa1bb0fc..acbf5c2329d16d 100644 --- a/chrome/android/modules/stack_unwinder/internal/BUILD.gn +++ b/chrome/android/modules/stack_unwinder/internal/BUILD.gn @@ -52,16 +52,3 @@ component("stack_unwinder") { cflags = [ "-fsymbol-partition=stack_unwinder_partition" ] } } - -# Since loading of a partitioned library is not supported in an APK, a separate -# target without partition is necessary for testing. -source_set("stack_unwinder_for_testing") { - testonly = true - sources = [ "stack_unwinder_module_contents_impl.cc" ] - deps = [ - ":jni_headers", - "//base", - "//base:native_unwinder_android", - "//chrome/android/features/stack_unwinder/public:native", - ] -} diff --git a/chrome/common/profiler/stack_sampling_browsertest.cc b/chrome/common/profiler/stack_sampling_browsertest.cc index 2618753f743b97..36ac29b9e0a0aa 100644 --- a/chrome/common/profiler/stack_sampling_browsertest.cc +++ b/chrome/common/profiler/stack_sampling_browsertest.cc @@ -11,20 +11,14 @@ #include "base/test/scoped_run_loop_timeout.h" #include "base/thread_annotations.h" #include "base/threading/platform_thread.h" -#include "build/build_config.h" #include "chrome/common/channel_info.h" #include "chrome/common/chrome_switches.h" +#include "chrome/test/base/in_process_browser_test.h" #include "components/metrics/call_stack_profile_metrics_provider.h" #include "components/version_info/channel.h" #include "content/public/test/browser_test.h" #include "third_party/metrics_proto/sampled_profile.pb.h" -#if defined(OS_ANDROID) -#include "chrome/test/base/android/android_browser_test.h" -#else -#include "chrome/test/base/in_process_browser_test.h" -#endif - namespace { // Class that intercepts and stores profiles provided to the @@ -101,7 +95,7 @@ bool MatchesProfile(metrics::SampledProfile::TriggerEvent trigger_event, profile.process() == process && profile.thread() == thread; } -class StackSamplingBrowserTest : public PlatformBrowserTest { +class StackSamplingBrowserTest : public InProcessBrowserTest { public: void SetUp() override { // Arrange to intercept the CPU profiles at the time they're provided to the @@ -110,7 +104,7 @@ class StackSamplingBrowserTest : public PlatformBrowserTest { SetCpuInterceptorCallbackForTesting(base::BindRepeating( &ProfileInterceptor::Intercept, base::Unretained(&ProfileInterceptor::GetInstance()))); - PlatformBrowserTest::SetUp(); + InProcessBrowserTest::SetUp(); } void SetUpCommandLine(base::CommandLine* command_line) override { @@ -196,15 +190,8 @@ IN_PROC_BROWSER_TEST_F(StackSamplingBrowserTest, metrics::COMPOSITOR_THREAD)); } -// Android doesn't have a network service process. -#if defined(OS_ANDROID) -#define MAYBE_NetworkServiceProcessIOThread \ - DISABLED_NetworkServiceProcessIOThread -#else -#define MAYBE_NetworkServiceProcessIOThread NetworkServiceProcessIOThread -#endif IN_PROC_BROWSER_TEST_F(StackSamplingBrowserTest, - MAYBE_NetworkServiceProcessIOThread) { + NetworkServiceProcessIOThread) { EXPECT_TRUE(WaitForProfile(metrics::SampledProfile::PROCESS_STARTUP, metrics::NETWORK_SERVICE_PROCESS, metrics::IO_THREAD)); diff --git a/chrome/common/profiler/stack_sampling_configuration.cc b/chrome/common/profiler/stack_sampling_configuration.cc index ab723cd55820b1..eb790d086b98c5 100644 --- a/chrome/common/profiler/stack_sampling_configuration.cc +++ b/chrome/common/profiler/stack_sampling_configuration.cc @@ -37,6 +37,17 @@ namespace { base::LazyInstance::Leaky g_configuration = LAZY_INSTANCE_INITIALIZER; +bool IsProfilerEnabledForChannel() { +#if BUILDFLAG(GOOGLE_CHROME_BRANDING) + // Only run on canary and dev. + const version_info::Channel channel = chrome::GetChannel(); + return channel == version_info::Channel::CANARY || + channel == version_info::Channel::DEV; +#else + return true; +#endif +} + // Returns true if the current execution is taking place in the browser process. bool IsBrowserProcess() { const base::CommandLine* command_line = @@ -66,22 +77,6 @@ bool IsBrowserTestModeEnabled() { switches::kStartStackProfilerBrowserTest; } -bool IsProfilerEnabledForChannel() { -#if defined(OS_ANDROID) - // Profiling is only enable in it's own dedicated browser tests on Android. - // TODO(crbug.com/1004855): Remove this logic to launch profiler. - return IsBrowserTestModeEnabled(); -#endif -#if BUILDFLAG(GOOGLE_CHROME_BRANDING) - // Only run on canary and dev. - const version_info::Channel channel = chrome::GetChannel(); - return channel == version_info::Channel::CANARY || - channel == version_info::Channel::DEV; -#else - return true; -#endif -} - bool ShouldEnableProfilerForNextRendererProcess() { // Ensure deterministic behavior for testing the profiler itself. if (IsBrowserTestModeEnabled()) diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index b2ed29445d464c..070e98d76cfc64 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn @@ -499,26 +499,6 @@ if (is_android) { "base/android/android_browser_test_browsertest_android.cc", ] - # Add unwind tables in android_browsertests apk to support enabling - # sampling profiler. The unwind tables are generated from debug info in the - # binary. Removing "default_symbols" and adding symbols config removes the - # "strip_debug" config that strips the debug info, on android_browsertests - # apk. - if (enable_arm_cfi_table) { - sources += [ "../common/profiler/stack_sampling_browsertest.cc" ] - deps += [ - "//chrome/android/modules/stack_unwinder/internal:java", - "//chrome/android/modules/stack_unwinder/internal:stack_unwinder_for_testing", - ] - configs -= [ "//build/config/compiler:default_symbols" ] - if (symbol_level == 2) { - configs += [ "//build/config/compiler:symbols" ] - } else { - configs += [ "//build/config/compiler:minimal_symbols" ] - } - add_unwind_tables_in_apk = true - } - data = [ "$root_gen_dir/chrome/android/chrome_apk_paks/chrome_100_percent.pak", "$root_gen_dir/chrome/android/chrome_apk_paks/locales/en-US.pak",