fix(puppet): extract navigation loader

Author: blakebyrnes

commons/Resolvable.ts

@@ -31,15 +31,15 @@ export default class Resolvable<T = any> implements IResolvablePromise<T>, Promi
 
   public resolve(value: T | PromiseLike<T>): void {
     if (this.isResolved) return;
-    this.isResolved = true;
     clearTimeout(this.timeout);
     this.resolveFn(value);
     Promise.resolve(value)
       // eslint-disable-next-line promise/always-return
       .then(x => {
+        this.isResolved = true;
         this.resolved = x;
       })
-      .catch(() => null);
+      .catch(this.reject);
   }
 
   public reject(error: Error): void {

core/lib/DetachedTabState.ts

@@ -56,7 +56,7 @@ export default class DetachedTabState {
     );
     await Promise.all([
       page.mainFrame.waitForLoader(loader.loaderId),
-      page.mainFrame.waitForLoad('DOMContentLoaded'),
+      page.mainFrame.waitForLifecycleEvent('DOMContentLoaded', loader.loaderId),
       InjectedScripts.installDetachedScripts(page),
     ]);
     await InjectedScripts.restoreDom(page, this.domChanges);

core/lib/FrameEnvironment.ts

@@ -60,7 +60,7 @@ export default class FrameEnvironment {
   }
 
   public get isAttached(): boolean {
-    return this.puppetFrame.isAttached();
+    return this.puppetFrame.isAttached;
   }
 
   public get securityOrigin(): string {
@@ -442,8 +442,10 @@ b) Use the UserProfile feature to set cookies for 1 or more domains before they'
 
     for (const [event, url, timestamp] of loadEvents) {
       const incomingStatus = pageStateToLoadStatus[event];
-
-      this.navigations.onLoadStateChanged(incomingStatus, url, null, new Date(timestamp));
+      // only record the content paint
+      if (incomingStatus === LoadStatus.ContentPaint) {
+        this.navigations.onLoadStateChanged(incomingStatus, url, null, new Date(timestamp));
+      }
     }
 
     this.sessionState.captureDomEvents(
@@ -571,7 +573,11 @@ b) Use the UserProfile feature to set cookies for 1 or more domains before they'
     else if (lowerEventName === 'domcontentloaded') status = LoadStatus.DomContentLoaded;
 
     if (status) {
-      this.navigations.onLoadStateChanged(status, event.frame.url, event.loaderId);
+      this.navigations.onLoadStateChanged(
+        status,
+        event.loader.url ?? event.frame.url,
+        event.loader.id,
+      );
     }
   }
 

core/lib/FrameNavigations.ts

@@ -37,7 +37,7 @@ export default class FrameNavigations extends TypedEventEmitter<IFrameNavigation
 
   public logger: IBoundLog;
 
-  private loaderIds = new Set<string>();
+  private historyByLoaderId: { [loaderId: string]: INavigation } = {};
 
   private nextNavigationReason: { url: string; reason: NavigationReason };
 
@@ -73,7 +73,7 @@ export default class FrameNavigations extends TypedEventEmitter<IFrameNavigation
       resourceId: createPromise(),
       browserRequestId,
     };
-    if (loaderId) this.loaderIds.add(loaderId);
+    if (loaderId) this.historyByLoaderId[loaderId] = nextTop;
 
     this.checkStoredNavigationReason(nextTop, url);
 
@@ -149,7 +149,7 @@ export default class FrameNavigations extends TypedEventEmitter<IFrameNavigation
     else if (
       top.stateChanges.has(LocationStatus.HttpRequested) === true &&
       // add new entries for redirects
-      (!this.loaderIds.has(loaderId) || redirectedFromUrl)
+      (!this.historyByLoaderId[loaderId] || redirectedFromUrl)
     ) {
       this.onNavigationRequested(reason, url, lastCommandId, loaderId, browserRequestId);
     }
