fix: Properly perform page readiness validation

lib/mixins/navigate.js

@@ -5,10 +5,8 @@ import { timing, util } from '@appium/support';
 import _ from 'lodash';
 import B from 'bluebird';
 
-
-/**
- * @typedef {(() => Promise<any>|void)|undefined} TPageLoadVerifyHook
- */
+const DEFAULT_PAGE_READINESS_TIMEOUT_MS = 1000 * 60; // 1 minute
+const PAGE_READINESS_CHECK_INTERVAL_MS = 30;
 
 /**
  * @this {import('../remote-debugger').RemoteDebugger}
@@ -20,59 +18,69 @@ function frameDetached () {
 
 /**
  * @this {import('../remote-debugger').RemoteDebugger}
- * @param {timing.Timer?} startPageLoadTimer
- * @param {TPageLoadVerifyHook} [pageLoadVerifyHook]
+ * @param {timing.Timer?} [startPageLoadTimer]
  * @returns {Promise<void>}
  */
-async function pageLoad (startPageLoadTimer, pageLoadVerifyHook = _.noop) {
-  const timeoutMs = 500;
+async function pageLoad (startPageLoadTimer) {
   if (!_.isFunction(startPageLoadTimer?.getDuration)) {
     log.debug(`Page load timer not a timer. Creating new timer`);
     startPageLoadTimer = new timing.Timer().start();
   }
 
-  log.debug('Page loaded, verifying whether ready');
+  const readinessTimeoutMs = this.pageLoadMs || DEFAULT_PAGE_READINESS_TIMEOUT_MS;
+  log.debug(`Page loaded, waiting up to ${readinessTimeoutMs}ms until it is ready`);
   this.pageLoading = true;
+  this.pageLoadDelay = util.cancellableDelay(readinessTimeoutMs);
 
-  const verify = async () => {
-    this.pageLoadDelay = util.cancellableDelay(timeoutMs);
+  /** @type {B} */
+  const pageReadinessCancellationListenerPromise = B.resolve((async () => {
     try {
       await this.pageLoadDelay;
-    } catch (err) {
-      if (err instanceof B.CancellationError) {
-        // if the promise has been cancelled
-        // we want to skip checking the readiness
+    } catch (ign) {
+      this.pageLoading = false;
+    }
+  })());
+
+  /** @type {B} */
+  const pageReadinessPromise = B.resolve((async () => {
+    while (this.pageLoading) {
+      await B.delay(PAGE_READINESS_CHECK_INTERVAL_MS);
+      // we can get this called in the middle of trying to find a new app
+      if (!this.appIdKey) {
+        log.debug('Not connected to an application. Ignoring page readiess check');
+        return;
+      }
+      if (!this.pageLoading) {
         return;
       }
-    }
 
-    // we can get this called in the middle of trying to find a new app
-    if (!this.appIdKey) {
-      log.debug('Not connected to an application. Ignoring page load');
-      return;
+      // if we are ready, or we've spend too much time on this
+      // @ts-ignore startPageLoadTimer is defined here
+      const elapsedMs = startPageLoadTimer.getDuration().asMilliSeconds;
+      if (elapsedMs > readinessTimeoutMs) {
+        log.info(`Timed out after ${readinessTimeoutMs}ms of waiting for the page readiness. Continuing anyway`);
+        this.pageLoading = false;
+        return;
+      }
+      if (await this.checkPageIsReady()) {
+        log.debug(`Page is ready in ${elapsedMs}ms`);
+        this.pageLoading = false;
+        return;
+      }
     }
+  })());
 
-    if (_.isFunction(pageLoadVerifyHook)) {
-      await pageLoadVerifyHook();
+  try {
+    await B.any([pageReadinessCancellationListenerPromise, pageReadinessPromise]);
+    if (pageReadinessPromise.isPending()) {
+      await pageReadinessPromise;
     }
-
-    // if we are ready, or we've spend too much time on this
-    // @ts-ignore startPageLoadTimer is defined here
-    const elapsedMs = startPageLoadTimer.getDuration().asMilliSeconds;
-    if (await this.checkPageIsReady() || (this.pageLoadMs > 0 && elapsedMs > this.pageLoadMs)) {
-      log.debug('Page is ready');
-      this.pageLoading = false;
-    } else {
-      log.debug('Page was not ready, retrying');
-      await verify();
+    if (pageReadinessCancellationListenerPromise.isPending()) {
+      this.pageLoadDelay.cancel();
     }
-  };
-  try {
-    await verify();
   } finally {
-    // @ts-ignore startPageLoadTimer is defined here
-    log.debug(`Page load completed in ${startPageLoadTimer.getDuration().asMilliSeconds.toFixed(0)}ms`);
     this.pageLoading = false;
+    this.pageLoadDelay = B.resolve();
   }
 }
 /**
@@ -99,12 +107,11 @@ async function pageUnload () {
 /**
  * @this {import('../remote-debugger').RemoteDebugger}
  * @param {timing.Timer|null|undefined} startPageLoadTimer
- * @param {TPageLoadVerifyHook} [pageLoadVerifyHook]
  * @returns {Promise<void>}
  */
-async function waitForDom (startPageLoadTimer, pageLoadVerifyHook) {
+async function waitForDom (startPageLoadTimer) {
   log.debug('Waiting for dom...');
-  await this.pageLoad(startPageLoadTimer, pageLoadVerifyHook);
+  await this.pageLoad(startPageLoadTimer);
 }
 
 /**
@@ -114,30 +121,24 @@ async function waitForDom (startPageLoadTimer, pageLoadVerifyHook) {
 async function checkPageIsReady () {
   checkParams({appIdKey: this.appIdKey});
 
-  log.debug('Checking document readyState');
   const readyCmd = 'document.readyState;';
-  let readyState = 'loading';
   try {
-    readyState = await B.resolve(this.execute(readyCmd, true)).timeout(this.pageReadyTimeout);
+    return await B.resolve(this.execute(readyCmd, true)).timeout(this.pageReadyTimeout) === 'complete';
   } catch (err) {
     if (!(err instanceof B.TimeoutError)) {
       throw err;
     }
-    log.debug(`Page readiness check timed out after ${this.pageReadyTimeout}ms`);
-    return false;
   }
-  log.debug(`Document readyState is '${readyState}'`);
-
-  return readyState === 'complete';
+  log.debug(`Page readiness check timed out after ${this.pageReadyTimeout}ms`);
+  return false;
 }
 
 /**
  * @this {import('../remote-debugger').RemoteDebugger}
  * @param {string} url
- * @param {TPageLoadVerifyHook} [pageLoadVerifyHook]
  * @returns {Promise<void>}
  */
-async function navToUrl (url, pageLoadVerifyHook) {
+async function navToUrl (url) {
   checkParams({appIdKey: this.appIdKey, pageIdKey: this.pageIdKey});
 
   if (!this.rpcClient) {
@@ -166,7 +167,7 @@ async function navToUrl (url, pageLoadVerifyHook) {
     // wait until the page has been navigated
     await waitForFramePromise;
 
-    await this.waitForDom(new timing.Timer().start(), pageLoadVerifyHook);
+    await this.waitForDom(new timing.Timer().start());
 
     // enable console logging, so we get the events (otherwise we only
     // get notified when navigating to a local page

lib/remote-debugger.js

@@ -61,9 +61,9 @@ class RemoteDebugger extends EventEmitter {
   checkPageIsReady;
   /** @type {(dict: Record<string, any>) => void} */
   updateAppsWithDict;
-  /** @type {(startPageLoadTimer?: import('@appium/support').timing.Timer, pageLoadVerifyHook?: import('./mixins/navigate').TPageLoadVerifyHook) => Promise<void>} */
+  /** @type {(startPageLoadTimer?: import('@appium/support').timing.Timer) => Promise<void>} */
   waitForDom;
-  /** @type {(startPageLoadTimer?: import('@appium/support').timing.Timer?, pageLoadVerifyHook?: import('./mixins/navigate').TPageLoadVerifyHook) => Promise<void>} */
+  /** @type {(startPageLoadTimer?: import('@appium/support').timing.Timer?) => Promise<void>} */
   pageLoad;
   /** @type {(command: string, override?: boolean) => Promise<any>} */
   execute;