Skip to content
This repository was archived by the owner on Jul 29, 2024. It is now read-only.
This repository was archived by the owner on Jul 29, 2024. It is now read-only.

Remove internal uses of browser.executeAsyncScript #4053

Open
@sjelin

Description

@sjelin

We use browser.executeAsyncScript in browser.get to wait for angular to be loaded and in browser.waitForAngular in order to wait for our stability script. This can be a problem if a page unload happens while we're waiting for the callback to get called, leading to the notorious error message:

Detected a page unload event; async script execution does not work across page loads

(note: this error message may have changed. I haven't even confirmed that selenium bothers to detect unload events in v3, in which case we'd get timeout errors instead.)

We've been getting reports of this error forever (e.g. #85, #1651, #3713). Generally the problem is that the browser is initiating navigation, so it ends up happening outside our promise chaining/the Control Flow. This problem will be a bigger deal with #3857, since there could be executeAsyncScript calls happening at times when the user isn't expecting.

Proposed Implementation 1 (using browser.driver.wait)

If we follow the proposed implementation for #4052 we do this easily by replacing our internal executeAsyncScript_ with safeExecuteAsyncScript_:

private safeExecuteAsyncScript_(
    safeExecuteScriptWithDescription: SafeExecuteScript, asyncScript: string|Function,
    description: string, ...scriptArgs: any[]): wdpromise.Promise<void> {
  if (typeof asyncScript === 'function') {
    script = 'return (' + asyncScript + ').apply(null, arguments);';
  }
  asyncScript = clientSideScripts.wrapAsyncScriptForPolling(asyncScript);
  scriptArgs.unshift(asyncScript, description);
  return safeExecuteScriptWithDescription.apply(null, scriptArgs).then(() => {
    return this.driver.wait(() => {
      return safeExecuteScriptWithDescription(clientSideScripts.pollAsyncScript,
          description + 'checking if callback has been called');
    }, description);
  });
}

With the following additions to clientsidescripts.js:

exports.wrapAsyncScriptForPolling = function(async) {
  var unwrappedFn = 'function unwrappedFn() {' + script + '}';
  return wrapWithHelpers(function() {
    var callbackIndex = arguments.length;
    arguments[callbackIndex] = function() {
      getCallMetadata().asyncScriptCalled = true;
    };
    arguments.length = callbackIndex + 1;
    getCallMetadata().asyncScriptCalled = false;
    return unwrappedFn.apply(this, arguments);
  }, unwrappedFn);
}

functions.pollAsyncScript = function(safeCallUUID) {
  return getCallMetadata().asyncScriptCalled;
}

Proposed Implementation 2 (using unload listeners)

We can add an unload listener to the relevant client-side scripts, which calls the callback with an error if an unload occurs. This is actually how it's done in github.com/SeleniumHQ/selenium/javascript/atoms/inject.js#executeAsyncScript at the moment.

However, I'm not sure it would work for us. Their listener is set up first, so it would get invoked first, so we would still error. We could switch to beforeUnload, but still I'm not convinced we'd make it in time. We don't know how long between when our callback is called and their listener is removed. Plus, what if they switched to beforeUnload? On the whole, this just seems like an unstable, implementation-dependent approach.

Edge Case: Plugins

The plugin functions onPageLoad, onPageStable, onPageLoad and onPageStable many also want to execute asynchronous scripts. We should probably do

const safeExecuteAsyncScriptWithDescription = this.safeExecuteAsyncScript_.bind(this);

and then pass them safeExecuteAsyncScriptWithDescription.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions