Fix incorrect disposing of crashed targets

In April 2024 last year we landed crrev.com/c/5458415 which handled a
case where the performance panel would freeze after trace recording. We
tracked the freezing down to a call to `resumeAllTargets()` which never
resolved. The performance panel suspends all targets before starting a
recording.

The cause of this was a target crashing during the recording process.
This would mean it would hang when `resume()` was called. To fix this,
we added code that disposed of the target when it crashed. This meant we
would not then try to resume it after the trace recording was complete.

Fast forward to the present, and the bug crbug.com/387258086 gets
reported which shows a case where if you crash the page ("awww snap")
and then reload the page, the page itself is fine again but DevTools for
that page is completely broken. The triage folks tracked this down to
the original CL mentioned above (crrev.com/c/5458415) which disposed of
crashed targets.

When debugging this I realised that our solution of `target.dispose()`
on a crashed target is too heavy-handed. I had incorrectly assumed that
a crashed target can never "un-crash", but this is not true. If you have
a page that crashes, and reload the page, the target can go from crashed
=> working. So the fix from the original CL that disposed it was
incorrect.

This CL introduces a fix for both bugs; we still prevent the hanging on
`resume()` in the performance panel, but we fix the page crash bug also.
Rather than dispose the target, this CL now:

1. Marks a target as crashed when we get the `targetCrashed` CDP event.
2. Marks a target as not-crashed when we get the `targetInfoChanged` CDP
   event (from my debugging this is what gets fired on crashed targets
   when you reload the page).
3. Skips attempting to `resume()` a crashed target (thus avoiding the
   performance panel freezing issue).

Fixed: 387258086, 333989070
Change-Id: Ia609f6ceee8e166d9defcdd826bcff61002e50cb
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6148211
Reviewed-by: Alex Rudenko <[email protected]>
Commit-Queue: Jack Franklin <[email protected]>

front_end/core/sdk/ChildTargetManager.test.ts

@@ -222,7 +222,7 @@ describeWithMockConnection('ChildTargetManager', () => {
     assert.isTrue(attachCallback.secondCall.firstArg.waitingForDebugger);
   });
 
