chore: introduce DocumentInfo (#2765)

It encapsulates documentId and request.

Author: dgozman

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,

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) {

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) {

src/frames.ts

@@ -36,6 +36,13 @@ type ContextData = {
   rerunnableTasks: Set<RerunnableTask>;
 };
 
+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,32 +132,34 @@ 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) {
     const frame = this._frames.get(frameId)!;
     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<types.LifecycleEvent>;
-  _lastDocumentId = '';
-  _pendingDocumentId = '';
+  _currentDocument: DocumentInfo;
+  _pendingDocument?: DocumentInfo;
   _frameTasks = new Set<FrameTask>();
   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<void>(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<string, network.Request>();
   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,32 +1053,22 @@ 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}"`);
     if (this._onSameDocument && helper.urlMatches(this._frame.url(), this._onSameDocument.url))
       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<void> {
+  waitForSpecificDocument(expectedDocumentId: string): Promise<network.Request | undefined> {
     return new Promise((resolve, reject) => {
       assert(!this._onSpecificDocument);
       this._onSpecificDocument = { expectedDocumentId, resolve, reject };
     });
   }
 
-  waitForNewDocument(url?: types.URLMatch): Promise<string> {
+  waitForNewDocument(url?: types.URLMatch): Promise<network.Request | undefined> {
     return new Promise((resolve, reject) => {
       assert(!this._onNewDocument);
       this._onNewDocument = { url, resolve, reject };

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) {

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);