fix: do not use loaderId for lifecycle events (#8395)

This PR works around the upstream bug crbug.com/1325782. Previously Puppeteer relied on the presence of the loaderId to determine the kind of navigation and expected events. It does not look like there is a reason to do so: instead, we could see what events we get and proceed accordingly.
puppeteer · May 30, 2022 · c96c915 · c96c915
1 parent 5c235c7
commit c96c915
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 18 deletions.
diff --git a/src/common/FrameManager.ts b/src/common/FrameManager.ts
@@ -194,17 +194,15 @@ export class FrameManager extends EventEmitter {
     } = options;
 
     const watcher = new LifecycleWatcher(this, frame, waitUntil, timeout);
-    let ensureNewDocumentNavigation = false;
     let error = await Promise.race([
       navigate(this._client, url, referer, frame._id),
       watcher.timeoutOrTerminationPromise(),
     ]);
     if (!error) {
       error = await Promise.race([
         watcher.timeoutOrTerminationPromise(),
-        ensureNewDocumentNavigation
-          ? watcher.newDocumentNavigationPromise()
-          : watcher.sameDocumentNavigationPromise(),
+        watcher.newDocumentNavigationPromise(),
+        watcher.sameDocumentNavigationPromise(),
       ]);
     }
     watcher.dispose();
@@ -223,7 +221,6 @@ export class FrameManager extends EventEmitter {
           referrer,
           frameId,
         });
-        ensureNewDocumentNavigation = !!response.loaderId;
         return response.errorText
           ? new Error(`${response.errorText} at ${url}`)
           : null;

diff --git a/src/common/LifecycleWatcher.ts b/src/common/LifecycleWatcher.ts
@@ -66,7 +66,6 @@ export class LifecycleWatcher {
   _timeout: number;
   _navigationRequest: HTTPRequest | null = null;
   _eventListeners: PuppeteerEventListener[];
-  _initialLoaderId: string;
 
   _sameDocumentNavigationCompleteCallback: (x?: Error) => void = noop;
   _sameDocumentNavigationPromise = new Promise<Error | undefined>((fulfill) => {
@@ -94,6 +93,7 @@ export class LifecycleWatcher {
 
   _maximumTimer?: NodeJS.Timeout;
   _hasSameDocumentNavigation?: boolean;
+  _newDocumentNavigation?: boolean;
   _swapped?: boolean;
 
   constructor(
@@ -112,7 +112,6 @@ export class LifecycleWatcher {
 
     this._frameManager = frameManager;
     this._frame = frame;
-    this._initialLoaderId = frame._loaderId;
     this._timeout = timeout;
     this._eventListeners = [
       helper.addEventListener(
@@ -133,6 +132,11 @@ export class LifecycleWatcher {
         FrameManagerEmittedEvents.FrameNavigatedWithinDocument,
         this._navigatedWithinDocument.bind(this)
       ),
+      helper.addEventListener(
+        this._frameManager,
+        FrameManagerEmittedEvents.FrameNavigated,
+        this._navigated.bind(this)
+      ),
       helper.addEventListener(
         this._frameManager,
         FrameManagerEmittedEvents.FrameSwapped,
@@ -211,6 +215,12 @@ export class LifecycleWatcher {
     this._checkLifecycleComplete();
   }
 
+  _navigated(frame: Frame): void {
+    if (frame !== this._frame) return;
+    this._newDocumentNavigation = true;
+    this._checkLifecycleComplete();
+  }
+
   _frameSwapped(frame: Frame): void {
     if (frame !== this._frame) return;
     this._swapped = true;
@@ -221,19 +231,9 @@ export class LifecycleWatcher {
     // We expect navigation to commit.
     if (!checkLifecycle(this._frame, this._expectedLifecycle)) return;
     this._lifecycleCallback();
-    if (
-      this._frame._loaderId === this._initialLoaderId &&
-      !this._hasSameDocumentNavigation
-    ) {
-      if (this._swapped) {
-        this._swapped = false;
-        this._newDocumentNavigationCompleteCallback();
-      }
-      return;
-    }
     if (this._hasSameDocumentNavigation)
       this._sameDocumentNavigationCompleteCallback();
-    if (this._frame._loaderId !== this._initialLoaderId)
+    if (this._swapped || this._newDocumentNavigation)
       this._newDocumentNavigationCompleteCallback();
 
     /**