🐛 Fix situations where asset discovery might hang (#490)

* 🔊 Log a better message for network idle timeouts

* ♻ Move page resizing out of page init

🐛 And always pass the minHeight when resizing

* 🐛 Disable browser translate feature correctly

* 🐛 Automatically initialize subframe sessions

This allows request interception bindings to propagate down to subframes

* ♻ Unprivate some page properties

* 🐛 Track and properly cleanup frame requests

If a request for a document results in a new session, such as for isolated pages, the frame is
detached before the request's loading finished event gets a chance to fire. Requests for new frames
can be tracked and when the frame becomes detached the associated request can be cleaned up.

* ✅ Prefer localhost subdomains for some tests

* 🙈 Adjust coverage comments

* ♻ Refactor page navigation to be more reliable

The existing lifecycle handling combined with the polling nature of navigation resolution could
sometimes result in a situation where a page's lifecycle is reset before navigation can be deemed
complete. We can use the same technique watching for the proper lifecycle as we do for frame
navigation. However, rather than repeating code within the handlers, they were refactored slightly
to be mapped handlers. The navigation trigger was also refactored slightly into a callback that can
be called the moment we start waiting for the navigation handlers to finish.

* 🐛 Fix flakey redirect handling

Do to certain race conditions, intecepted redirects were not properly getting associated with an
intercept id. But since the previous request in the chain had been cleaned up, this could sometimes
cause asset discovery to hang. Adjusting the if-else logic of this handling fixes this issue by
ensuring that if a request is not being handled, it is always set up to be intercepted.

packages/core/src/browser.js

@@ -19,6 +19,7 @@ export default class Browser extends EventEmitter {
 
   defaultArgs = [
     '--enable-features=NetworkService,NetworkServiceInProcess',
+    '--disable-features=Translate',
     '--disable-background-networking',
     '--disable-background-timer-throttling',
     '--disable-backgrounding-occluded-windows',
@@ -28,7 +29,6 @@ export default class Browser extends EventEmitter {
     '--disable-default-apps',
     '--disable-dev-shm-usage',
     '--disable-extensions',
-    '--disable-features=TranslateUI',
     '--disable-hang-monitor',
     '--disable-ipc-flooding-protection',
     '--disable-popup-blocking',

packages/core/src/network.js

@@ -6,13 +6,16 @@ import {
   createRequestFailedHandler
 } from './discovery';
 
+const NETWORK_TIMEOUT = 30000;
+
 // The Interceptor class creates common handlers for dealing with intercepting asset requests
 // for a given page using various devtools protocol events and commands.
 export default class Network {
   #pending = new Map();
   #requests = new Map();
   #intercepts = new Map();
   #authentications = new Set();
+  #frames = new Map();
 
   log = logger('core:network');
 
@@ -25,6 +28,7 @@ export default class Network {
     this.page.on('Network.eventSourceMessageReceived', this._handleEventSourceMessageReceived);
     this.page.on('Network.loadingFinished', this._handleLoadingFinished);
     this.page.on('Network.loadingFailed', this._handleLoadingFailed);
+    this.page.on('Page.frameDetached', this._handleFrameDetached);
 
     /* istanbul ignore next: race condition */
     this.page.send('Network.enable')
@@ -47,25 +51,43 @@ export default class Network {
 
   // Resolves after the timeout when there are no more in-flight requests.
   async idle(filter = r => r, timeout = this.timeout || 100) {
+    let getRequests = () => Array.from(this.#requests.values())
+      .reduce((a, r) => filter(r) ? a.concat(r.url) : a, []);
+
     this.log.debug(`Wait for ${timeout}ms idle`, this.page.meta);
 
-    await waitFor(() => {
-      if (this.page.closedReason) {
-        throw new Error(`Network error: ${this.page.closedReason}`);
-      }
+    try {
+      await waitFor(() => {
+        if (this.page.closedReason) {
+          throw new Error(`Network error: ${this.page.closedReason}`);
+        }
 
-      return Array.from(this.#requests.values())
-        .filter(filter).length === 0;
-    }, {
-      timeout: 30 * 1000, // 30 second error timeout
-      idle: timeout
-    });
+        return getRequests().length === 0;
+      }, {
+        timeout: NETWORK_TIMEOUT,
+        idle: timeout
+      });
+    } catch (error) {
+      // throw a better timeout error
+      if (error.message.startsWith('Timeout')) {
+        let msg = 'Timed out waiting for network requests to idle.';
+
+        if (this.log.shouldLog('debug')) {
+          msg += `\n\n  ${['Active requests:', ...getRequests()].join('\n  -> ')}\n`;
+        }
+
+        throw new Error(msg);
+      } else {
+        throw error;
+      }
+    }
   }
 
   // Called when a request should be removed from various trackers
-  _forgetRequest({ requestId, interceptId }, keepPending) {
+  _forgetRequest({ requestId, interceptId, frameId }, keepPending) {
     this.#requests.delete(requestId);
     this.#authentications.delete(interceptId);
+    this.#frames.delete(frameId);
 
     if (!keepPending) {
       this.#pending.delete(requestId);
@@ -98,15 +120,12 @@ export default class Network {
   _handleRequestPaused = event => {
     let { networkId: requestId } = event;
     let pending = this.#pending.get(requestId);
+    this.#pending.delete(requestId);
 
     // guard against redirects with the same requestId
     if (pending?.request.url === event.request.url &&
         pending.request.method === event.request.method) {
       this._handleRequest(pending, event.requestId);
-    }
-
-    if (pending) {
-      this.#pending.delete(requestId);
     } else {
       this.#intercepts.set(requestId, event);
     }
@@ -137,18 +156,18 @@ export default class Network {
   // responses and calls this.onrequest with request info and callbacks to continue, respond,
   // or abort a request. One of the callbacks is required to be called and only one.
   _handleRequest = async (event, interceptId) => {
-    let { requestId, request } = event;
+    let { frameId, requestId, request } = event;
     let redirectChain = [];
 
     // if handling a redirected request, associate the response and add to its redirect chain
     if (event.redirectResponse && this.#requests.has(requestId)) {
       let req = this.#requests.get(requestId);
-      req.response = event.redirectResponse;
       redirectChain = [...req.redirectChain, req];
       // clean up interim requests
       this._forgetRequest(req, true);
     }
 
+    request.frameId = frameId;
     request.requestId = requestId;
     request.interceptId = interceptId;
     request.redirectChain = redirectChain;
@@ -194,6 +213,10 @@ export default class Network {
       let { body, base64Encoded } = await this.page.send('Network.getResponseBody', { requestId });
       return Buffer.from(body, base64Encoded ? 'base64' : 'utf8');
     };
+
+    if (request.frameId !== this.page.frameId) {
+      this.#frames.set(request.frameId, request);
+    }
   }
 
   // Called when a request streams events. These types of requests break asset discovery because
@@ -207,9 +230,8 @@ export default class Network {
   // Called when a request has finished loading which triggers the this.onrequestfinished
   // callback. The request should have an associated response and be finished with any redirects.
   _handleLoadingFinished = async event => {
-    let { requestId } = event;
-    let request = this.#requests.get(requestId);
-    /* istanbul ignore next: race condition paranioa */
+    let request = this.#requests.get(event.requestId);
+    /* istanbul ignore if: race condition paranioa */
     if (!request) return;
 
     if (this._intercept) {
@@ -221,16 +243,30 @@ export default class Network {
 
   // Called when a request has failed loading and triggers the this.onrequestfailed callback.
   _handleLoadingFailed = async event => {
-    let { requestId, errorText } = event;
-    let request = this.#requests.get(requestId);
+    let request = this.#requests.get(event.requestId);
     /* istanbul ignore if: race condition paranioa */
     if (!request) return;
 
     if (this._intercept) {
-      request.error = errorText;
+      request.error = event.errorText;
       await this.onrequestfailed(request);
     }
 
     this._forgetRequest(request);
   }
+
+  // Called after a frame detaches from the main frame. It's likely that the frame created its own
+  // process before the request finish event had a chance to be triggered.
+  _handleFrameDetached = async event => {
+    let request = this.#frames.get(event.frameId);
+    /* istanbul ignore if: race condition paranioa */
+    if (!request) return;
+
+    /* istanbul ignore else: could be false when a page is used without asset discovery */
+    if (this._intercept) {
+      await this.onrequestfinished(request);
+    }
+
+    this._forgetRequest(request);
+  }
 }