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: Fix issues with stack traces and command log in Chrome 99 #20049

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion browser-versions.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"chrome:beta": "98.0.4758.80",
"chrome:beta": "99.0.4844.17",
"chrome:stable": "98.0.4758.80"
}
20 changes: 20 additions & 0 deletions packages/driver/src/cy/chai/inspect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,18 @@ export function create (chai) {
typeof object.nodeName === 'string'
}

// We can't just check if object instanceof ShadowRoot, because it might be the document of an iframe,
// which in Chrome 99+ is a separate class, and instanceof ShadowRoot returns false.
const isShadowRoot = function (object) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this change with chrome has impact on the implementation of the cy.shadow() command?

Copy link
Contributor

Choose a reason for hiding this comment

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

So no tests in the shadom dom specs required changing, but there is a change in the way it's displayed. I'm updating the PR description with details, since it's user-facing.

Unintentional, but I think the new version is much cleaner, so looks like a feature rather than a bug to me!

return isDOMElement(object.host) && object.host.shadowRoot === object
}

// We can't just check if object instanceof Document, because it might be the document of an iframe,
// which in Chrome 99+ is a separate class, and instanceof Document returns false.
const isDocument = function (object) {
return object.defaultView && object.defaultView === object.defaultView.window
}

let formatValueHook

const setFormatValueHook = (fn) => formatValueHook = fn
Expand Down Expand Up @@ -124,6 +136,14 @@ export function create (chai) {
}
}

if (isShadowRoot(value)) {
Copy link
Contributor

@BlueWinds BlueWinds Feb 8, 2022

Choose a reason for hiding this comment

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

When we start recursively inspecting objects, listing out their properties, this can include elements with references to objects like window and document. And in Chrome 99, when we iterate over the properties of document.adoptedStyleSheets, it throws an error.

Ultimately though, there's no reason we should be looking over every property of a Document. In the same way we already abort for HTMLElements above, I added an early exit for ShadowRoot and Document, returning a string of their HTML rather than recursively enumerating their properties. This should be a minor performance win, saving on hundreds of calls through formatValue in cases where we pass in window, document or other global-ish objects that aren't literal HTMLElements.

No tests have been added, because the output of this function is already comprehensively tested - dozens of tests failed in Chrome 99 before the changes to this file.

Copy link
Member

Choose a reason for hiding this comment

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

🥳 yay to improved performance!

return value.innerHTML
}

if (isDocument(value)) {
return value.documentElement.outerHTML
}

// Look up the keys of the object.
let visibleKeys = getEnumerableProperties(value)
let keys = ctx.showHidden ? getProperties(value) : visibleKeys
Expand Down
10 changes: 9 additions & 1 deletion packages/driver/src/cypress/stack_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,21 @@ const captureUserInvocationStack = (ErrorConstructor, userInvocationStack?) => {
if (!userInvocationStack) {
const newErr = new ErrorConstructor('userInvocationStack')

userInvocationStack = newErr.stack

// if browser natively supports Error.captureStackTrace, use it (chrome) (must be bound)
// otherwise use our polyfill on top.Error
const captureStackTrace = ErrorConstructor.captureStackTrace ? ErrorConstructor.captureStackTrace.bind(ErrorConstructor) : Error.captureStackTrace

captureStackTrace(newErr, captureUserInvocationStack)

userInvocationStack = newErr.stack
// On Chrome 99+, captureStackTrace strips away the whole stack,
// leaving nothing beyond the error message. If we get back a single line
// (just the error message with no stack trace), then use the original value
// instead of the trimmed one.
if (newErr.stack.match('\n')) {
userInvocationStack = newErr.stack
Copy link
Member

Choose a reason for hiding this comment

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

I'm not understanding this comment 100%, which one is the trimmed one?
Above on line 142, we are setting userInvocationStack = newErr.stack so regardless of this check, won't the stack be the newError stack?

Copy link
Contributor

Choose a reason for hiding this comment

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

captureStackTrace modifies the error object in place. So for example, beforehand, newErr.stack might will be 15 lines. Then we call captureStackTrace(newErr, captureUserInvocationStack), and now newErr.stack is only 10 lines long.

This guard is against the case - in Chrome 99 - where it was already only two lines, and gets reduced to one. Eg, it might be

Err: Foobar is bad!
  at (localhost:3500/cypress/stuff/myspec.js:54:12)

and it gets trimmed down to

Err: Foobar is bad!

I'm not 100% sure what caused the changed behavior of captureStackTrace in chromium 99, but the first stack trace is far more useful than the second.

Copy link
Member

Choose a reason for hiding this comment

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

ahhh the fact captureStackTrace modifies this as well was the part I was missing! Thanks for the extra info.

}
}

userInvocationStack = normalizedUserInvocationStack(userInvocationStack)
Expand Down