Remove internal uses of browser.executeAsyncScript
#4053
Description
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
.