-  it('disposes of the target if it crashes', async () => {
+  it('marks the target as crashed when it crashes', async () => {
     const parentTarget = createTarget();
     const childTargetManager = new SDK.ChildTargetManager.ChildTargetManager(parentTarget);
     await childTargetManager.attachedToTarget({
@@ -233,11 +233,27 @@ describeWithMockConnection('ChildTargetManager', () => {
     const target = childTargetManager.childTargets().at(0);
     assert.isDefined(target);
     assert.strictEqual(target.id(), 'child-target-id');
-    const disposeSpy = sinon.spy(target, 'dispose');
     childTargetManager.targetCrashed(
         {targetId: target.id() as Protocol.Target.TargetID, status: 'crashed', errorCode: 1});
 
-    // Ensure that the target has been disposed after it crashed.
-    assert.isTrue(disposeSpy.calledOnce);
+    assert.isTrue(target.hasCrashed());
+  });
+
+  it('"un-crashes" a target when the target info message is received', async () => {
+    const parentTarget = createTarget();
+    const childTargetManager = new SDK.ChildTargetManager.ChildTargetManager(parentTarget);
+    await childTargetManager.attachedToTarget({
+      sessionId: createSessionId(),
+      targetInfo: createTargetInfo('child-target-id'),
+      waitingForDebugger: false,
+    });
+    const target = childTargetManager.childTargets().at(0);
+    assert.isDefined(target);
+    assert.strictEqual(target.id(), 'child-target-id');
+    childTargetManager.targetCrashed(
+        {targetId: target.id() as Protocol.Target.TargetID, status: 'crashed', errorCode: 1});
+    assert.isTrue(target.hasCrashed());
+    childTargetManager.targetInfoChanged({targetInfo: createTargetInfo(target.id())});
+    assert.isFalse(target.hasCrashed());
   });
 });

front_end/core/sdk/ChildTargetManager.ts

@@ -95,6 +95,7 @@ export class ChildTargetManager extends SDKModel<EventTypes> implements Protocol
     this.#targetInfosInternal.set(targetInfo.targetId, targetInfo);
     const target = this.#childTargetsById.get(targetInfo.targetId);
     if (target) {
+      void target.setHasCrashed(false);
       if (target.targetInfo()?.subtype === 'prerender' && !targetInfo.subtype) {
         const resourceTreeModel = target.model(ResourceTreeModel);
         target.updateTargetInfo(targetInfo);
@@ -117,13 +118,10 @@ export class ChildTargetManager extends SDKModel<EventTypes> implements Protocol
   }
 
   targetCrashed({targetId}: Protocol.Target.TargetCrashedEvent): void {
-    this.#targetInfosInternal.delete(targetId);
     const target = this.#childTargetsById.get(targetId);
     if (target) {
-      target.dispose('targetCrashed event from CDP');
+      target.setHasCrashed(true);
     }
-    this.fireAvailableTargetsChanged();
-    this.dispatchEventToListeners(Events.TARGET_DESTROYED, targetId);
   }
 
   private fireAvailableTargetsChanged(): void {
@@ -173,8 +171,7 @@ export class ChildTargetManager extends SDKModel<EventTypes> implements Protocol
       type = Type.FRAME;
     } else if (targetInfo.type === 'background_page' || targetInfo.type === 'app' || targetInfo.type === 'popup_page') {
       type = Type.FRAME;
-    }
-    else if (targetInfo.type === 'page') {
+    } else if (targetInfo.type === 'page') {
       type = Type.FRAME;
     } else if (targetInfo.type === 'worker') {
       type = Type.Worker;

front_end/core/sdk/Target.test.ts

@@ -60,4 +60,38 @@ describeWithMockConnection('Target', () => {
     const serviceWorkerTarget = createTarget({type: SDK.Target.Type.ServiceWorker, parentTarget: browserTarget});
     assert.strictEqual(serviceWorkerTarget.outermostTarget(), serviceWorkerTarget);
   });
+
+  it('tries to resume itself if it was crashed and is then recovered', () => {
+    const target = createTarget();
+    target.setHasCrashed(true);
+    const spy = sinon.spy(target, 'resume');
+    target.setHasCrashed(false);
+    assert.isTrue(spy.calledOnce);
+  });
+
+  it('does not resume itself if it was not already crashed', async () => {
+    const target = createTarget();
+    target.setHasCrashed(true);
+    const spy = sinon.spy(target, 'resume');
+    // Call this twice, but ensure we only call the spy once.
+    target.setHasCrashed(false);
+    target.setHasCrashed(false);
+    assert.strictEqual(spy.callCount, 1);
+  });
+
+  it('marks a crashed target as suspended', async () => {
+    const target = createTarget();
+    target.setHasCrashed(true);
+    await target.suspend();
+    assert.isTrue(target.suspended());
+  });
+
+  it('marks a crashed, suspended target as resumed', async () => {
+    const target = createTarget();
+    target.setHasCrashed(true);
+    await target.suspend();
+    assert.isTrue(target.suspended());
+    await target.resume();
+    assert.isFalse(target.suspended());
+  });
 });

front_end/core/sdk/Target.ts

@@ -21,6 +21,18 @@ export class Target extends ProtocolClient.InspectorBackend.TargetBase {
   #idInternal: Protocol.Target.TargetID|'main';
   #modelByConstructor: Map<new(arg1: Target) => SDKModel, SDKModel>;
   #isSuspended: boolean;
+  /**
+   * Generally when a target crashes we don't need to know, with one exception.
+   * If a target crashes during the recording of a performance trace, after the
+   * trace when we try to resume() it, it will fail because it has crashed. This
+   * causes the performance panel to freeze (see crbug.com/333989070). So we
+   * mark the target as crashed so we can exit without trying to resume it. In
+   * `ChildTargetManager` we will mark a target as "un-crashed" when we get the
+   * `targetInfoChanged` event. This helps ensure we can deal with cases where
+   * the page crashes, but a reload fixes it and the targets get restored (see
+   * crbug.com/387258086).
+   */
+  #hasCrashed: boolean = false;
   #targetInfoInternal: Protocol.Target.TargetInfo|undefined;
   #creatingModels?: boolean;
 
@@ -214,12 +226,36 @@ export class Target extends ProtocolClient.InspectorBackend.TargetBase {
     }
   }
 
+  hasCrashed(): boolean {
+    return this.#hasCrashed;
+  }
+
+  setHasCrashed(isCrashed: boolean): void {
+    const wasCrashed = this.#hasCrashed;
+
+    this.#hasCrashed = isCrashed;
+    // If the target has now been restored, check to see if it needs resuming.
+    // This ensures that if a target crashes whilst suspended, it is resumed
+    // when it is recovered.
+    // If the target is not suspended, resume() is a no-op, so it's safe to call.
+    if (wasCrashed && !isCrashed) {
+      void this.resume();
+    }
+  }
+
   async suspend(reason?: string): Promise<void> {
     if (this.#isSuspended) {
       return;
     }
     this.#isSuspended = true;
 
+    // If the target has crashed, we will not attempt to suspend all the
+    // models, but we still mark it as suspended so we correctly track the
+    // state.
+    if (this.#hasCrashed) {
+      return;
+    }
+
     await Promise.all(Array.from(this.models().values(), m => m.preSuspendModel(reason)));
     await Promise.all(Array.from(this.models().values(), m => m.suspendModel(reason)));
   }
@@ -230,6 +266,10 @@ export class Target extends ProtocolClient.InspectorBackend.TargetBase {
     }
     this.#isSuspended = false;
 
+    if (this.#hasCrashed) {
+      return;
+    }
+
     await Promise.all(Array.from(this.models().values(), m => m.resumeModel()));
     await Promise.all(Array.from(this.models().values(), m => m.postResumeModel()));
   }