Revert "[Clank SSM]: Enable stack sampling in android browsertests."

This reverts commit ec645d5. 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 <wez@chromium.org> > Reviewed-by: Mike Wittman <wittman@chromium.org> > Reviewed-by: Dirk Pranke <dpranke@google.com> > Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org> > 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 <oysteine@chromium.org> Commit-Queue: oysteine <oysteine@chromium.org> Cr-Commit-Position: refs/heads/master@{#789554}
FairyWorld · Jul 17, 2020 · d680e1c · d680e1c
1 parent db99b0a
commit d680e1c
Show file tree

Hide file tree

Showing 7 changed files with 22 additions and 74 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -37,6 +37,17 @@ namespace {
 base::LazyInstance<StackSamplingConfiguration>::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
@@ -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",