refactor: make deferred promises more robust to use (#10245)

packages/puppeteer-core/src/common/Browser.ts

@@ -360,7 +360,10 @@ export class CDPBrowser extends BrowserBase {
   };
 
   #onAttachedToTarget = async (target: Target) => {
-    if ((await target._initializedPromise) === InitializationStatus.SUCCESS) {
+    if (
+      (await target._initializedPromise.valueOrThrow()) ===
+      InitializationStatus.SUCCESS
+    ) {
       this.emit(BrowserEmittedEvents.TargetCreated, target);
       target
         .browserContext()
@@ -371,7 +374,10 @@ export class CDPBrowser extends BrowserBase {
   #onDetachedFromTarget = async (target: Target): Promise<void> => {
     target._initializedPromise.resolve(InitializationStatus.ABORTED);
     target._isClosedPromise.resolve();
-    if ((await target._initializedPromise) === InitializationStatus.SUCCESS) {
+    if (
+      (await target._initializedPromise.valueOrThrow()) ===
+      InitializationStatus.SUCCESS
+    ) {
       this.emit(BrowserEmittedEvents.TargetDestroyed, target);
       target
         .browserContext()
@@ -432,7 +438,8 @@ export class CDPBrowser extends BrowserBase {
       throw new Error(`Missing target for page (id = ${targetId})`);
     }
     const initialized =
-      (await target._initializedPromise) === InitializationStatus.SUCCESS;
+      (await target._initializedPromise.valueOrThrow()) ===
+      InitializationStatus.SUCCESS;
     if (!initialized) {
       throw new Error(`Failed to create target for page (id = ${targetId})`);
     }
@@ -501,9 +508,13 @@ export class CDPBrowser extends BrowserBase {
     try {
       this.targets().forEach(check);
       if (!timeout) {
-        return await targetPromise;
+        return await targetPromise.valueOrThrow();
       }
-      return await waitWithTimeout(targetPromise, 'target', timeout);
+      return await waitWithTimeout(
+        targetPromise.valueOrThrow(),
+        'target',
+        timeout
+      );
     } finally {
       this.off(BrowserEmittedEvents.TargetCreated, check);
       this.off(BrowserEmittedEvents.TargetChanged, check);

packages/puppeteer-core/src/common/ChromeTargetManager.ts

@@ -131,7 +131,7 @@ export class ChromeTargetManager extends EventEmitter implements TargetManager {
       autoAttach: true,
     });
     this.#finishInitializationIfReady();
-    await this.#initializePromise;
+    await this.#initializePromise.valueOrThrow();
   }
 
   dispose(): void {

packages/puppeteer-core/src/common/Connection.ts

@@ -18,12 +18,13 @@ import {Protocol} from 'devtools-protocol';
 import {ProtocolMapping} from 'devtools-protocol/types/protocol-mapping.js';
 
 import {assert} from '../util/assert.js';
-import {createDeferredPromise} from '../util/util.js';
+import {createDeferredPromise, DeferredPromise} from '../util/util.js';
 
 import {ConnectionTransport} from './ConnectionTransport.js';
 import {debug} from './Debug.js';
 import {TargetCloseError, ProtocolError} from './Errors.js';
 import {EventEmitter} from './EventEmitter.js';
+import {debugError} from './util.js';
 
 const debugProtocolSend = debug('puppeteer:protocol:SEND ►');
 const debugProtocolReceive = debug('puppeteer:protocol:RECV ◀');
@@ -96,7 +97,7 @@ class Callback {
     return this.#id;
   }
 
-  get promise(): Promise<unknown> {
+  get promise(): DeferredPromise<unknown> {
     return this.#promise;
   }
 
@@ -130,14 +131,17 @@ export class CallbackRegistry {
     } catch (error) {
       // We still throw sync errors synchronously and clean up the scheduled
       // callback.
-      callback.promise.catch(() => {
-        this.#callbacks.delete(callback.id);
-      });
+      callback.promise
+        .valueOrThrow()
+        .catch(debugError)
+        .finally(() => {
+          this.#callbacks.delete(callback.id);
+        });
       callback.reject(error as Error);
       throw error;
     }
     // Must only have sync code up until here.
-    return callback.promise.finally(() => {
+    return callback.promise.valueOrThrow().finally(() => {
       this.#callbacks.delete(callback.id);
     });
   }

packages/puppeteer-core/src/common/DeviceRequestPrompt.ts

@@ -161,7 +161,7 @@ export class DeviceRequestPrompt {
     const handle = {filter, promise};
     this.#waitForDevicePromises.add(handle);
     try {
-      return await promise;
+      return await promise.valueOrThrow();
     } finally {
       this.#waitForDevicePromises.delete(handle);
     }
@@ -262,7 +262,10 @@ export class DeviceRequestPromptManager {
     this.#deviceRequestPromptPromises.add(promise);
 
     try {
-      const [result] = await Promise.all([promise, enablePromise]);
+      const [result] = await Promise.all([
+        promise.valueOrThrow(),
+        enablePromise,
+      ]);
       return result;
     } finally {
       this.#deviceRequestPromptPromises.delete(promise);

packages/puppeteer-core/src/common/FirefoxTargetManager.ts

@@ -169,7 +169,7 @@ export class FirefoxTargetManager
       filter: [{}],
     });
     this.#targetsIdsForInit = new Set(this.#discoveredTargetsByTargetId.keys());
-    await this.#initializePromise;
+    await this.#initializePromise.valueOrThrow();
   }
 
   #onTargetCreated = async (

packages/puppeteer-core/src/common/Frame.ts

@@ -419,7 +419,7 @@ export class Frame extends BaseFrame {
             script.id = id;
           }
           document.head.appendChild(script);
-          await promise;
+          await promise.valueOrThrow();
           return script;
         },
         LazyArg.create(context => {
@@ -488,7 +488,7 @@ export class Frame extends BaseFrame {
             {once: true}
           );
           document.head.appendChild(element);
-          await promise;
+          await promise.valueOrThrow();
           return element;
         },
         LazyArg.create(context => {

packages/puppeteer-core/src/common/FrameTree.ts

@@ -57,7 +57,7 @@ export class FrameTree<Frame extends BaseFrame> {
     const callbacks =
       this.#waitRequests.get(frameId) || new Set<DeferredPromise<Frame>>();
     callbacks.add(deferred);
-    return deferred;
+    return deferred.valueOrThrow();
   }
 
   frames(): Frame[] {

packages/puppeteer-core/src/common/HTTPResponse.ts

@@ -136,31 +136,37 @@ export class HTTPResponse extends BaseHTTPResponse {
 
   override buffer(): Promise<Buffer> {
     if (!this.#contentPromise) {
-      this.#contentPromise = this.#bodyLoadedPromise.then(async error => {
-        if (error) {
-          throw error;
-        }
-        try {
-          const response = await this.#client.send('Network.getResponseBody', {
-            requestId: this.#request._requestId,
-          });
-          return Buffer.from(
-            response.body,
-            response.base64Encoded ? 'base64' : 'utf8'
-          );
-        } catch (error) {
-          if (
-            error instanceof ProtocolError &&
-            error.originalMessage === 'No resource with given identifier found'
-          ) {
-            throw new ProtocolError(
-              'Could not load body for this request. This might happen if the request is a preflight request.'
-            );
+      this.#contentPromise = this.#bodyLoadedPromise
+        .valueOrThrow()
+        .then(async error => {
+          if (error) {
+            throw error;
           }
+          try {
+            const response = await this.#client.send(
+              'Network.getResponseBody',
+              {
+                requestId: this.#request._requestId,
+              }
+            );
+            return Buffer.from(
+              response.body,
+              response.base64Encoded ? 'base64' : 'utf8'
+            );
+          } catch (error) {
+            if (
+              error instanceof ProtocolError &&
+              error.originalMessage ===
+                'No resource with given identifier found'
+            ) {
+              throw new ProtocolError(
+                'Could not load body for this request. This might happen if the request is a preflight request.'
+              );
+            }
 
-          throw error;
-        }
-      });
+            throw error;
+          }
+        });
     }
     return this.#contentPromise;
   }

packages/puppeteer-core/src/common/IsolatedWorld.ts

@@ -174,7 +174,7 @@ export class IsolatedWorld {
     if (this.#context === null) {
       throw new Error(`Execution content promise is missing`);
     }
-    return this.#context;
+    return this.#context.valueOrThrow();
   }
 
   async evaluateHandle<
@@ -425,13 +425,17 @@ export class IsolatedWorld {
       return;
     }
 
-    const context = await this.#context;
-    if (event.executionContextId !== context._contextId) {
-      return;
-    }
+    try {
+      const context = await this.#context.valueOrThrow();
+      if (event.executionContextId !== context._contextId) {
+        return;
+      }
 
-    const binding = this._bindings.get(name);
-    await binding?.run(context, seq, args, isTrivial);
+      const binding = this._bindings.get(name);
+      await binding?.run(context, seq, args, isTrivial);
+    } catch (err) {
+      debugError(err);
+    }
   };
 
   waitForFunction<

packages/puppeteer-core/src/common/LifecycleWatcher.ts

@@ -204,7 +204,7 @@ export class LifecycleWatcher {
 
   async navigationResponse(): Promise<HTTPResponse | null> {
     // Continue with a possibly null response.
-    await this.#navigationResponseReceived?.catch(() => {});
+    await this.#navigationResponseReceived?.valueOrThrow();
     return this.#navigationRequest ? this.#navigationRequest.response() : null;
   }
 
@@ -213,19 +213,22 @@ export class LifecycleWatcher {
   }
 
   sameDocumentNavigationPromise(): Promise<Error | undefined> {
-    return this.#sameDocumentNavigationPromise;
+    return this.#sameDocumentNavigationPromise.valueOrThrow();
   }
 
   newDocumentNavigationPromise(): Promise<Error | undefined> {
-    return this.#newDocumentNavigationPromise;
+    return this.#newDocumentNavigationPromise.valueOrThrow();
   }
 
   lifecyclePromise(): Promise<void> {
-    return this.#lifecyclePromise;
+    return this.#lifecyclePromise.valueOrThrow();
   }
 
   timeoutOrTerminationPromise(): Promise<Error | TimeoutError | undefined> {
-    return Promise.race([this.#timeoutPromise, this.#terminationPromise]);
+    return Promise.race([
+      this.#timeoutPromise,
+      this.#terminationPromise.valueOrThrow(),
+    ]);
   }
 
   async #createTimeoutPromise(): Promise<TimeoutError | undefined> {

packages/puppeteer-core/src/common/NetworkManager.ts

@@ -131,7 +131,7 @@ export class NetworkManager extends EventEmitter {
    */
   initialize(): Promise<void> {
     if (this.#deferredInitPromise) {
-      return this.#deferredInitPromise;
+      return this.#deferredInitPromise.valueOrThrow();
     }
     this.#deferredInitPromise = createDebuggableDeferredPromise(
       'NetworkManager initialization timed out'
@@ -152,7 +152,7 @@ export class NetworkManager extends EventEmitter {
       .catch(err => {
         deferredInitPromise.reject(err);
       });
-    return this.#deferredInitPromise;
+    return this.#deferredInitPromise.valueOrThrow();
   }
 
   async authenticate(credentials?: Credentials): Promise<void> {

packages/puppeteer-core/src/common/Page.ts

@@ -256,17 +256,23 @@ export class CDPPage extends Page {
     client.on('Page.fileChooserOpened', event => {
       return this.#onFileChooser(event);
     });
-    void this.#target._isClosedPromise.then(() => {
-      this.#target
-        ._targetManager()
-        .removeTargetInterceptor(this.#client, this.#onAttachedToTarget);
+    this.#target._isClosedPromise
+      .valueOrThrow()
+      .then(() => {
+        this.#target
+          ._targetManager()
+          .removeTargetInterceptor(this.#client, this.#onAttachedToTarget);
 
-      this.#target
-        ._targetManager()
-        .off(TargetManagerEmittedEvents.TargetGone, this.#onDetachedFromTarget);
-      this.emit(PageEmittedEvents.Close);
-      this.#closed = true;
-    });
+        this.#target
+          ._targetManager()
+          .off(
+            TargetManagerEmittedEvents.TargetGone,
+            this.#onDetachedFromTarget
+          );
+        this.emit(PageEmittedEvents.Close);
+        this.#closed = true;
+      })
+      .catch(debugError);
   }
 
   #onDetachedFromTarget = (target: Target) => {
@@ -376,7 +382,7 @@ export class CDPPage extends Page {
         enabled: true,
       });
     }
-    return Promise.all([promise, enablePromise])
+    return Promise.all([promise.valueOrThrow(), enablePromise])
       .then(([result]) => {
         return result;
       })
@@ -1048,7 +1054,7 @@ export class CDPPage extends Page {
     ];
 
     await Promise.race([
-      idlePromise,
+      idlePromise.valueOrThrow(),
       ...eventPromises,
       this.#sessionClosePromise(),
     ]).then(
@@ -1556,7 +1562,7 @@ export class CDPPage extends Page {
       await connection.send('Target.closeTarget', {
         targetId: this.#target._targetId,
       });
-      await this.#target._isClosedPromise;
+      await this.#target._isClosedPromise.valueOrThrow();
     }
   }
 

packages/puppeteer-core/src/common/Target.ts

@@ -249,6 +249,7 @@ export class PageTarget extends Target {
 
   protected override _initialize(): void {
     this._initializedPromise
+      .valueOrThrow()
       .then(async result => {
         if (result === InitializationStatus.ABORTED) {
           return;

packages/puppeteer-core/src/common/WaitTask.ts

@@ -18,6 +18,7 @@ import {ElementHandle} from '../api/ElementHandle.js';
 import {JSHandle} from '../api/JSHandle.js';
 import type {Poller} from '../injected/Poller.js';
 import {createDeferredPromise} from '../util/DeferredPromise.js';
+import {isErrorLike} from '../util/ErrorLike.js';
 import {stringifyFunction} from '../util/Function.js';
 
 import {TimeoutError} from './Errors.js';
@@ -97,7 +98,7 @@ export class WaitTask<T = unknown> {
   }
 
   get result(): Promise<HandleFor<T>> {
-    return this.#result;
+    return this.#result.valueOrThrow();
   }
 
   async rerun(): Promise<void> {
@@ -170,7 +171,7 @@ export class WaitTask<T = unknown> {
     }
   }
 
-  async terminate(error?: unknown): Promise<void> {
+  async terminate(error?: Error): Promise<void> {
     this.#world.taskManager.delete(this);
 
     if (this.#timeout) {
@@ -199,8 +200,8 @@ export class WaitTask<T = unknown> {
   /**
    * Not all errors lead to termination. They usually imply we need to rerun the task.
    */
-  getBadError(error: unknown): unknown {
-    if (error instanceof Error) {
+  getBadError(error: unknown): Error | undefined {
+    if (isErrorLike(error)) {
       // When frame is detached the task should have been terminated by the IsolatedWorld.
       // This can fail if we were adding this task while the frame was detached,
       // so we terminate here instead.
@@ -223,9 +224,14 @@ export class WaitTask<T = unknown> {
       if (error.message.includes('Cannot find context with specified id')) {
         return;
       }
+
+      return error;
     }
 
-    return error;
+    // @ts-expect-error TODO: uncomment once cause is supported in Node types.
+    return new Error('WaitTask failed with an error', {
+      cause: error,
+    });
   }
 }
 

packages/puppeteer-core/src/common/WebWorker.ts

@@ -91,14 +91,18 @@ export class WebWorker extends EventEmitter {
       this.#executionContext.resolve(context);
     });
     this.#client.on('Runtime.consoleAPICalled', async event => {
-      const context = await this.#executionContext;
-      return consoleAPICalled(
-        event.type,
-        event.args.map((object: Protocol.Runtime.RemoteObject) => {
-          return new CDPJSHandle(context, object);
-        }),
-        event.stackTrace
-      );
+      try {
+        const context = await this.#executionContext.valueOrThrow();
+        return consoleAPICalled(
+          event.type,
+          event.args.map((object: Protocol.Runtime.RemoteObject) => {
+            return new CDPJSHandle(context, object);
+          }),
+          event.stackTrace
+        );
+      } catch (err) {
+        debugError(err);
+      }
     });
     this.#client.on('Runtime.exceptionThrown', exception => {
       return exceptionThrown(exception.exceptionDetails);
@@ -112,7 +116,7 @@ export class WebWorker extends EventEmitter {
    * @internal
    */
   async executionContext(): Promise<ExecutionContext> {
-    return this.#executionContext;
+    return this.#executionContext.valueOrThrow();
   }
 
   /**
@@ -154,7 +158,7 @@ export class WebWorker extends EventEmitter {
       this.evaluate.name,
       pageFunction
     );
-    const context = await this.#executionContext;
+    const context = await this.#executionContext.valueOrThrow();
     return context.evaluate(pageFunction, ...args);
   }
 
@@ -181,7 +185,7 @@ export class WebWorker extends EventEmitter {
       this.evaluateHandle.name,
       pageFunction
     );
-    const context = await this.#executionContext;
+    const context = await this.#executionContext.valueOrThrow();
     return context.evaluateHandle(pageFunction, ...args);
   }
 }

packages/puppeteer-core/src/injected/Poller.ts

@@ -80,7 +80,7 @@ export class MutationPoller<T> implements Poller<T> {
 
   result(): Promise<T> {
     assert(this.#promise, 'Polling never started.');
-    return this.#promise;
+    return this.#promise.valueOrThrow();
   }
 }
 
@@ -126,7 +126,7 @@ export class RAFPoller<T> implements Poller<T> {
 
   result(): Promise<T> {
     assert(this.#promise, 'Polling never started.');
-    return this.#promise;
+    return this.#promise.valueOrThrow();
   }
 }
 
@@ -176,6 +176,6 @@ export class IntervalPoller<T> implements Poller<T> {
 
   result(): Promise<T> {
     assert(this.#promise, 'Polling never started.');
-    return this.#promise;
+    return this.#promise.valueOrThrow();
   }
 }

packages/puppeteer-core/src/util/DeferredPromise.ts

@@ -3,12 +3,13 @@ import {TimeoutError} from '../common/Errors.js';
 /**
  * @internal
  */
-export interface DeferredPromise<T> extends Promise<T> {
+export interface DeferredPromise<T> {
   finished: () => boolean;
   resolved: () => boolean;
   resolve: (value: T) => void;
-  reject: (reason?: unknown) => void;
-  value: () => T | undefined;
+  reject: (error: Error) => void;
+  value: () => T | Error | undefined;
+  valueOrThrow: () => Promise<T>;
 }
 
 /**
@@ -20,10 +21,10 @@ export interface DeferredPromiseOptions {
 }
 
 /**
- * Creates and returns a promise along with the resolve/reject functions.
+ * Creates and returns a deferred object along with the resolve/reject functions.
  *
- * If the promise has not been resolved/rejected within the `timeout` period,
- * the promise gets rejected with a timeout error. `timeout` has to be greater than 0 or
+ * If the deferred has not been resolved/rejected within the `timeout` period,
+ * the deferred gets resolves with a timeout error. `timeout` has to be greater than 0 or
  * it is ignored.
  *
  * @internal
@@ -33,42 +34,58 @@ export function createDeferredPromise<T>(
 ): DeferredPromise<T> {
   let isResolved = false;
   let isRejected = false;
-  let _value: T | undefined;
-  let resolver: (value: T) => void;
-  let rejector: (reason?: unknown) => void;
-  const taskPromise = new Promise<T>((resolve, reject) => {
+  let _value: T | Error | undefined;
+  let resolver: (value: void) => void;
+  const taskPromise = new Promise<void>(resolve => {
     resolver = resolve;
-    rejector = reject;
   });
   const timeoutId =
     opts && opts.timeout > 0
       ? setTimeout(() => {
-          isRejected = true;
-          rejector(new TimeoutError(opts.message));
+          reject(new TimeoutError(opts.message));
         }, opts.timeout)
       : undefined;
-  return Object.assign(taskPromise, {
+
+  function finish(value: T | Error) {
+    clearTimeout(timeoutId);
+    _value = value;
+    resolver();
+  }
+
+  function resolve(value: T) {
+    if (isRejected || isResolved) {
+      return;
+    }
+    isResolved = true;
+    finish(value);
+  }
+
+  function reject(error: Error) {
+    if (isRejected || isResolved) {
+      return;
+    }
+    isRejected = true;
+    finish(error);
+  }
+
+  return {
     resolved: () => {
       return isResolved;
     },
     finished: () => {
       return isResolved || isRejected;
     },
-    resolve: (value: T) => {
-      if (timeoutId) {
-        clearTimeout(timeoutId);
-      }
-      isResolved = true;
-      _value = value;
-      resolver(value);
-    },
-    reject: (err?: unknown) => {
-      clearTimeout(timeoutId);
-      isRejected = true;
-      rejector(err);
-    },
+    resolve,
+    reject,
     value: () => {
       return _value;
     },
-  });
+    async valueOrThrow() {
+      await taskPromise;
+      if (isRejected) {
+        throw _value;
+      }
+      return _value as T;
+    },
+  };
 }

test/src/DeferredPromise.spec.ts

@@ -0,0 +1,51 @@
+/**
+ * Copyright 2023 Google Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import expect from 'expect';
+import {
+  DeferredPromise,
+  createDeferredPromise,
+} from 'puppeteer-core/internal/util/DeferredPromise.js';
+
+describe('DeferredPromise', function () {
+  it('should catch errors', async () => {
+    // Async function before try/catch.
+    async function task() {
+      await new Promise(resolve => {
+        return setTimeout(resolve, 50);
+      });
+    }
+    // Async function that fails.
+    function fails(): DeferredPromise<void> {
+      const deferred = createDeferredPromise<void>();
+      setTimeout(() => {
+        deferred.reject(new Error('test'));
+      }, 25);
+      return deferred;
+    }
+
+    const expectedToFail = fails();
+    await task();
+    let caught = false;
+    try {
+      await expectedToFail.valueOrThrow();
+    } catch (err) {
+      expect((err as Error).message).toEqual('test');
+      caught = true;
+    }
+    expect(caught).toBeTruthy();
+  });
+});

test/src/DeviceRequestPrompt.spec.ts

@@ -1,3 +1,19 @@
+/**
+ * Copyright 2022 Google Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
 import expect from 'expect';
 import {TimeoutError} from 'puppeteer';
 import {