From c96c915b535dcf414038677bd3d3ed6b980a4901 Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Mon, 30 May 2022 10:30:30 +0200 Subject: [PATCH] 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. --- src/common/FrameManager.ts | 7 ++----- src/common/LifecycleWatcher.ts | 26 +++++++++++++------------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/src/common/FrameManager.ts b/src/common/FrameManager.ts index c9e3960e6f942..f3dcc32488375 100644 --- a/src/common/FrameManager.ts +++ b/src/common/FrameManager.ts @@ -194,7 +194,6 @@ 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(), @@ -202,9 +201,8 @@ export class FrameManager extends EventEmitter { 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 index 16f2b94d7c2ec..a9731410a2a26 100644 --- 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((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(); /**