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 captureJSCallUsage in the face of Error polyfills. #1218

Merged
merged 6 commits into from
Dec 21, 2016

Conversation

wardpeet
Copy link
Collaborator

V8 has the property prepareStackTrace on errors where you can format a stacktrace. This only works on native errors not on custom ones.

Fixes issues like #1194

@wardpeet
Copy link
Collaborator Author

wardpeet commented Dec 21, 2016

@note need to write a test

url to test on https://cc3test.careercruising.com/?debug=1

Copy link
Contributor

@ebidel ebidel left a comment

Choose a reason for hiding this comment

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

Nice catch. Looks like some linting and smoke test failures from travis.

@@ -712,19 +712,21 @@ class Driver {
* @param {function(...*): *} funcRef The function call to track.
* @param {!Set} set An empty set to populate with stack traces. Should be
* on the global object.
* @param {Error} nativeError The origal error object some libraries overwrite it.
Copy link
Contributor

Choose a reason for hiding this comment

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

NativeError

origal -> original

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks :)

const originalFunc = funcRef;
const originalPrepareStackTrace = Error.prepareStackTrace;
const originalPrepareStackTrace = nativeError.prepareStackTrace;
Copy link
Contributor

Choose a reason for hiding this comment

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

NativeError

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks :)

…s overwritten)

V8 has the property prepareStackTrace on errors where you can format a stacktrace. This only works on native errors not on custom ones.
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

really nice detective work! Looking good, mostly styling stuff

@@ -88,6 +88,7 @@
<a href="./doesnotexist" target="_blank">internal link is ok</a>
</template>

<script>window.Error = function(error) { this.stack = 'stacktrace'; };</script>
Copy link
Member

Choose a reason for hiding this comment

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

will you add a comment about why we're doing this and maybe referencing #1194 for future reference?

@@ -701,7 +701,7 @@ class Driver {

this.evaluateScriptOnLoad(`
${globalVarToPopulate} = new Set();
(${funcName} = ${funcBody}(${funcName}, ${globalVarToPopulate}))`);
(${funcName} = ${funcBody}(${funcName}, ${globalVarToPopulate}, window.__nativeError))`);
Copy link
Member

Choose a reason for hiding this comment

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

should we inject this here or just have the script assume window.__nativeError exists like evaluateAsync does? I'm not sure which is better, but it may be a bit cleaner to just use window.__nativeError inside captureJSCallUsage

@@ -94,7 +94,8 @@ class GatherRunner {
return driver.assertNoSameOriginServiceWorkerClients(options.url)
.then(_ => driver.beginEmulation(options.flags))
.then(_ => driver.enableRuntimeEvents())
.then(_ => driver.evaluateScriptOnLoad('window.__nativePromise = Promise;'))
.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.

will you pull this out into a separate function (driver.cacheNatives or something)? I'm starting to get the feeling these aren't the only two we'll end up needing to cache :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Great work 🦎 ➡️ ❌

@brendankenny brendankenny merged commit 39cc6a7 into GoogleChrome:master Dec 21, 2016
@ebidel
Copy link
Contributor

ebidel commented Dec 21, 2016

I felt a great disturbance in the Force, as if dozens of Github issues suddenly cried out in terror, and were suddenly closed. I fear something terrible terrific has happened.

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017
…1218)

V8 has the property prepareStackTrace on errors where you can format a stacktrace. This only works on native errors not on custom ones.
@wardpeet wardpeet deleted the fix-native-error branch July 24, 2017 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants