diff --git a/src/chromium/crNetworkManager.ts b/src/chromium/crNetworkManager.ts index 93a6bd2b0c..c04e283d27 100644 --- a/src/chromium/crNetworkManager.ts +++ b/src/chromium/crNetworkManager.ts @@ -220,8 +220,6 @@ export class CRNetworkManager { } const isNavigationRequest = requestWillBeSentEvent.requestId === requestWillBeSentEvent.loaderId && requestWillBeSentEvent.type === 'Document'; const documentId = isNavigationRequest ? requestWillBeSentEvent.loaderId : undefined; - if (isNavigationRequest) - this._page._frameManager.frameUpdatedDocumentIdForNavigation(requestWillBeSentEvent.frameId!, documentId!); const request = new InterceptableRequest({ client: this._client, frame, diff --git a/src/chromium/crPage.ts b/src/chromium/crPage.ts index 95ab316643..598fe5fadd 100644 --- a/src/chromium/crPage.ts +++ b/src/chromium/crPage.ts @@ -498,7 +498,7 @@ class FrameSession { _onFrameRequestedNavigation(payload: Protocol.Page.frameRequestedNavigationPayload) { if (payload.disposition === 'currentTab') - this._page._frameManager.frameRequestedNavigation(payload.frameId, ''); + this._page._frameManager.frameRequestedNavigation(payload.frameId); } _onFrameNavigatedWithinDocument(frameId: string, url: string) { diff --git a/src/firefox/ffPage.ts b/src/firefox/ffPage.ts index 304166ebf2..3886cbbd60 100644 --- a/src/firefox/ffPage.ts +++ b/src/firefox/ffPage.ts @@ -140,9 +140,7 @@ export class FFPage implements PageDelegate { } _onNavigationAborted(params: Protocol.Page.navigationAbortedPayload) { - const frame = this._page._frameManager.frame(params.frameId)!; - for (const task of frame._frameTasks) - task.onNewDocument(params.navigationId, new Error(params.errorText)); + this._page._frameManager.frameAbortedNavigation(params.frameId, params.errorText, params.navigationId); } _onNavigationCommitted(params: Protocol.Page.navigationCommittedPayload) { diff --git a/src/frames.ts b/src/frames.ts index f3be8b2af4..a4d67b337d 100644 --- a/src/frames.ts +++ b/src/frames.ts @@ -36,6 +36,13 @@ type ContextData = { rerunnableTasks: Set; }; +type DocumentInfo = { + // Unfortunately, we don't have documentId when we find out about + // a pending navigation from things like frameScheduledNavigaiton. + documentId: string | undefined, + request: network.Request | undefined, +}; + export type GotoResult = { newDocumentId?: string, }; @@ -125,20 +132,17 @@ export class FrameManager { barrier.release(); } - frameRequestedNavigation(frameId: string, documentId: string) { + frameRequestedNavigation(frameId: string, documentId?: string) { const frame = this._frames.get(frameId); if (!frame) return; for (const barrier of this._signalBarriers) barrier.addFrameNavigation(frame); - frame._pendingDocumentId = documentId; - } - - frameUpdatedDocumentIdForNavigation(frameId: string, documentId: string) { - const frame = this._frames.get(frameId); - if (!frame) + if (frame._pendingDocument && frame._pendingDocument.documentId === documentId) { + // Do not override request with undefined. return; - frame._pendingDocumentId = documentId; + } + frame._pendingDocument = { documentId, request: undefined }; } frameCommittedNewDocumentNavigation(frameId: string, url: string, name: string, documentId: string, initial: boolean) { @@ -146,11 +150,16 @@ export class FrameManager { this.removeChildFramesRecursively(frame); frame._url = url; frame._name = name; - debugAssert(!frame._pendingDocumentId || frame._pendingDocumentId === documentId); - frame._lastDocumentId = documentId; - frame._pendingDocumentId = ''; + if (frame._pendingDocument && frame._pendingDocument.documentId === undefined) + frame._pendingDocument.documentId = documentId; + debugAssert(!frame._pendingDocument || frame._pendingDocument.documentId === documentId); + if (frame._pendingDocument && frame._pendingDocument.documentId === documentId) + frame._currentDocument = frame._pendingDocument; + else + frame._currentDocument = { documentId, request: undefined }; + frame._pendingDocument = undefined; for (const task of frame._frameTasks) - task.onNewDocument(documentId); + task.onNewDocument(frame._currentDocument); this.clearFrameLifecycle(frame); if (!initial) this._page.emit(Events.Page.FrameNavigated, frame); @@ -166,6 +175,18 @@ export class FrameManager { this._page.emit(Events.Page.FrameNavigated, frame); } + frameAbortedNavigation(frameId: string, errorText: string, documentId?: string) { + const frame = this._frames.get(frameId); + if (!frame || !frame._pendingDocument) + return; + if (documentId !== undefined && frame._pendingDocument.documentId !== documentId) + return; + const pending = frame._pendingDocument; + frame._pendingDocument = undefined; + for (const task of frame._frameTasks) + task.onNewDocument(pending, new Error(errorText)); + } + frameDetached(frameId: string) { const frame = this._frames.get(frameId); if (frame) @@ -194,16 +215,17 @@ export class FrameManager { clearFrameLifecycle(frame: Frame) { frame._firedLifecycleEvents.clear(); // Keep the current navigation request if any. - frame._inflightRequests = new Set(Array.from(frame._inflightRequests).filter(request => request._documentId === frame._lastDocumentId)); + frame._inflightRequests = new Set(Array.from(frame._inflightRequests).filter(request => request === frame._currentDocument.request)); frame._stopNetworkIdleTimer(); if (frame._inflightRequests.size === 0) frame._startNetworkIdleTimer(); } requestStarted(request: network.Request) { + const frame = request.frame(); this._inflightRequestStarted(request); - for (const task of request.frame()._frameTasks) - task.onRequest(request); + if (request._documentId) + frame._pendingDocument = { documentId: request._documentId, request }; if (request._isFavicon) { const route = request._route(); if (route) @@ -225,27 +247,18 @@ export class FrameManager { } requestFailed(request: network.Request, canceled: boolean) { + const frame = request.frame(); this._inflightRequestFinished(request); - if (request._documentId) { - const isPendingDocument = request.frame()._pendingDocumentId === request._documentId; - if (isPendingDocument) { - request.frame()._pendingDocumentId = ''; - let errorText = request.failure()!.errorText; - if (canceled) - errorText += '; maybe frame was detached?'; - for (const task of request.frame()._frameTasks) - task.onNewDocument(request._documentId, new Error(errorText)); - } + if (frame._pendingDocument && frame._pendingDocument.request === request) { + let errorText = request.failure()!.errorText; + if (canceled) + errorText += '; maybe frame was detached?'; + this.frameAbortedNavigation(frame._id, errorText, frame._pendingDocument.documentId); } if (!request._isFavicon) this._page.emit(Events.Page.RequestFailed, request); } - provisionalLoadFailed(frame: Frame, documentId: string, error: string) { - for (const task of frame._frameTasks) - task.onNewDocument(documentId, new Error(error)); - } - private _notifyLifecycle(frame: Frame, lifecycleEvent: types.LifecycleEvent) { for (let parent: Frame | null = frame; parent; parent = parent.parentFrame()) { for (const frameTask of parent._frameTasks) @@ -301,8 +314,8 @@ export class FrameManager { export class Frame { _id: string; readonly _firedLifecycleEvents: Set; - _lastDocumentId = ''; - _pendingDocumentId = ''; + _currentDocument: DocumentInfo; + _pendingDocument?: DocumentInfo; _frameTasks = new Set(); readonly _page: Page; private _parentFrame: Frame | null; @@ -322,6 +335,7 @@ export class Frame { this._firedLifecycleEvents = new Set(); this._page = page; this._parentFrame = parentFrame; + this._currentDocument = { documentId: undefined, request: undefined }; this._detachedPromise = new Promise(x => this._detachedCallback = x); @@ -358,14 +372,14 @@ export class Frame { sameDocumentPromise.catch(e => {}); throw e; }); + let request: network.Request | undefined; if (navigateResult.newDocumentId) { // Do not leave sameDocumentPromise unhandled. sameDocumentPromise.catch(e => {}); - await frameTask.waitForSpecificDocument(navigateResult.newDocumentId); + request = await frameTask.waitForSpecificDocument(navigateResult.newDocumentId); } else { await sameDocumentPromise; } - const request = (navigateResult && navigateResult.newDocumentId) ? frameTask.request(navigateResult.newDocumentId) : null; await frameTask.waitForLifecycle(options.waitUntil === undefined ? 'load' : options.waitUntil); frameTask.done(); return request ? request._finalRequest().response() : null; @@ -377,12 +391,11 @@ export class Frame { const toUrl = typeof options.url === 'string' ? ` to "${options.url}"` : ''; progress.logger.info(`waiting for navigation${toUrl} until "${options.waitUntil || 'load'}"`); const frameTask = new FrameTask(this, progress); - let documentId: string | undefined; + let request: network.Request | undefined; await Promise.race([ - frameTask.waitForNewDocument(options.url).then(id => documentId = id), + frameTask.waitForNewDocument(options.url).then(r => request = r), frameTask.waitForSameDocumentNavigation(options.url), ]); - const request = documentId ? frameTask.request(documentId) : null; await frameTask.waitForLifecycle(options.waitUntil === undefined ? 'load' : options.waitUntil); frameTask.done(); return request ? request._finalRequest().response() : null; @@ -1026,11 +1039,10 @@ class SignalBarrier { class FrameTask { private readonly _frame: Frame; - private readonly _requestMap = new Map(); private readonly _progress: Progress | null = null; private _onSameDocument?: { url?: types.URLMatch, resolve: () => void }; - private _onSpecificDocument?: { expectedDocumentId: string, resolve: () => void, reject: (error: Error) => void }; - private _onNewDocument?: { url?: types.URLMatch, resolve: (documentId: string) => void, reject: (error: Error) => void }; + private _onSpecificDocument?: { expectedDocumentId: string, resolve: (request: network.Request | undefined) => void, reject: (error: Error) => void }; + private _onNewDocument?: { url?: types.URLMatch, resolve: (request: network.Request | undefined) => void, reject: (error: Error) => void }; private _onLifecycle?: { waitUntil: types.LifecycleEvent, resolve: () => void }; constructor(frame: Frame, progress: Progress | null) { @@ -1041,16 +1053,6 @@ class FrameTask { progress.cleanupWhenAborted(() => this.done()); } - onRequest(request: network.Request) { - if (!request._documentId || request.redirectedFrom()) - return; - this._requestMap.set(request._documentId, request); - } - - request(documentId: string): network.Request | undefined { - return this._requestMap.get(documentId); - } - onSameDocument() { if (this._progress) this._progress.logger.info(`navigated to "${this._frame._url}"`); @@ -1058,15 +1060,15 @@ class FrameTask { this._onSameDocument.resolve(); } - onNewDocument(documentId: string, error?: Error) { + onNewDocument(documentInfo: DocumentInfo, error?: Error) { if (this._progress && !error) this._progress.logger.info(`navigated to "${this._frame._url}"`); if (this._onSpecificDocument) { - if (documentId === this._onSpecificDocument.expectedDocumentId) { + if (documentInfo.documentId === this._onSpecificDocument.expectedDocumentId) { if (error) this._onSpecificDocument.reject(error); else - this._onSpecificDocument.resolve(); + this._onSpecificDocument.resolve(documentInfo.request); } else if (!error) { this._onSpecificDocument.reject(new Error('Navigation interrupted by another one')); } @@ -1075,7 +1077,7 @@ class FrameTask { if (error) this._onNewDocument.reject(error); else if (helper.urlMatches(this._frame.url(), this._onNewDocument.url)) - this._onNewDocument.resolve(documentId); + this._onNewDocument.resolve(documentInfo.request); } } @@ -1093,14 +1095,14 @@ class FrameTask { }); } - waitForSpecificDocument(expectedDocumentId: string): Promise { + waitForSpecificDocument(expectedDocumentId: string): Promise { return new Promise((resolve, reject) => { assert(!this._onSpecificDocument); this._onSpecificDocument = { expectedDocumentId, resolve, reject }; }); } - waitForNewDocument(url?: types.URLMatch): Promise { + waitForNewDocument(url?: types.URLMatch): Promise { return new Promise((resolve, reject) => { assert(!this._onNewDocument); this._onNewDocument = { url, resolve, reject }; diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 0a79f04d05..c1418d3f20 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -89,14 +89,14 @@ export class WKBrowser extends BrowserBase { const page = this._wkPages.get(payload.pageProxyId); if (!page) return; - const frameManager = page._page._frameManager; - const frame = frameManager.frame(payload.frameId); - if (frame) { - // In some cases, e.g. blob url download, we receive only frameScheduledNavigation - // but no signals that the navigation was canceled and replaced by download. Fix it - // here by simulating cancelled provisional load which matches downloads from network. - frameManager.provisionalLoadFailed(frame, '', 'Download is starting'); - } + // In some cases, e.g. blob url download, we receive only frameScheduledNavigation + // but no signals that the navigation was canceled and replaced by download. Fix it + // here by simulating cancelled provisional load which matches downloads from network. + // + // TODO: this is racy, because download might be unrelated any navigation, and we will + // abort navgitation that is still running. We should be able to fix this by + // instrumenting policy decision start/proceed/cancel. + page._page._frameManager.frameAbortedNavigation(payload.frameId, 'Download is starting'); let originPage = page._initializedPage; // If it's a new window download, report it on the opener page. if (!originPage) { diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index 6f58c4279d..a04c128ee9 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -248,7 +248,7 @@ export class WKPage implements PageDelegate { let errorText = event.error; if (errorText.includes('cancelled')) errorText += '; maybe frame was detached?'; - this._page._frameManager.provisionalLoadFailed(this._page.mainFrame(), event.loaderId, errorText); + this._page._frameManager.frameAbortedNavigation(this._page.mainFrame()._id, errorText, event.loaderId); } handleWindowOpen(event: Protocol.Playwright.windowOpenPayload) { @@ -366,7 +366,7 @@ export class WKPage implements PageDelegate { } private _onFrameScheduledNavigation(frameId: string) { - this._page._frameManager.frameRequestedNavigation(frameId, ''); + this._page._frameManager.frameRequestedNavigation(frameId); } private _onFrameStoppedLoading(frameId: string) { @@ -835,8 +835,6 @@ export class WKPage implements PageDelegate { // TODO(einbinder) this will fail if we are an XHR document request const isNavigationRequest = event.type === 'Document'; const documentId = isNavigationRequest ? event.loaderId : undefined; - if (isNavigationRequest) - this._page._frameManager.frameUpdatedDocumentIdForNavigation(event.frameId, documentId!); // We do not support intercepting redirects. const allowInterception = this._page._needsRequestInterception() && !redirectedFrom; const request = new WKInterceptableRequest(session, allowInterception, frame, event, redirectedFrom, documentId);