Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: evaluateAsync behavior #1037

Merged
merged 7 commits into from
Dec 3, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
Original file line number Diff line number Diff line change
Expand Up @@ -280,5 +280,6 @@ <h2>Do better web tester page</h2>
}
</script>

<script src="./promise_polyfill.js"></script>
</body>
</html>
257 changes: 257 additions & 0 deletions lighthouse-cli/test/fixtures/dobetterweb/promise_polyfill.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,257 @@
/*
* @license
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be in lighthouse-core/third_party/ (or better, the npm version...should be fine since just a dev dependency)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brendankenny suggestions on getting this loaded by the static server? just add static check to fetch this from third party?
and i wish we could just do the npm version but this polyfill won't work on its own since chrome already defines Promise so I modified it, we could try to find another one that storms over it instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya lets put in third_party. the static server can just look for a request to promise_polyfill and reach across to adjust the absoluteFilePath

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* From taylorhakes/promise-polyfill

* Copyright (c) 2014 Taylor Hakes
* Copyright (c) 2014 Forbes Lindesay

* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:

* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.

* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
'use strict';

(function() {
/* eslint-disable */
// Store setTimeout reference so promise-polyfill will be unaffected by
// other code modifying setTimeout (like sinon.useFakeTimers())
var setTimeoutFunc = setTimeout;

function noop() {}

// Polyfill for Function.prototype.bind
function bind(fn, thisArg) {
return function () {
fn.apply(thisArg, arguments);
};
}

function Promise(fn) {
if (typeof this !== 'object') throw new TypeError('Promises must be constructed via new');
if (typeof fn !== 'function') throw new TypeError('not a function');
this._state = 0;
this._handled = false;
this._value = undefined;
this._deferreds = [];

doResolve(fn, this);
}

function handle(self, deferred) {
while (self._state === 3) {
self = self._value;
}
if (self._state === 0) {
self._deferreds.push(deferred);
return;
}
self._handled = true;
Promise._immediateFn(function () {
var cb = self._state === 1 ? deferred.onFulfilled : deferred.onRejected;
if (cb === null) {
(self._state === 1 ? resolve : reject)(deferred.promise, self._value);
return;
}
var ret;
try {
ret = cb(self._value);
} catch (e) {
reject(deferred.promise, e);
return;
}
resolve(deferred.promise, ret);
});
}

function resolve(self, newValue) {
try {
// Promise Resolution Procedure: https://github.com/promises-aplus/promises-spec#the-promise-resolution-procedure
if (newValue === self) throw new TypeError('A promise cannot be resolved with itself.');
if (newValue && (typeof newValue === 'object' || typeof newValue === 'function')) {
var then = newValue.then;
if (newValue instanceof Promise) {
self._state = 3;
self._value = newValue;
finale(self);
return;
} else if (typeof then === 'function') {
doResolve(bind(then, newValue), self);
return;
}
}
self._state = 1;
self._value = newValue;
finale(self);
} catch (e) {
reject(self, e);
}
}

function reject(self, newValue) {
self._state = 2;
self._value = newValue;
finale(self);
}

function finale(self) {
if (self._state === 2 && self._deferreds.length === 0) {
Promise._immediateFn(function() {
if (!self._handled) {
Promise._unhandledRejectionFn(self._value);
}
});
}

for (var i = 0, len = self._deferreds.length; i < len; i++) {
handle(self, self._deferreds[i]);
}
self._deferreds = null;
}

function Handler(onFulfilled, onRejected, promise) {
this.onFulfilled = typeof onFulfilled === 'function' ? onFulfilled : null;
this.onRejected = typeof onRejected === 'function' ? onRejected : null;
this.promise = promise;
}

/**
* Take a potentially misbehaving resolver function and make sure
* onFulfilled and onRejected are only called once.
*
* Makes no guarantees about asynchrony.
*/
function doResolve(fn, self) {
var done = false;
try {
fn(function (value) {
if (done) return;
done = true;
resolve(self, value);
}, function (reason) {
if (done) return;
done = true;
reject(self, reason);
});
} catch (ex) {
if (done) return;
done = true;
reject(self, ex);
}
}

