Reland: Redesign ATrace integration

(Reland of https://chromium-review.googlesource.com/c/chromium/src/+/2332664)

Chrome and WebView have supported basic Android ATrace tracing, but the
current approach has some shortcomings:

1) Android R changes the way atrace session activations are broadcast,
   breaking the code in WebView listening for activations
   (OnTraceEnabledChangeListener). This means you can't trace a running
   WebView app with atrace.

2) Neither Chrome nor WebView record early startup events to atrace,
   which means all events before the native library has loaded are
   lost.

3) It's not possible to specify trace categories via atrace, which means
   we need more cumbersome alternative solutions (i.e., command line
   flags) for startup tracing.

4) Writing ATrace events is only supported in WebView.

This patch reworks the ATrace integration to resolve these problems and
to align the Chrome and WebView implementations. In short, ATrace
session management is moved to the common (TraceEvent) layer, and Chrome
and WebView only differ by which trace tags they listen to (APP vs.
WEBVIEW).

We also add a way to declare trace categories through per-app tags:

$ atrace -a org.chromium.chrome,org.chromium.chrome/<category_filter>

(Note that the plain package name without any category filters must
always appear in the list on its own.)

Multiple categories can be separated with a double colon:

$ atrace -a org.chromium.chrome,org.chromium.chrome/cat1:cat2

Or by specifying the the app several times to get around the 91
character limit for each entry:

$ atrace -a org.chromium.chrome,\
            org.chromium.chrome/cat1,\
            org.chromium.chrome/cat2

Finally, to capture Java startup events into a tracing session
controlled by the system's Perfetto service instead of atrace, a
special "-atrace" category can be used to only write events into
Chrome's own tracing service instead of atrace. This way when we
connect to Perfetto later in the startup sequence and establish
the real tracing session, startup-related events can be flushed
into that session without emitting duplicate events into ATrace.

TEST=atrace -a org.chromium.chrome

TEST=atrace webview

TEST=atrace -a org.chromium.chrome

TEST=atrace webview

TEST=atrace -a org.chromium.chrome,org.chromium.chrome/-*:cc

TEST=atrace -a com.google.android.gm,com.google.android.gm/-*:cc webview

TEST=atrace -a org.chromium.chrome,org.chromium.chrome/-atrace

Bug: 1095587, b/160768681
Change-Id: Ifc1488cc1a045bb79a804f13596935234fc7a163
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2364635
Commit-Queue: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800611}

android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumAwInit.java

@@ -16,8 +16,6 @@ import android.webkit.GeolocationPermissions;
 import android.webkit.WebStorage;
 import android.webkit.WebViewDatabase;
 
-import com.android.webview.chromium.WebViewDelegateFactory.WebViewDelegate;
-
 import org.chromium.android_webview.AwBrowserContext;
 import org.chromium.android_webview.AwBrowserProcess;
 import org.chromium.android_webview.AwContents;
