-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
fix: evaluateAsync behavior #1037
Conversation
88bfa18
to
8d29791
Compare
I think there are two issues coming out of this that we should investigate if WAI from the debugger protocol:
|
yeah this threw me for a loop at first, but it started to make sense from the perspective of "load this JS on the page" should be off when JS is off, but later commanding you specifically to run JS will still work (which is critical for our gatherer anyway) |
and there is an object populated but it has mostly information about where the error occurred in the script just not the usual suspects you would use to typically report a user-friendly message. any first steps where I should poke around to see if we can easily add those without disrupting a larger beast? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some preliminary notes.
Since we're now wrapping everything we evaluate in another promise layer anyways, we could move back to supporting sync expressions in evaluateAsync
, which would get rid of some noise in gatherers calling it
@@ -280,5 +280,7 @@ | |||
} | |||
</script> | |||
|
|||
<!-- Import zone.js to test Promise polyfill --> | |||
<script src="https://unpkg.com/zone.js?main=browser"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we've been avoiding any network for the plain smoke tests. Any way we could do a quick Promise wrapper that would just defer to native promise fns but fail the Promise === __nativePromise
check?
@@ -90,6 +90,7 @@ class GatherRunner { | |||
return driver.assertNoSameOriginServiceWorkerClients(options.url) | |||
.then(_ => driver.beginEmulation(options.flags)) | |||
.then(_ => driver.enableRuntimeEvents()) | |||
.then(_ => driver.evaluateScriptOnLoad('window.__nativePromise = Promise;')) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
// 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. | ||
/* istanbul ignore next */ | ||
const errorWrapper = err => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move this below (outside of the class) with captureJSCallUsage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
this.sendCommand('Runtime.evaluate', { | ||
expression: asyncExpression, | ||
expression: `(function wrapInNativePromise () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no space after function name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
this.sendCommand('Runtime.evaluate', { | ||
expression: asyncExpression, | ||
expression: `(function wrapInNativePromise () { | ||
var __nativePromise = window.__nativePromise || Promise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
(here and below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
expression: asyncExpression, | ||
expression: `(function wrapInNativePromise () { | ||
var __nativePromise = window.__nativePromise || Promise; | ||
return new __nativePromise(function (resolve) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no space after function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
that makes some sense, but it's not something we document for |
not sure. I did notice the devtools tests for |
8732398
to
5b10bf1
Compare
addresses GoogleChrome#1000 and GoogleChrome#976 1. Ensures all `Runtime.evaluate` calls result in the native Promise 2. Transforms all errors produced during the evaluation to a standard object that can be rejected by our driver wrapper
5b10bf1
to
46b8301
Compare
@ak239 could you take a look at this? It looks like you do similar stuff in native: https://cs.chromium.org/chromium/src/v8/src/inspector/v8-runtime-agent-impl.cc?q=f:v8-runtime+wrapEvaluateResultAsync&sq=package:chromium&l=215&dr=CSs .. and I want to make sure we're being robust in handling the error cases. And specifically you probably have a decent answer for @brendankenny's questions here: #1037 (comment) |
3494070
to
11ddfa7
Compare
Runtime.evaluate method with awaitPromise flag returns rejected value in result.result object. Protocol.Runtime.evaluate({ expression: 'Promise.reject(new Error())', awaitPromise: true })
.then(wrapErrorIfNeeded)
.then(console.log);
function wrapResultIfNeeded(message) {
if (message.error) {
// if protocol returns internal error - return it.
return `Internal error: ${message.error.message}`;
}
if (message.result.result.value) {
// if value has primitive type then return its value.
return message.result.result.value;
}
if (message.result.result.unserializableValue) {
// if value can't be represented in json (e.g. NaN) - evaluate unserializableValue.
return eval(message.result.result.unserializableValue);
}
if (message.result.result.objectId) {
// if values is object ..
if (message.result.result.subtype === "error") {
// .. and it's an error - call function on this object which will return JSON.stringifiable object.
return Protocol.Runtime.callFunctionOn({ objectId: message.result.result.objectId, functionDeclaration: wrapError.toString(), returnByValue: true }).then(wrapResultIfNeeded);
} else {
// .. otherwise do something, we can try to stringify it via protocol:
return Protocol.Runtime.callFunctionOn({ objectId: message.result.result.objectId, functionDeclaration: "function foo() { return this; }", returnByValue: true }).then(wrapResultIfNeeded);
}
}
// should be not reached.
return "Unknown error";
} I think you can use this to avoid try .. catch. |
discussed with @ak239 in person, summary:
@paulirish any other comments here? |
@@ -0,0 +1,257 @@ | |||
/* | |||
* @license |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
this.sendCommand('Runtime.evaluate', { | ||
expression: asyncExpression, | ||
expression: `(function wrapInNativePromise() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done :)
} else { | ||
resolve(result.result.value); | ||
resolve(value); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
I've another idea how to remove try catch:
Code snippet to demonstrate approach: this.sendCommand('Runtime.evaluate', {
expression: `(function wrapInNativePromise() {
const __nativePromise = window.__nativePromise || Promise;
return __nativePromise.resolve()
.then(_ => ${asyncExpression});
}())`,
includeCommandLineAPI: true,
awaitPromise: true
}).then(wrapResultIfNeeded.bind(this, undefined));
function wrapResultIfNeeded(isError, message) {
if (typeof isError !== 'boolean')
isError = !!message.exceptionDetails;
if (message.error) {
// if protocol returns internal error - return it.
return Promise.reject(`Internal error: ${message.error.message}`);
}
if (message.result.result.value) {
// if value has primitive type then return its value.
if (isError)
return Promise.reject(message.result.result.value);
else
return Promise.resolve(message.result.result.value);
}
if (message.result.result.unserializableValue) {
// if value can't be represented in json (e.g. NaN) - evaluate unserializableValue.
if (isError)
return Promise.reject(eval(message.result.result.unserializableValue));
else
return Promise.resolve(eval(message.result.result.unserializableValue));
}
if (message.result.result.objectId) {
// if values is object ..
if (message.result.result.subtype === "error") {
// .. and it's an error - call function on this object which will return JSON.stringifiable object.
return Protocol.Runtime.callFunctionOn({ objectId: message.result.result.objectId, functionDeclaration: wrapError.toString(), returnByValue: true }).then(wrapResultIfNeeded.bind(this, isError));
} else {
// .. otherwise do something, we can try to stringify it via protocol:
return Protocol.Runtime.callFunctionOn({ objectId: message.result.result.objectId, functionDeclaration: "function foo() { return this; }", returnByValue: true }).then(wrapResultIfNeeded.bind(this, isError));
}
}
// should be not reached.
return "Unknown error";
}
function wrapError()
{
// this - Error object, do something smart and return stringifiable object.
return ({
customError: true,
name: this.name || 'Error',
message: this.message || 'fallbackMessage',
stack: this.stack || (new Error()).stack,
__failedInBrowser: true
});
} |
@ak239 what's the benefit to using that approach over the new seems like either way we have to execute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, a few more things. Also need to update evaluateAsync
jsdocs to drop the must evaluate to a promise part
@@ -24,7 +24,9 @@ const parseURL = require('url').parse; | |||
function requestHandler(request, response) { | |||
const filePath = parseURL(request.url).pathname; | |||
const queryString = parseURL(request.url).search; | |||
const absoluteFilePath = path.join(__dirname, filePath); | |||
const absoluteFilePath = filePath === '/dobetterweb/promise_polyfill.js' ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a comment here for why this is done?
I also feel like this is also going to be a long term resident in static-server
, so a full if statement is probably better so it's a bit quicker mentally parse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} else { | ||
resolve(result.result.value); | ||
resolve(value); |
There was a problem hiding this comment.
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
if (result.exceptionDetails) { | ||
reject(result.exceptionDetails.exception.value); | ||
// An error occurred before we could even enter our try block, should be *very* rare |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no more try block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,257 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rviscomi for another third_party/
thing to delete for import. Sorry about this, but we need to modify it to be able to use.
Good news is that this is for testing only, so you can just delete? Not sure how you deal with third_party/traceviewer-js/
today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This will be great for evaluateAsync errors
1. Ensures all `Runtime.evaluate` calls result in the native Promise 2. Transforms all errors produced during the evaluation to a standard object that can be rejected by our driver wrapper
R: @brendankenny @ebidel
addresses #1000 and #976
Runtime.evaluate
calls result in the native Promise