ui: fix race condition on extra macro registration (#3014)

There was a subtle race between us figuring out whether a user is an
internal user (and fetching associated data) and registering the macros
for that user. Break the race by waiting to register extra macros until
after the internal user status is known.

Also defer registration of the settings macros until trace load time:
there's no reason for this to happen at activate time: most of these
macros will be completely unrunnable at that time anyway.

Author: LalitMaganti

ui/src/core/app_impl.ts

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 import {AsyncLimiter} from '../base/async_limiter';
+import {defer} from '../base/deferred';
 import {assertExists, assertTrue} from '../base/logging';
 import {createProxy, getOrCreate} from '../base/utils';
 import {ServiceWorkerController} from '../frontend/service_worker_controller';
@@ -98,6 +99,9 @@ export class AppContext {
   // via is_internal_user.js
   extraMacros: Record<string, CommandInvocation[]>[] = [];
 
+  // Promise which is resolved when extra loading is completed.
+  extrasLoadingDeferred = defer<undefined>();
+
   // The currently open trace.
   currentTrace?: TraceContext;
 
@@ -435,4 +439,12 @@ export class AppImpl implements App {
   set isInternalUser(value: boolean) {
     this.appCtx.isInternalUser = value;
   }
+
+  notifyOnExtrasLoadingCompleted() {
+    this.appCtx.extrasLoadingDeferred.resolve();
+  }
+
+  get extraLoadingPromise(): Promise<undefined> {
+    return this.appCtx.extrasLoadingDeferred;
+  }
 }

ui/src/core_plugins/dev.perfetto.CoreCommands/index.ts

@@ -42,6 +42,8 @@ import {DurationPrecision, TimestampFormat} from '../../public/timeline';
 import {getTimeSpanOfSelectionOrVisibleWindow} from '../../public/utils';
 import {Workspace} from '../../public/workspace';
 import {showModal} from '../../widgets/modal';
+import {assertExists} from '../../base/logging';
+import {Setting} from '../../public/settings';
 
 const QUICKSAVE_LOCALSTORAGE_KEY = 'quicksave';
 
@@ -129,9 +131,14 @@ function getOrPromptForTimestamp(tsRaw: unknown): time | undefined {
   return promptForTimestamp('Enter a timestamp');
 }
 
+const macroSchema = z.record(z.array(commandInvocationSchema));
+type MacroConfig = z.infer<typeof macroSchema>;
+
 export default class CoreCommands implements PerfettoPlugin {
   static readonly id = 'dev.perfetto.CoreCommands';
 
+  static macrosSetting: Setting<MacroConfig> | undefined = undefined;
+
   static onActivate(ctx: AppImpl) {
     if (ctx.sidebar.enabled) {
       ctx.commands.registerCommand({
@@ -144,13 +151,10 @@ export default class CoreCommands implements PerfettoPlugin {
       });
     }
 
-    // Use shared commandInvocationSchema where 'id' is the command's unique identifier
-    const macroSchema = z.record(z.array(commandInvocationSchema));
-    type MacroConfig = z.infer<typeof macroSchema>;
     const macroSettingsEditor = new JsonSettingsEditor<MacroConfig>({
       schema: macroSchema,
     });
-    const setting = ctx.settings.register({
+    CoreCommands.macrosSetting = ctx.settings.register({
       id: 'perfetto.CoreCommands#UserDefinedMacros',
       name: 'Macros',
       description:
@@ -161,27 +165,6 @@ export default class CoreCommands implements PerfettoPlugin {
       render: (setting) => macroSettingsEditor.render(setting),
     });
 
-    // Register both user-defined macros from settings and extra macros from google3
-    const allMacros = {
-      ...setting.get(),
-      ...ctx.extraMacros.reduce((acc, macro) => ({...acc, ...macro}), {}),
-    } as MacroConfig;
-    for (const [macroName, commands] of Object.entries(allMacros)) {
-      ctx.commands.registerCommand({
-        id: `dev.perfetto.UserMacro.${macroName}`,
-        name: macroName,
-        callback: async () => {
-          // Macros could run multiple commands, some of which might prompt the
-          // user in an optional way. But macros should be self-contained
-          // so we disable prompts during their execution.
-          using _ = ctx.omnibox.disablePrompts();
-          for (const command of commands) {
-            await ctx.commands.runCommand(command.id, ...command.args);
-          }
-        },
-      });
-    }
-
     const input = document.createElement('input');
     input.classList.add('trace_file');
     input.setAttribute('type', 'file');
@@ -232,6 +215,23 @@ export default class CoreCommands implements PerfettoPlugin {
   }
 
   async onTraceLoad(ctx: TraceImpl): Promise<void> {
+    const app = AppImpl.instance;
+
+    // Rgister macros from settings first.
+    registerMacros(ctx, assertExists(CoreCommands.macrosSetting).get());
+
+    // Register the macros from extras at onTraceReady (the latest time
+    // possible).
+    ctx.onTraceReady.addListener(async (_) => {
+      // Await the promise: we've tried to be async as long as possible but
+      // now we need the extras to be loaded.
+      await app.extraLoadingPromise;
+      registerMacros(
+        ctx,
+        app.extraMacros.reduce((acc, macro) => ({...acc, ...macro}), {}),
+      );
+    });
+
     ctx.commands.registerCommand({
       id: 'dev.perfetto.RunQueryAllProcesses',
       name: 'Run query: All processes',
@@ -866,3 +866,21 @@ async function openWithLegacyUi(file: File) {
   }
   return await openInOldUIWithSizeCheck(file);
 }
+
+function registerMacros(trace: TraceImpl, config: MacroConfig) {
+  for (const [macroName, commands] of Object.entries(config)) {
+    trace.commands.registerCommand({
+      id: `dev.perfetto.UserMacro.${macroName}`,
+      name: macroName,
+      callback: async () => {
+        // Macros could run multiple commands, some of which might prompt the
+        // user in an optional way. But macros should be self-contained
+        // so we disable prompts during their execution.
+        using _ = trace.omnibox.disablePrompts();
+        for (const command of commands) {
+          await trace.commands.runCommand(command.id, ...command.args);
+        }
+      },
+    });
+  }
+}

ui/src/frontend/index.ts

@@ -280,6 +280,7 @@ function main() {
   const app = AppImpl.instance;
   tryLoadIsInternalUserScript(app).then(() => {
     app.analytics.initialize(app.isInternalUser);
+    app.notifyOnExtrasLoadingCompleted();
   });
 
   // Route errors to both the UI bugreport dialog and Analytics (if enabled).

ui/src/frontend/is_internal_user_script_loader.ts

@@ -21,7 +21,7 @@ import {CommandInvocation} from '../core/command_manager';
 // proceeding as if the user is not internal.
 const SCRIPT_LOAD_TIMEOUT_MS = 5000;
 const SCRIPT_URL =
-  'https://storage.cloud.google.com/perfetto-ui-internal/is-internal-user/is_internal_user.js';
+  'https://storage.cloud.google.com/perfetto-ui-internal/internal-data-v1/amalgamated.js';
 
 // This interface describes the required interface that the script expect to
 // find on window.globals.