@@ -97,6 +95,8 @@ public class WebViewChromiumAwInit {
         mFactory = factory;
         // Do not make calls into 'factory' in this ctor - this ctor is called from the
         // WebViewChromiumFactoryProvider ctor, so 'factory' is not properly initialized yet.
+        TraceEvent.maybeEnableEarlyTracing(
+                TraceEvent.ATRACE_TAG_WEBVIEW, /*readCommandLine=*/false);
     }
 
     public AwTracingController getAwTracingController() {
@@ -126,7 +126,6 @@ public class WebViewChromiumAwInit {
     protected void startChromiumLocked() {
         try (ScopedSysTraceEvent event =
                         ScopedSysTraceEvent.scoped("WebViewChromiumAwInit.startChromiumLocked")) {
-            TraceEvent.setATraceEnabled(mFactory.getWebViewDelegate().isTraceTagEnabled());
             assert Thread.holdsLock(mLock) && ThreadUtils.runningOnUiThread();
 
             // The post-condition of this method is everything is ready, so notify now to cover all
@@ -178,14 +177,6 @@ public class WebViewChromiumAwInit {
                 mSharedStatics.setWebContentsDebuggingEnabledUnconditionally(true);
             }
 
-            mFactory.getWebViewDelegate().setOnTraceEnabledChangeListener(
-                    new WebViewDelegate.OnTraceEnabledChangeListener() {
-                        @Override
-                        public void onTraceEnabledChange(boolean enabled) {
-                            TraceEvent.setATraceEnabled(enabled);
-                        }
-                    });
-
             mStarted = true;
 
             RecordHistogram.recordSparseHistogram("Android.WebView.TargetSdkVersion",

android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java

@@ -11,7 +11,6 @@ import android.content.pm.PackageInfo;
 import android.content.res.AssetManager;
 import android.content.res.Resources;
 import android.graphics.Canvas;
-import android.os.Trace;
 import android.util.SparseArray;
 import android.view.View;
 
@@ -41,17 +40,6 @@ class WebViewDelegateFactory {
      * See {@link WebViewDelegateFactory} for the reasons why this copy is needed.
      */
     interface WebViewDelegate extends AwDrawFnImpl.DrawFnAccess {
-        /** @see android.webkit.WebViewDelegate.OnTraceEnabledChangeListener */
-        interface OnTraceEnabledChangeListener {
-            void onTraceEnabledChange(boolean enabled);
-        }
-
-        /** @see android.webkit.WebViewDelegate#setOnTraceEnabledChangeListener */
-        void setOnTraceEnabledChangeListener(final OnTraceEnabledChangeListener listener);
-
-        /** @see android.webkit.WebViewDelegate#isTraceTagEnabled */
-        boolean isTraceTagEnabled();
-
         /** @see android.webkit.WebViewDelegate#canInvokeDrawGlFunctor */
         boolean canInvokeDrawGlFunctor(View containerView);
 
@@ -123,22 +111,6 @@ class WebViewDelegateFactory {
             mDelegate = delegate;
         }
 
-        @Override
-        public void setOnTraceEnabledChangeListener(final OnTraceEnabledChangeListener listener) {
-            mDelegate.setOnTraceEnabledChangeListener(
-                    new android.webkit.WebViewDelegate.OnTraceEnabledChangeListener() {
-                        @Override
-                        public void onTraceEnabledChange(boolean enabled) {
-                            listener.onTraceEnabledChange(enabled);
-                        }
-                    });
-        }
-
-        @Override
-        public boolean isTraceTagEnabled() {
-            return mDelegate.isTraceTagEnabled();
-        }
-
         @Override
         public boolean canInvokeDrawGlFunctor(View containerView) {
             return mDelegate.canInvokeDrawGlFunctor(containerView);
@@ -234,12 +206,7 @@ class WebViewDelegateFactory {
      * framework.
      */
     private static class Api21CompatibilityDelegate implements WebViewDelegate {
-        /** Copy of Trace.TRACE_TAG_WEBVIEW */
-        private static final long TRACE_TAG_WEBVIEW = 1L << 4;
-
         /** Hidden APIs released in the API 21 version of the framework */
-        private final Method mIsTagEnabledMethod;
-        private final Method mAddChangeCallbackMethod;
         private final Method mGetViewRootImplMethod;
         private final Method mInvokeFunctorMethod;
         private final Method mCallDrawGLFunctionMethod;
@@ -255,9 +222,6 @@ class WebViewDelegateFactory {
                 // Important: This reflection essentially defines a snapshot of some hidden APIs
                 // at version 21 of the framework for compatibility reasons, and the reflection
                 // should not be changed even if those hidden APIs change in future releases.
-                mIsTagEnabledMethod = Trace.class.getMethod("isTagEnabled", long.class);
-                mAddChangeCallbackMethod = Class.forName("android.os.SystemProperties")
-                                                   .getMethod("addChangeCallback", Runnable.class);
                 mGetViewRootImplMethod = View.class.getMethod("getViewRootImpl");
                 mInvokeFunctorMethod =
                         Class.forName("android.view.ViewRootImpl")
@@ -280,29 +244,6 @@ class WebViewDelegateFactory {
             }
         }
 
-        @Override
-        public void setOnTraceEnabledChangeListener(final OnTraceEnabledChangeListener listener) {
-            try {
-                mAddChangeCallbackMethod.invoke(null, new Runnable() {
-                    @Override
-                    public void run() {
-                        listener.onTraceEnabledChange(isTraceTagEnabled());
-                    }
-                });
-            } catch (Exception e) {
-                throw new RuntimeException("Invalid reflection", e);
-            }
-        }
-
-        @Override
-        public boolean isTraceTagEnabled() {
-            try {
-                return ((Boolean) mIsTagEnabledMethod.invoke(null, TRACE_TAG_WEBVIEW));
-            } catch (Exception e) {
-                throw new RuntimeException("Invalid reflection", e);
-            }
-        }
-
         @Override
         public boolean canInvokeDrawGlFunctor(View containerView) {
             try {

android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellActivity.java

@@ -71,10 +71,12 @@ public class AwShellActivity extends Activity {
 
         AwBrowserProcess.loadLibrary(null);
 
+        // This flag is deprecated. Print a hint instead.
         if (CommandLine.getInstance().hasSwitch(AwShellSwitches.ENABLE_ATRACE)) {
-            Log.e(TAG, "Enabling Android trace.");
-            TraceEvent.setATraceEnabled(true);
+            Log.e(TAG, "To trace the test shell, run \"atrace webview\"");
         }
+        TraceEvent.maybeEnableEarlyTracing(
+                TraceEvent.ATRACE_TAG_WEBVIEW, /*readCommandLine=*/false);
 
         setContentView(R.layout.testshell_activity);
 

android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellSwitches.java

@@ -9,7 +9,7 @@ package org.chromium.android_webview.shell;
  * the android_webview glue layer.
  */
 public abstract class AwShellSwitches {
-    // Enables Android systrace path for Chrome traces.
+    // Deprecated: instead, run "atrace webview".
     public static final String ENABLE_ATRACE = "enable-atrace";
 
     // Prevent instantiation.

base/android/java/src/org/chromium/base/EarlyTraceEvent.java

@@ -148,7 +148,6 @@ public class EarlyTraceEvent {
         if (shouldEnable) enable();
     }
 
-    @VisibleForTesting
     static void enable() {
         synchronized (sLock) {
             if (sState != STATE_DISABLED) return;

base/android/java/src/org/chromium/base/ThreadUtils.java

@@ -118,18 +118,24 @@ public class ThreadUtils {
                 sUiThreadHandler = new Handler(looper);
             }
         }
+        TraceEvent.onUiThreadReady();
     }
 
     public static Handler getUiThreadHandler() {
+        boolean createdHandler = false;
         synchronized (sLock) {
             if (sUiThreadHandler == null) {
                 if (sWillOverride) {
                     throw new RuntimeException("Did not yet override the UI thread");
                 }
                 sUiThreadHandler = new Handler(Looper.getMainLooper());
+                createdHandler = true;
             }
-            return sUiThreadHandler;
         }
+        if (createdHandler) {
+            TraceEvent.onUiThreadReady();
+        }
+        return sUiThreadHandler;
     }
 
     /**

base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java

@@ -633,7 +633,7 @@ public class LibraryLoader {
         UmaRecorderHolder.onLibraryLoaded();
 
         // From now on, keep tracing in sync with native.
-        TraceEvent.registerNativeEnabledObserver();
+        TraceEvent.onNativeTracingReady();
 
         // From this point on, native code is ready to use, but non-MainDex JNI may not yet have
         // been registered. Check isInitialized() to be sure that initialization is fully complete.

base/android/trace_event_binding.cc

@@ -49,22 +49,39 @@ static void JNI_TraceEvent_RegisterEnabledObserver(JNIEnv* env) {
       std::make_unique<TraceEnabledObserver>());
 }
 
-static void JNI_TraceEvent_StartATrace(JNIEnv* env) {
-  base::trace_event::TraceLog::GetInstance()->StartATrace();
+static void JNI_TraceEvent_StartATrace(
+    JNIEnv* env,
+    const JavaParamRef<jstring>& category_filter) {
+  std::string category_filter_utf8 =
+      ConvertJavaStringToUTF8(env, category_filter);
+  base::trace_event::TraceLog::GetInstance()->StartATrace(category_filter_utf8);
 }
 
 static void JNI_TraceEvent_StopATrace(JNIEnv* env) {
   base::trace_event::TraceLog::GetInstance()->StopATrace();
 }
 
+static void JNI_TraceEvent_SetupATraceStartupTrace(
+    JNIEnv* env,
+    const JavaParamRef<jstring>& category_filter) {
+  std::string category_filter_utf8 =
+      ConvertJavaStringToUTF8(env, category_filter);
+  base::trace_event::TraceLog::GetInstance()->SetupATraceStartupTrace(
+      category_filter_utf8);
+}
+
 #else  // BUILDFLAG(ENABLE_BASE_TRACING)
 
 // Empty implementations when TraceLog isn't available.
 static void JNI_TraceEvent_RegisterEnabledObserver(JNIEnv* env) {
   base::android::Java_TraceEvent_setEnabled(env, false);
 }
-static void JNI_TraceEvent_StartATrace(JNIEnv* env) {}
+static void JNI_TraceEvent_StartATrace(JNIEnv* env,
+                                       base::android::JavaParamRef<jstring>&) {}
 static void JNI_TraceEvent_StopATrace(JNIEnv* env) {}
+static void JNI_TraceEvent_SetupATraceStartupTrace(
+    JNIEnv* env,
+    base::android::JavaParamRef<jstring>&) {}
 
 #endif  // BUILDFLAG(ENABLE_BASE_TRACING)
 

base/trace_event/trace_event_android.cc

@@ -34,7 +34,10 @@ void WriteToATrace(int fd, const char* buffer, size_t size) {
       break;
     total_written += written;
   }
-  if (total_written < size) {
+  // Tracing might have been disabled before we were notified about it, which
+  // triggers EBADF. Since enabling and disabling atrace is racy, ignore the
+  // error in that case to avoid logging an error for every trace event.
+  if (total_written < size && errno != EBADF) {
     PLOG(WARNING) << "Failed to write buffer '" << std::string(buffer, size)
                   << "' to " << kATraceMarkerFile;
   }
@@ -73,20 +76,6 @@ void WriteEvent(char phase,
   WriteToATrace(g_atrace_fd, out.c_str(), out.size());
 }
 
-void NoOpOutputCallback(WaitableEvent* complete_event,
-                        const scoped_refptr<RefCountedString>&,
-                        bool has_more_events) {
-  if (!has_more_events)
-    complete_event->Signal();
-}
-
-void EndChromeTracing(TraceLog* trace_log,
-                      WaitableEvent* complete_event) {
-  trace_log->SetDisabled();
-  // Delete the buffered trace events as they have been sent to atrace.
-  trace_log->Flush(BindRepeating(&NoOpOutputCallback, complete_event));
-}
-
 }  // namespace
 
 // These functions support Android systrace.py when 'webview' category is
@@ -99,7 +88,7 @@ void EndChromeTracing(TraceLog* trace_log,
 //   StartATrace, StopATrace and SendToATrace, and perhaps send Java traces
 //   directly to atrace in trace_event_binding.cc.
 
-void TraceLog::StartATrace() {
+void TraceLog::StartATrace(const std::string& category_filter) {
   if (g_atrace_fd != -1)
     return;
 
@@ -108,28 +97,17 @@ void TraceLog::StartATrace() {
     PLOG(WARNING) << "Couldn't open " << kATraceMarkerFile;
     return;
   }
-  TraceConfig trace_config;
+  TraceConfig trace_config(category_filter);
   trace_config.SetTraceRecordMode(RECORD_CONTINUOUSLY);
   SetEnabled(trace_config, TraceLog::RECORDING_MODE);
 }
 
 void TraceLog::StopATrace() {
-  if (g_atrace_fd == -1)
-    return;
-
-  close(g_atrace_fd);
-  g_atrace_fd = -1;
-
-  // TraceLog::Flush() requires the current thread to have a message loop, but
-  // this thread called from Java may not have one, so flush in another thread.
-  Thread end_chrome_tracing_thread("end_chrome_tracing");
-  WaitableEvent complete_event(WaitableEvent::ResetPolicy::AUTOMATIC,
-                               WaitableEvent::InitialState::NOT_SIGNALED);
-  end_chrome_tracing_thread.Start();
-  end_chrome_tracing_thread.task_runner()->PostTask(
-      FROM_HERE, base::BindOnce(&EndChromeTracing, Unretained(this),
-                                Unretained(&complete_event)));
-  complete_event.Wait();
+  if (g_atrace_fd != -1) {
+    close(g_atrace_fd);
+    g_atrace_fd = -1;
+  }
+  SetDisabled();
 }
 
 void TraceEvent::SendToATrace() {
@@ -198,5 +176,13 @@ void TraceLog::AddClockSyncMetadataEvent() {
   close(atrace_fd);
 }
 
+void TraceLog::SetupATraceStartupTrace(const std::string& category_filter) {
+  atrace_startup_config_ = TraceConfig(category_filter);
+}
+
+Optional<TraceConfig> TraceLog::TakeATraceStartupConfig() {
+  return std::move(atrace_startup_config_);
+}
+
 }  // namespace trace_event
 }  // namespace base

base/trace_event/trace_event_android_unittest.cc

@@ -12,11 +12,20 @@ namespace trace_event {
 TEST(TraceEventAndroidTest, WriteToATrace) {
   // Just a smoke test to ensure no crash.
   TraceLog* trace_log = TraceLog::GetInstance();
-  trace_log->StartATrace();
+  trace_log->StartATrace("test");
   TRACE_EVENT0("test", "test-event");
   trace_log->StopATrace();
   trace_log->AddClockSyncMetadataEvent();
 }
 
+TEST(TraceEventAndroidTest, ATraceStartup) {
+  TraceLog* trace_log = TraceLog::GetInstance();
+  EXPECT_FALSE(trace_log->TakeATraceStartupConfig());
+  trace_log->SetupATraceStartupTrace("cat");
+  auto config = trace_log->TakeATraceStartupConfig();
+  EXPECT_TRUE(config);
+  EXPECT_TRUE(config->IsCategoryGroupEnabled("cat"));
+}
+
 }  // namespace trace_event
 }  // namespace base

base/trace_event/trace_log.h

@@ -19,6 +19,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/optional.h"
 #include "base/single_thread_task_runner.h"
 #include "base/time/time_override.h"
 #include "base/trace_event/category_registry.h"
@@ -106,10 +107,12 @@ class BASE_EXPORT TraceLog : public MemoryDumpProvider {
   int GetNumTracesRecorded();
 
 #if defined(OS_ANDROID)
-  void StartATrace();
+  void StartATrace(const std::string& category_filter);
   void StopATrace();
   void AddClockSyncMetadataEvent();
-#endif
+  void SetupATraceStartupTrace(const std::string& category_filter);
+  Optional<TraceConfig> TakeATraceStartupConfig();
+#endif  // defined(OS_ANDROID)
 
   // Enabled state listeners give a callback when tracing is enabled or
   // disabled. This can be used to tie into other library's tracing systems
@@ -576,6 +579,10 @@ class BASE_EXPORT TraceLog : public MemoryDumpProvider {
 
   FilterFactoryForTesting filter_factory_for_testing_;
 
+#if defined(OS_ANDROID)
+  base::Optional<TraceConfig> atrace_startup_config_;
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(TraceLog);
 };
 

chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java

@@ -7,6 +7,7 @@ package org.chromium.chrome.browser;
 import android.app.Application;
 import android.content.Context;
 import android.content.Intent;
+import android.content.pm.ApplicationInfo;
 import android.content.res.Configuration;
 import android.os.Bundle;
 
@@ -92,8 +93,14 @@ public class ChromeApplication extends Application {
             CommandLineInitUtil.initCommandLine(
                     COMMAND_LINE_FILE, ChromeApplication::shouldUseDebugFlags);
 
+            // Enable ATrace on debug OS or app builds.
+            int applicationFlags = context.getApplicationInfo().flags;
+            boolean isAppDebuggable = (applicationFlags & ApplicationInfo.FLAG_DEBUGGABLE) != 0;
+            boolean isOsDebuggable = BuildInfo.isDebugAndroid();
             // Requires command-line flags.
-            TraceEvent.maybeEnableEarlyTracing();
+            TraceEvent.maybeEnableEarlyTracing(
+                    (isAppDebuggable || isOsDebuggable) ? TraceEvent.ATRACE_TAG_APP : 0,
+                    /*readCommandLine=*/true);
             TraceEvent.begin("ChromeApplication.attachBaseContext");
 
             // Register for activity lifecycle callbacks. Must be done before any activities are

components/tracing/common/trace_startup_config.cc

@@ -15,6 +15,7 @@
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/trace_event/trace_log.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "components/tracing/common/tracing_switches.h"
@@ -95,6 +96,10 @@ TraceStartupConfig::TraceStartupConfig() {
     DCHECK(!IsTracingStartupForDuration());
     DCHECK_EQ(SessionOwner::kBackgroundTracing, session_owner_);
     CHECK(!ShouldTraceToResultFile());
+  } else if (EnableFromATrace()) {
+    DCHECK(IsEnabled());
+    DCHECK_EQ(SessionOwner::kSystemTracing, session_owner_);
+    CHECK(!ShouldTraceToResultFile());
   }
 }
 
@@ -196,6 +201,24 @@ bool TraceStartupConfig::EnableFromCommandLine() {
   return true;
 }
 
+bool TraceStartupConfig::EnableFromATrace() {
+#if defined(OS_ANDROID)
+  auto atrace_config =
+      base::trace_event::TraceLog::GetInstance()->TakeATraceStartupConfig();
+  if (!atrace_config)
+    return false;
+  trace_config_ = *atrace_config;
+  is_enabled_ = true;
+  // We only support ATrace-initiated startup tracing together with the system
+  // service, because DevTools and background tracing generally use Chrome
+  // command line flags to control startup tracing instead of ATrace.
+  session_owner_ = SessionOwner::kSystemTracing;
+  return true;
+#else   // defined(OS_ANDROID)
+  return false;
+#endif  // !defined(OS_ANDROID)
+}
+
 bool TraceStartupConfig::EnableFromConfigFile() {
 #if defined(OS_ANDROID)
   base::FilePath trace_config_file(kAndroidTraceConfigFile);

components/tracing/common/trace_startup_config.h

@@ -157,6 +157,7 @@ class TRACING_EXPORT TraceStartupConfig {
   bool EnableFromCommandLine();
   bool EnableFromConfigFile();
   bool EnableFromBackgroundTracing();
+  bool EnableFromATrace();
 
   bool ParseTraceConfigFileContent(const std::string& content);