Revert "Redesign ATrace integration"

This reverts commit 80207c25.

Reason for revert: Flaking tests, see crbug.com/1117683

Original change's description:
> Redesign ATrace integration
>
> 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.
>
> # Basic trace while Chrome is running. Generates ATrace with Chrome
> # events.
> TEST=atrace -a org.chromium.chrome
>
> # Ditto for WebView.
> TEST=atrace webview
>
> # Startup trace -- open Chrome after running this. Results in an ATrace
> # with Chrome pre- and post-startup events.
> TEST=atrace -a org.chromium.chrome
>
> # Ditto for WebView.
> TEST=atrace webview
>
> # Trace with custom categories. Results in a ATrace with only
> # "cc" events from Chrome.
> TEST=atrace -a org.chromium.chrome,org.chromium.chrome/-*:cc
>
> # Ditto for WebView, using GMail as a test app.
> TEST=atrace -a com.google.android.gm,com.google.android.gm/-*:cc webview
>
> # Chrome-only trace. No Chrome events written in the resulting ATrace.
> TEST=atrace -a org.chromium.chrome,org.chromium.chrome/-atrace
>
> Bug: 1095587, b/160768681
> Change-Id: Ia0bee252ce398804c00be8a4179241b75479bf5e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2332664
> Reviewed-by: Bo <boliu@chromium.org>
> Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> Reviewed-by: Eric Seckler <eseckler@chromium.org>
> Commit-Queue: Bo <boliu@chromium.org>
> Auto-Submit: Sami Kyöstilä <skyostil@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#798791}

TBR=boliu@chromium.org,skyostil@chromium.org,agrieve@chromium.org,eseckler@chromium.org,nuskos@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1095587, 1117683
Bug: b/160768681
Change-Id: Iffcaebb70c6eb92da1cb3080e8034ea5c36e5394
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2363564
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799456}

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

@@ -16,6 +16,8 @@ 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;
@@ -95,8 +97,6 @@ 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,6 +126,7 @@ 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
@@ -177,6 +178,14 @@ 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,6 +11,7 @@ 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;
 
@@ -40,6 +41,17 @@ 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);
 
@@ -111,6 +123,22 @@ 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);
@@ -206,7 +234,12 @@ 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;
@@ -222,6 +255,9 @@ 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")
@@ -244,6 +280,29 @@ 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,12 +71,10 @@ 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, "To trace the test shell, run \"atrace webview\"");
+            Log.e(TAG, "Enabling Android trace.");
+            TraceEvent.setATraceEnabled(true);
         }
-        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 {
-    // Deprecated: instead, run "atrace webview".
+    // Enables Android systrace path for Chrome traces.
     public static final String ENABLE_ATRACE = "enable-atrace";
 
     // Prevent instantiation.

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

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

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.onNativeTracingReady();
+        TraceEvent.registerNativeEnabledObserver();
 
         // 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,39 +49,22 @@ static void JNI_TraceEvent_RegisterEnabledObserver(JNIEnv* env) {
       std::make_unique<TraceEnabledObserver>());
 }
 
-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_StartATrace(JNIEnv* env) {
+  base::trace_event::TraceLog::GetInstance()->StartATrace();
 }
 
 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,
-                                       base::android::JavaParamRef<jstring>&) {}
+static void JNI_TraceEvent_StartATrace(JNIEnv* env) {}
 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,10 +34,7 @@ void WriteToATrace(int fd, const char* buffer, size_t size) {
       break;
     total_written += written;
   }
-  // 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) {
+  if (total_written < size) {
     PLOG(WARNING) << "Failed to write buffer '" << std::string(buffer, size)
                   << "' to " << kATraceMarkerFile;
   }
@@ -76,6 +73,20 @@ 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
@@ -88,7 +99,7 @@ void WriteEvent(char phase,
 //   StartATrace, StopATrace and SendToATrace, and perhaps send Java traces
 //   directly to atrace in trace_event_binding.cc.
 
-void TraceLog::StartATrace(const std::string& category_filter) {
+void TraceLog::StartATrace() {
   if (g_atrace_fd != -1)
     return;
 
@@ -97,17 +108,28 @@ void TraceLog::StartATrace(const std::string& category_filter) {
     PLOG(WARNING) << "Couldn't open " << kATraceMarkerFile;
     return;
   }
-  TraceConfig trace_config(category_filter);
+  TraceConfig trace_config;
   trace_config.SetTraceRecordMode(RECORD_CONTINUOUSLY);
   SetEnabled(trace_config, TraceLog::RECORDING_MODE);
 }
 
 void TraceLog::StopATrace() {
-  if (g_atrace_fd != -1) {
-    close(g_atrace_fd);
-    g_atrace_fd = -1;
-  }
-  SetDisabled();
+  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();
 }
 
 void TraceEvent::SendToATrace() {
@@ -176,13 +198,5 @@ 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,20 +12,11 @@ namespace trace_event {
 TEST(TraceEventAndroidTest, WriteToATrace) {
   // Just a smoke test to ensure no crash.
   TraceLog* trace_log = TraceLog::GetInstance();
-  trace_log->StartATrace("test");
+  trace_log->StartATrace();
   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,7 +19,6 @@
 #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"
@@ -107,12 +106,10 @@ class BASE_EXPORT TraceLog : public MemoryDumpProvider {
   int GetNumTracesRecorded();
 
 #if defined(OS_ANDROID)
-  void StartATrace(const std::string& category_filter);
+  void StartATrace();
   void StopATrace();
   void AddClockSyncMetadataEvent();
-  void SetupATraceStartupTrace(const std::string& category_filter);
-  Optional<TraceConfig> TakeATraceStartupConfig();
-#endif  // defined(OS_ANDROID)
+#endif
 
   // Enabled state listeners give a callback when tracing is enabled or
   // disabled. This can be used to tie into other library's tracing systems
@@ -579,10 +576,6 @@ 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,7 +7,6 @@ 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;
 
@@ -93,14 +92,8 @@ 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(
-                    (isAppDebuggable || isOsDebuggable) ? TraceEvent.ATRACE_TAG_APP : 0,
-                    /*readCommandLine=*/true);
+            TraceEvent.maybeEnableEarlyTracing();
             TraceEvent.begin("ChromeApplication.attachBaseContext");
 
             // Register for activity lifecycle callbacks. Must be done before any activities are

components/tracing/common/trace_startup_config.cc

@@ -15,7 +15,6 @@
 #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"
@@ -96,10 +95,6 @@ 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());
   }
 }
 
@@ -201,24 +196,6 @@ 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,7 +157,6 @@ class TRACING_EXPORT TraceStartupConfig {
   bool EnableFromCommandLine();
   bool EnableFromConfigFile();
   bool EnableFromBackgroundTracing();
-  bool EnableFromATrace();
 
   bool ParseTraceConfigFileContent(const std::string& content);