Promise.prototype['catch'] = function (onRejected) {
return this.then(null, onRejected);
};

Promise.prototype.then = function (onFulfilled, onRejected) {
var prom = new (this.constructor)(noop);

handle(this, new Handler(onFulfilled, onRejected, prom));
return prom;
};

Promise.all = function (arr) {
var args = Array.prototype.slice.call(arr);

return new Promise(function (resolve, reject) {
if (args.length === 0) return resolve([]);
var remaining = args.length;

function res(i, val) {
try {
if (val && (typeof val === 'object' || typeof val === 'function')) {
var then = val.then;
if (typeof then === 'function') {
then.call(val, function (val) {
res(i, val);
}, reject);
return;
}
}
args[i] = val;
if (--remaining === 0) {
resolve(args);
}
} catch (ex) {
reject(ex);
}
}

for (var i = 0; i < args.length; i++) {
res(i, args[i]);
}
});
};

Promise.resolve = function (value) {
if (value && typeof value === 'object' && value.constructor === Promise) {
return value;
}

return new Promise(function (resolve) {
resolve(value);
});
};

Promise.reject = function (value) {
return new Promise(function (resolve, reject) {
reject(value);
});
};

Promise.race = function (values) {
return new Promise(function (resolve, reject) {
for (var i = 0, len = values.length; i < len; i++) {
values[i].then(resolve, reject);
}
});
};

// Use polyfill for setImmediate for performance gains
Promise._immediateFn = (typeof setImmediate === 'function' && function (fn) { setImmediate(fn); }) ||
function (fn) {
setTimeoutFunc(fn, 0);
};

Promise._unhandledRejectionFn = function _unhandledRejectionFn(err) {
if (typeof console !== 'undefined' && console) {
console.warn('Possible Unhandled Promise Rejection:', err); // eslint-disable-line no-console
}
};

/**
* Set the immediate function to execute callbacks
* @param fn {function} Function to execute
* @deprecated
*/
Promise._setImmediateFn = function _setImmediateFn(fn) {
Promise._immediateFn = fn;
};

/**
* Change the function to execute on unhandled rejection
* @param {function} fn Function to execute on unhandled rejection
* @deprecated
*/
Promise._setUnhandledRejectionFn = function _setUnhandledRejectionFn(fn) {
Promise._unhandledRejectionFn = fn;
};

window.Promise = Promise;

