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.

Sign up for GitHub

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
merged 7 commits into from
Feb 10, 2022
Merged

fix: Fix issues with stack traces and command log in Chrome 99 #20049

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2921c16
chore: Update Chrome (beta) to 99.0.4844.17
Feb 4, 2022
74e964e
Merge branch 'develop' into update-chrome-stable-from-98.0.4758.80-be…
emilyrohrbough Feb 4, 2022
ed0ed36
Attempt to fix failures with chrome-beta
BlueWinds Feb 7, 2022
6b489e1
Fix stack_utils on new chrome beta
BlueWinds Feb 7, 2022
3424f90
Better fix for chrome 99 stack traces
BlueWinds Feb 8, 2022
ac6af16
Apply suggestions from code review
Feb 8, 2022
d2cabd8
chore: Update Chrome (beta) to 99.0.4844.27
Feb 10, 2022
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
2 changes: 1 addition & 1 deletion browser-versions.json
View file
Open in desktop
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
View file
Open in desktop
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.
let isShadowRoot = function (object) {
BlueWinds marked this conversation as resolved.
Show resolved Hide resolved
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.
let isDocument = function (object) {
BlueWinds marked this conversation as resolved.
Show resolved Hide resolved
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 •
edited
Loading

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
View file
Open in desktop
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