RPP: cache SyntheticEventsManager on rawEvents array to fix tests

We have had some tests on CI that fail when run as part of the overall
DevTools suite, but pass in isolation.

One of the reasons this happens is because of how the
SyntheticEventsManager is initialised. In tests, because we cache and
don't re-run model.parse() on the same trace, we end up not having the
right active SyntheticEventsManager for a given trace. The test will
then pass if run in isolation because when it is the only test running
it has the correct data setup.

To fix this I modified the SyntheticEventsManager; now it caches
instances based on the rawEvents() array, and tracks the active one. We
did support fetching by an index, but the API didn't actually ever
return the user what the index for a given trace was, so this was
useless, and we only ever care about the active one anyway.

Bug: none
Change-Id: Ie709b8bff370c395e3eeed2029fddd5086a3b2b4
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5687953
Reviewed-by: Alina Varkki <alinavarkki@chromium.org>
Commit-Queue: Jack Franklin <jacktfranklin@chromium.org>
Auto-Submit: Jack Franklin <jacktfranklin@chromium.org>

Author: jackfranklin

front_end/models/trace/ModelImpl.ts

@@ -106,8 +106,7 @@ export class Model<EnabledModelHandlers extends {[key: string]: Handlers.Types.T
     try {
       // Wait for all outstanding promises before finishing the async execution,
       // but perform all tasks in parallel.
-      const syntheticEventsManager =
-          Helpers.SyntheticEvents.SyntheticEventsManager.initSyntheticEventsManagerForTrace(traceEvents);
+      const syntheticEventsManager = Helpers.SyntheticEvents.SyntheticEventsManager.initAndActivate(traceEvents);
       await this.#processor.parse(traceEvents, isFreshRecording);
       this.#storeParsedFileData(file, this.#processor.traceParsedData, this.#processor.insights);
       // We only push the file onto this.#traces here once we know it's valid

front_end/models/trace/handlers/ScreenshotsHandler.test.ts

@@ -42,7 +42,7 @@ describe('ScreenshotsHandler', function() {
         {...baseEvent, ts: TraceModel.Types.Timing.MicroSeconds(100)},
         {...baseEvent, ts: TraceModel.Types.Timing.MicroSeconds(200)},
       ];
-      TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.initSyntheticEventsManagerForTrace(baseEvents);
+      TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.initAndActivate(baseEvents);
 
       for (const event of baseEvents) {
         TraceModel.Handlers.ModelHandlers.Meta.handleEvent(event);

front_end/models/trace/helpers/SyntheticEvents.test.ts

@@ -9,32 +9,37 @@ describe('SyntheticEvents', function() {
   beforeEach(() => {
     TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.reset();
   });
+
   afterEach(() => {
     TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.reset();
   });
+
   describe('Initialization', function() {
     it('does not throw when invoking getActiveManager after executing the trace engine', async function() {
       const events = await TraceLoader.fixtureContents(this, 'basic.json.gz');
       await TraceLoader.executeTraceEngineOnFileContents(events);
       assert.doesNotThrow(TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.getActiveManager);
     });
 
-    it('returns the last active SyntheticEventsManager with getActiveManager', async function() {
-      const events = await TraceLoader.fixtureContents(this, 'basic.json.gz');
-      await TraceLoader.executeTraceEngineOnFileContents(events);
-      const firstActiveManager = TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.getActiveManager();
-      await TraceLoader.executeTraceEngineOnFileContents(events);
-      const secondActiveManager = TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.getActiveManager();
-      assert.notEqual(firstActiveManager, secondActiveManager);
-    });
-    it('returns the SyntheticEventsManager for a given trace index with getManagerForTrace', async function() {
-      const events = await TraceLoader.fixtureContents(this, 'basic.json.gz');
-      await TraceLoader.executeTraceEngineOnFileContents(events);
-      const firstActiveManager = TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.getActiveManager();
-      await TraceLoader.executeTraceEngineOnFileContents(events);
-      const testActiveManager = TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.getManagerForTrace(0);
-      assert.strictEqual(testActiveManager, firstActiveManager);
-    });
+    it('always returns the manager for the most recent trace and re-uses managers for the same set of input events',
+       async function() {
+         // Exact traces do not matter, as long as they are different
+         const trace1 = await TraceLoader.fixtureContents(this, 'basic.json.gz');
+         const trace2 = await TraceLoader.fixtureContents(this, 'basic-stack.json.gz');
+
+         const events1 = Array.isArray(trace1) ? trace1 : trace1.traceEvents;
+         const events2 = Array.isArray(trace2) ? trace2 : trace2.traceEvents;
+
+         const manager1 = TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.initAndActivate(events1);
+         const manager2 = TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.initAndActivate(events2);
+
+         // Manager2 is active as it was the last one to be initialized
+         assert.strictEqual(TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.getActiveManager(), manager2);
+         // Now re-init with events1 (should use the existing manager from the cache)
+         TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.initAndActivate(events1);
+         // Now manager1 is active
+         assert.strictEqual(TraceModel.Helpers.SyntheticEvents.SyntheticEventsManager.getActiveManager(), manager1);
+       });
   });
 
   describe('SyntheticBasedEvent registration', () => {

front_end/models/trace/helpers/SyntheticEvents.ts

@@ -6,6 +6,10 @@ import type * as Types from '../types/types.js';
 
 const syntheticEventsManagerByTraceIndex: SyntheticEventsManager[] = [];
 
+const managerByRawEvents = new Map<readonly Types.TraceEvents.TraceEventData[], SyntheticEventsManager>();
+
+let activeManager: SyntheticEventsManager|null = null;
+
 export class SyntheticEventsManager {
   /**
    * All synthetic entries created in a trace from a corresponding trace events.
@@ -22,45 +26,31 @@ export class SyntheticEventsManager {
    * called before running the trace engine handlers, since the instance
    * created here will be used by the handlers to register their
    * synthetic trace events.
+   *
+   * Can be called multiple times for the same set of raw events, in which case it will re-use the existing manager rather than recreate it again.
    */
-  static initSyntheticEventsManagerForTrace(rawEvents: readonly Types.TraceEvents.TraceEventData[]):
-      SyntheticEventsManager {
-    const manager = new SyntheticEventsManager(rawEvents);
-    syntheticEventsManagerByTraceIndex.push(manager);
-    return manager;
-  }
-
-  /**
-   * Gets the SyntheticEventsManager instance for a trace given the index
-   * of the trace given its index used in Model#traces. If no index is
-   * passed, defaults to the last created instance.
-   * If no instance is found throws error.
-   */
-  static getManagerForTrace(traceIndex?: number): SyntheticEventsManager {
-    const last = syntheticEventsManagerByTraceIndex.at(-1);
-    if (!last) {
-      throw new Error('Attempted to get a SyntheticEventsManager without initializing');
-    }
-    if (traceIndex === undefined) {
-      return last;
-    }
-    const manager = syntheticEventsManagerByTraceIndex.at(traceIndex);
-    if (!manager) {
-      throw new Error(`Attempted to get a SyntheticEventsManager with an invalid index ${traceIndex}`);
+  static initAndActivate(rawEvents: readonly Types.TraceEvents.TraceEventData[]): SyntheticEventsManager {
+    const existingManager = managerByRawEvents.get(rawEvents);
+    if (existingManager) {
+      activeManager = existingManager;
+    } else {
+      const manager = new SyntheticEventsManager(rawEvents);
+      managerByRawEvents.set(rawEvents, manager);
+      activeManager = manager;
     }
-    return manager;
+    return activeManager;
   }
 
   static getActiveManager(): SyntheticEventsManager {
-    const last = syntheticEventsManagerByTraceIndex.at(-1);
-    if (!last) {
+    if (!activeManager) {
       throw new Error('Attempted to get a SyntheticEventsManager without initializing');
     }
-    return SyntheticEventsManager.getManagerForTrace(syntheticEventsManagerByTraceIndex.length - 1);
+    return activeManager;
   }
 
   static reset(): void {
     syntheticEventsManagerByTraceIndex.length = 0;
+    activeManager = null;
   }
 
   static registerSyntheticBasedEvent<T extends Types.TraceEvents.SyntheticBasedEvent>(syntheticEvent: Omit<T, '_tag'>):

front_end/models/trace/lantern/core/BUILD.gn

@@ -33,7 +33,8 @@ ts_library("unittests") {
   sources = [ "NetworkAnalyzer.test.ts" ]
 
   deps = [
-    "../:bundle",
+    "../:bundle",  # Lantern bundle
+    "../../:bundle",  # Trace engine bundle
     "../testing:bundle",
   ]
 }

front_end/models/trace/lantern/simulation/BUILD.gn

@@ -46,6 +46,7 @@ ts_library("unittests") {
 
   deps = [
     "../:bundle",
+    "../../:bundle",  # Trace engine bundle
     "../testing:bundle",
   ]
 }

front_end/models/trace/lantern/testing/BUILD.gn

@@ -12,6 +12,7 @@ devtools_module("testing") {
 
   deps = [
     "../:bundle",
+    "../../:bundle",
     "../../../../testing",
   ]
 }

front_end/panels/timeline/EventsSerializer.test.ts

@@ -3,11 +3,11 @@
 // found in the LICENSE file.
 
 import * as TraceEngine from '../../models/trace/trace.js';
-import * as Timeline from './timeline.js';
-
 import {getMainThread} from '../../testing/TraceHelpers.js';
 import {TraceLoader} from '../../testing/TraceLoader.js';
 
+import * as Timeline from './timeline.js';
+
 function findFirstEntry(
     allEntries: readonly TraceEngine.Types.TraceEvents.SyntheticTraceEntry[],
     predicate: (entry: TraceEngine.Types.TraceEvents.SyntheticTraceEntry) =>
@@ -19,15 +19,11 @@ function findFirstEntry(
   return entry;
 }
 
-describe('ModificationsManager', () => {
+describe('EventsSerializer', () => {
   it('correctly implements a bidirectional key <-> event mapping', async function() {
-    const data = await TraceLoader.traceEngine(null, 'basic-stack.json.gz');
+    const data = await TraceLoader.traceEngine(this, 'basic-stack.json.gz');
     const eventsSerializer = new Timeline.EventsSerializer.EventsSerializer();
-    if (!eventsSerializer) {
-      throw new Error('Modifications manager does not exist.');
-    }
     const mainThread = getMainThread(data.Renderer);
-    assert.exists(eventsSerializer);
     // Find first 'Timer Fired' entry in the trace
     const rawEntry = findFirstEntry(mainThread.entries, entry => {
       return entry.name === 'TimerFire';

front_end/testing/BUILD.gn

@@ -66,7 +66,6 @@ ts_library("testing") {
     "../models/text_utils:bundle",
     "../models/timeline_model:bundle",
     "../models/trace:bundle",
-    "../models/trace:bundle",
     "../models/workspace:bundle",
     "../panels/elements:meta",
     "../panels/sensors:meta",

front_end/testing/TraceLoader.ts

@@ -95,7 +95,7 @@ export class TraceLoader {
     const contents = await TraceLoader.fixtureContents(context, name);
 
     const events = 'traceEvents' in contents ? contents.traceEvents : contents;
-    TraceEngine.Helpers.SyntheticEvents.SyntheticEventsManager.initSyntheticEventsManagerForTrace(events);
+    TraceEngine.Helpers.SyntheticEvents.SyntheticEventsManager.initAndActivate(events);
     return events;
   }
 
@@ -144,6 +144,19 @@ export class TraceLoader {
     if (fromCache) {
       TraceLoader.initTraceBoundsManager(fromCache.traceParsedData);
       Timeline.ModificationsManager.ModificationsManager.initAndActivateModificationsManager(fromCache.model, 0);
+
+      // This init step is usually done in model.parse(), but as we loaded from
+      // the cache here, we manually run it.
+      // The SyntheticEventsManager caches instances based on the rawEvents()
+      // array, so we can safely do this even if we have already created an
+      // instance for this trace before - the old one will be re-used, rather
+      // than creating a new one.
+      const rawEvents = fromCache.model.rawTraceEvents();
+      if (rawEvents) {
+        TraceEngine.Helpers.SyntheticEvents.SyntheticEventsManager.initAndActivate(
+            rawEvents,
+        );
+      }
       return fromCache.traceParsedData;
     }
     const fileContents = await TraceLoader.fixtureContents(context, name);
@@ -158,7 +171,6 @@ export class TraceLoader {
 
     TraceLoader.initTraceBoundsManager(traceEngineData.traceParsedData);
     Timeline.ModificationsManager.ModificationsManager.initAndActivateModificationsManager(traceEngineData.model, 0);
-
     return traceEngineData.traceParsedData;
   }