/* eslint-enable */
})();
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ class LinkBlockingFirstPaintAudit extends Audit {
if (typeof artifact === 'undefined' || artifact.value === -1) {
return {
rawValue: -1,
debugString: 'TagsBlockingFirstPaint gatherer did not run'
debugString: (artifact && artifact.debugString) ||
'TagsBlockingFirstPaint gatherer did not run'
};
}

Expand Down
42 changes: 39 additions & 3 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,17 +143,33 @@ class Driver {
(_ => reject(new Error('The asynchronous expression exceeded the allotted time of 60s'))),
60000
);

this.sendCommand('Runtime.evaluate', {
expression: asyncExpression,
expression: `(function wrapInNativePromise() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels like it could be simplified, but are there reasons for everything? Why wrap in a promise constructor and try/catch and Promise.resolve() instead of doing a single promise wrapper?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also update the function docs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes each has a purpose

try/catch - for errors that happen outside of promises
Promise.resolve - to enable sync executions
new Promise - to ensure the promise returned is indeed a native promise + avoid inconsistent error handling between sync and async paths

but as I'm typing this just remembering that we opted for Promise.resolve().then( => ), will fix

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanna add a comment to document this? we're definitely gonna be headscratching if we need to touch this code again. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

const __nativePromise = window.__nativePromise || Promise;
return new __nativePromise(function(resolve) {
const wrapError = ${wrapRuntimeEvalErrorInBrowser.toString()};
try {
__nativePromise.resolve(${asyncExpression}).then(resolve, wrapError);
} catch (e) {
wrapError(e);
}
});
}())`,
includeCommandLineAPI: true,
awaitPromise: true,
returnByValue: true
}).then(result => {
clearTimeout(asyncTimeout);
const value = result.result.value;

if (result.exceptionDetails) {
reject(result.exceptionDetails.exception.value);
// An error occurred before we could even enter our try block, should be *very* rare
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no more try block

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

reject(new Error('an unknown driver error occurred'));
} if (value && value.__failedInBrowser) {
reject(Object.assign(new Error(), value));
} else {
resolve(result.result.value);
resolve(value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can save for another issue (maybe this is also #941), but the fact that there are three cases here

  • success
  • failure due to driver error
  • failure (of some sort) due to the evaluated expression

makes it feel like we shouldn't be conflating the last two, but I'm not exactly sure of an elegant way to do this, or even what gatherers should do (catch or re-throw) if we do differentiate by type

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the differentiation is just done by what error message and stack trace results. Unless we think driver error should be fatal?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think @paulirish is addressing this in his latest comments in #941. We basically need a way to say "this is an error I expected (fetch rejected on offline request or whatever)" vs "whoooops". For this, the caller of driver.evaluateAsync will have to tell the difference for now and we can revisit as we figure out error paths

}
}).catch(err => {
clearTimeout(asyncTimeout);
Expand Down Expand Up @@ -713,4 +729,24 @@ function captureJSCallUsage(funcRef, set) {
};
}

/**
* The `exceptionDetails` provided by the debugger protocol does not contain the useful
* information such as name, message, and stack trace of the error when it's wrapped in a
* promise. Instead, map to a successful object that contains this information.
* @param {string|Error} err The error to convert
* istanbul ignore next
*/
function wrapRuntimeEvalErrorInBrowser(err) {
/* global resolve */
err = err || new Error();
const fallbackMessage = typeof err === 'string' ? err : 'unknown error';

resolve({
__failedInBrowser: true,
name: err.name || 'Error',
message: err.message || fallbackMessage,
stack: err.stack || (new Error()).stack,
});
}

module.exports = Driver;
7 changes: 5 additions & 2 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ const path = require('path');
* C. GatherRunner.setupDriver()
* i. assertNoSameOriginServiceWorkerClients
* ii. beginEmulation
* iii. cleanAndDisableBrowserCaches
* iiii. clearDataForOrigin
* iii. enableRuntimeEvents
* iv. evaluateScriptOnLoad rescue native Promise from potential polyfill
* v. cleanAndDisableBrowserCaches
* vi. clearDataForOrigin
*
* 2. For each pass in the config:
* A. GatherRunner.beforePass()
Expand Down Expand Up @@ -90,6 +92,7 @@ class GatherRunner {
return driver.assertNoSameOriginServiceWorkerClients(options.url)
.then(_ => driver.beginEmulation(options.flags))
.then(_ => driver.enableRuntimeEvents())
.then(_ => driver.evaluateScriptOnLoad('window.__nativePromise = Promise;'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to doc this in outline at top

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

.then(_ => driver.cleanAndDisableBrowserCaches())
.then(_ => driver.clearDataForOrigin(options.url));
}
Expand Down
6 changes: 5 additions & 1 deletion lighthouse-core/gather/gatherers/accessibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,13 @@ class Accessibility extends Gatherer {

afterPass(options) {
const driver = options.driver;
const expression = `(function () {
${axe};
return (${runA11yChecks.toString()}());
})()`;

return driver
.evaluateAsync(`${axe};(${runA11yChecks.toString()}())`)
.evaluateAsync(expression)
.then(returnedValue => {
if (!returnedValue) {
this.artifact = Accessibility._errorAccessibility('Unable to parse axe results');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ function collectTagsThatBlockFirstPaint() {
});
resolve(tagList);
} catch (e) {
reject(`Unable to gather Scripts/Stylesheets/HTML Imports on the page: ${e.message}`);
const friendly = 'Unable to gather Scripts/Stylesheets/HTML Imports on the page';
reject(new Error(`${friendly}: ${e.message}`));
}
});
}
Expand Down Expand Up @@ -117,10 +118,10 @@ class TagsBlockingFirstPaint extends Gatherer {
.then(artifact => {
this.artifact = artifact;
})
.catch(debugString => {
.catch(err => {
this.artifact = {
value: -1,
debugString
debugString: err.message
};
});
}
Expand Down
Loading