@@ -184,7 +184,7 @@ export default class FrameNavigations extends TypedEventEmitter<IFrameNavigation
   }
 
   public onLoadStateChanged(
-    incomingStatus: LoadStatus.DomContentLoaded | LoadStatus.Load,
+    incomingStatus: LoadStatus.DomContentLoaded | LoadStatus.Load | LoadStatus.ContentPaint,
     url: string,
     loaderId: string,
     statusChangeDate?: Date,
@@ -194,7 +194,8 @@ export default class FrameNavigations extends TypedEventEmitter<IFrameNavigation
     if (!loaderId) {
       for (let i = this.history.length - 1; i >= 0; i -= 1) {
         const nav = this.history[i];
-        if (nav && nav.finalUrl === url && nav.stateChanges.has(LoadStatus.HttpResponded)) {
+        const isUrlMatch = nav.finalUrl === url || nav.requestedUrl === url;
+        if (isUrlMatch && nav.stateChanges.has(LoadStatus.HttpResponded)) {
           loaderId = nav.loaderId;
           break;
         }
@@ -219,7 +220,8 @@ export default class FrameNavigations extends TypedEventEmitter<IFrameNavigation
 
   public assignLoaderId(navigation: INavigation, loaderId: string, url?: string): void {
     if (!loaderId) return;
-    this.loaderIds.add(loaderId);
+
+    this.historyByLoaderId[loaderId] = navigation;
     navigation.loaderId = loaderId;
     if (
       url &&
@@ -232,13 +234,17 @@ export default class FrameNavigations extends TypedEventEmitter<IFrameNavigation
 
   public getLastLoadedNavigation(): INavigation {
     let navigation: INavigation;
+    let hasInPageNav = false;
     for (let i = this.history.length - 1; i >= 0; i -= 1) {
       navigation = this.history[i];
-      if (
-        navigation.stateChanges.has(LoadStatus.DomContentLoaded) &&
-        navigation.navigationReason !== 'inPage' &&
-        !!navigation.finalUrl
-      ) {
+      if (navigation.navigationReason === 'inPage') {
+        hasInPageNav = true;
+        continue;
+      }
+      if (!navigation.finalUrl || !navigation.stateChanges.has(LoadStatus.HttpResponded)) continue;
+
+      // if we have an in-page nav, return the first non "inPage" url. Otherwise, use if DomContentLoaded was triggered
+      if (hasInPageNav || navigation.stateChanges.has(LoadStatus.DomContentLoaded)) {
         return navigation;
       }
     }
@@ -257,18 +263,7 @@ export default class FrameNavigations extends TypedEventEmitter<IFrameNavigation
   }
 
   private findMatchingNavigation(loaderId: string): INavigation {
-    const navigation = this.top;
-    if (!navigation) return undefined;
-    if (loaderId && navigation.loaderId && navigation.loaderId !== loaderId) {
-      // find the right loader id
-      for (let i = this.history.length - 1; i >= 0; i -= 1) {
-        const nav = this.history[i];
-        if (nav && nav.loaderId === loaderId) {
-          return nav;
-        }
-      }
-    }
-    return navigation;
+    return this.historyByLoaderId[loaderId] ?? this.top;
   }
 
   private recordRedirect(requestedUrl: string, finalUrl: string, loaderId: string): INavigation {
@@ -281,6 +276,7 @@ export default class FrameNavigations extends TypedEventEmitter<IFrameNavigation
     }
 
     // find the right loader id
+    // NOTE: loop through history since loaderId is reused across requests in a redirect
     for (let i = this.history.length - 1; i >= 0; i -= 1) {
       const navigation = this.history[i];
       if (navigation && navigation.loaderId === loaderId) {
@@ -301,16 +297,24 @@ export default class FrameNavigations extends TypedEventEmitter<IFrameNavigation
     loaderId?: string,
     statusChangeDate?: Date,
   ): void {
+    this.logger.info('FrameNavigations.changeNavigationState', {
+      newStatus,
+      loaderId,
+      statusChangeDate,
+    });
     const navigation = this.findMatchingNavigation(loaderId);
     if (!navigation) return;
+    if (!navigation.loaderId && loaderId) {
+      navigation.loaderId = loaderId;
+      this.historyByLoaderId[loaderId] = navigation;
+    }
     if (navigation.stateChanges.has(newStatus)) {
       if (statusChangeDate && statusChangeDate < navigation.stateChanges.get(newStatus)) {
         navigation.stateChanges.set(newStatus, statusChangeDate);
       }
       return;
     }
     this.recordStatusChange(navigation, newStatus, statusChangeDate);
-    if (loaderId) this.loaderIds.add(loaderId);
   }
 
   private recordStatusChange(

core/lib/Session.ts

@@ -432,7 +432,7 @@ export default class Session extends TypedEventEmitter<{
   ) {
     const tab = Tab.create(this, page, false, parentTab, {
       ...openParams,
-      loaderId: page.mainFrame.isDefaultUrl ? null : page.mainFrame.activeLoaderId,
+      loaderId: page.mainFrame.isDefaultUrl ? null : page.mainFrame.activeLoader.id,
     });
     this.sessionState.captureTab(tab.id, page.id, page.devtoolsSession.id, parentTab.id);
     this.registerTab(tab, page);

core/lib/Tab.ts

@@ -173,7 +173,7 @@ export default class Tab extends TypedEventEmitter<ITabEventParams> {
       resource.request?.url,
       resource.response?.url,
     );
-    if (frame) {
+    if (frame && !resource.isRedirect) {
       frame.navigations.onResourceLoaded(resource.id, resource.response?.statusCode, error);
       return true;
     }
@@ -366,8 +366,11 @@ export default class Tab extends TypedEventEmitter<ITabEventParams> {
     const timeoutMessage = `Timeout waiting for "tab.goto(${url})"`;
 
     const timer = new Timer(timeoutMs, this.waitTimeouts);
-    await timer.waitForPromise(this.puppetPage.navigate(formattedUrl), timeoutMessage);
-    this.navigations.assignLoaderId(navigation, this.puppetPage.mainFrame.activeLoaderId);
+    const loader = await timer.waitForPromise(
+      this.puppetPage.navigate(formattedUrl),
+      timeoutMessage,
+    );
+    this.navigations.assignLoaderId(navigation, loader.loaderId);
 
     const resource = await timer.waitForPromise(
       this.navigationsObserver.waitForNavigationResourceId(),
@@ -384,7 +387,11 @@ export default class Tab extends TypedEventEmitter<ITabEventParams> {
       null,
     );
     const backUrl = await this.puppetPage.goBack();
-    this.navigations.assignLoaderId(navigation, this.puppetPage.mainFrame.activeLoaderId, backUrl);
+    this.navigations.assignLoaderId(
+      navigation,
+      this.puppetPage.mainFrame.activeLoader?.id,
+      backUrl,
+    );
 
     await this.navigationsObserver.waitForLoad('PaintingStable', { timeoutMs });
     return this.url;
@@ -398,7 +405,7 @@ export default class Tab extends TypedEventEmitter<ITabEventParams> {
       null,
     );
     const url = await this.puppetPage.goForward();
-    this.navigations.assignLoaderId(navigation, this.puppetPage.mainFrame.activeLoaderId, url);
+    this.navigations.assignLoaderId(navigation, this.puppetPage.mainFrame.activeLoader?.id, url);
     await this.navigationsObserver.waitForLoad('PaintingStable', { timeoutMs });
     return this.url;
   }
@@ -415,7 +422,7 @@ export default class Tab extends TypedEventEmitter<ITabEventParams> {
     const timeoutMessage = `Timeout waiting for "tab.reload()"`;
 
     await timer.waitForPromise(this.puppetPage.reload(), timeoutMessage);
-    this.navigations.assignLoaderId(navigation, this.puppetPage.mainFrame.activeLoaderId);
+    this.navigations.assignLoaderId(navigation, this.puppetPage.mainFrame.activeLoader?.id);
 
     const resource = await timer.waitForPromise(
       this.navigationsObserver.waitForNavigationResourceId(),
@@ -462,7 +469,7 @@ export default class Tab extends TypedEventEmitter<ITabEventParams> {
       await newTab.navigations.waitOn('navigation-requested', null, timeoutMs).catch(() => null);
     }
 
-    await newTab.mainFrameEnvironment.navigationsObserver.waitForNavigationResourceId();
+    await newTab.navigationsObserver.waitForNavigationResourceId();
     return newTab;
   }
 

core/test/domRecorder.test.ts

@@ -216,8 +216,8 @@ function sort() {
     const state = tab.sessionState;
 
     expect(tab.puppetPage.frames).toHaveLength(4);
-    await tab.puppetPage.frames[1].waitForLoad();
-    await tab.puppetPage.frames[2].waitForLoad();
+    await tab.puppetPage.frames[1].waitForLifecycleEvent('load');
+    await tab.puppetPage.frames[2].waitForLifecycleEvent('load');
     // await tab.puppetPage.frames[3].waitOn('frame-lifecycle', f => f.name === 'load');
 
     await state.db.flush();