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

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 4, 2022

User facing changelog

Fixes multiple issues in Chrome 99:

  • No longer throws an exception when attempting to display a Document or ShadowRoot in the Cypress command log.
  • Stack traces were not reporting properly.

Additional details

Chrome 99 includes an updated version of v8, which breaks our stack trace trimming. A fix is included with commentary on why the change is needed.

It also updates the behavior around enumerating properties of StyleSheets. There's no good reason we should be doing this at all though.

How has the user experience changed?

In older versions, when a ShadowRoot was rendered to the command log, it would treat it as a Javascript object, and enumerate its properties, looking like this:

Screenshot_20220210_082740

After this change, it is treated like an HTML element, resulting in displaying this in the command log:

Screenshot_20220210_090443

The same is true for a Document. They now render their inner html to the command log rather than a verbose enumeration of their properties (which includes the innerHTML).

@github-actions github-actions bot requested a review from a team as a code owner February 4, 2022 08:01
@github-actions github-actions bot requested review from jennifer-shehane and removed request for a team February 4, 2022 08:01
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Feb 4, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Feb 4, 2022



Test summary

19177 0 218 0Flakiness 1


Run details

Project cypress
Status Passed
Commit d2cabd8
Started Feb 10, 2022 8:14 AM
Ended Feb 10, 2022 8:25 AM
Duration 11:39 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/integration/cypress/proxy-logging_spec.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@BlueWinds BlueWinds dismissed a stale review via ed0ed36 February 7, 2022 22:12
@BlueWinds BlueWinds force-pushed the update-chrome-stable-from-98.0.4758.80-beta-from-98.0.4758.80 branch from 2a4154c to ed0ed36 Compare February 7, 2022 22:12
@@ -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!

flotwig
flotwig previously approved these changes Feb 8, 2022
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

Ran bisect-builds against these changes to find what broke between this beta and stable:

bisect-chrome-builds.py --good=950365 --bad=961656 --command="zsh -c 'cd `pwd`/packages/driver && yarn cypress:run --spec **/type_spec.js --browser %p'" --archive=linux64 --use-local-cache --not-interactive --verify-range
Bad revision: 950808
You are probably looking for a change made after 950807 (known good), but no later than 950808 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/eef8f91d45d7b9d36f0e3cd1b6cd33d3bedff113..e5b482aff3e9e0031527108c7131861ed2640dcc

This is the only commit that changed between good and bad revisions: https://chromium.googlesource.com/chromium/src/+/e5b482aff3e9e0031527108c7131861ed2640dcc

Convert adoptedStyleSheets to use ObservableArray

This CL converts adoptedStyleSheets from a FrozenArray to the
new ObservableArray. This is per the new spec [1]. It should be
backwards compatible, and will allow mutation of adoptedStyleSheets
in place using regular array methods like .push():

  document.adoptedStyleSheets = [sheet1, sheet2];
  document.adoptedStyleSheets.push(sheet);
  ...etc...

Intent to ship:
https://groups.google.com/a/chromium.org/g/blink-dev/c/8p7QvGn3Ezo

[1] https://drafts.csswg.org/cssom/#extensions-to-the-document-or-shadow-root-interface

Bug: 1201744
Bug: 1236777
Change-Id: [Iacbf66a38c0abd477b5628224c97bec9f8a317e4](https://chromium-review.googlesource.com/#/q/Iacbf66a38c0abd477b5628224c97bec9f8a317e4)
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3270424
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Raphael Kubo Da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#950808}

Apparently it's supposed to be a backwards-compatible change. I wonder why it broke our usage here. Do you think we should file a bug with Chromium?

LGTM otherwise

packages/driver/src/cy/chai/inspect.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/chai/inspect.ts Outdated Show resolved Hide resolved
Co-authored-by: Zach Bloomquist <git@chary.us>
@BlueWinds
Copy link
Contributor

Apparently it's supposed to be a backwards-compatible change. I wonder why it broke our usage here. Do you think we should file a bug with Chromium?

LGTM otherwise

I'm fairly convinced this is a browser bug. After spending some time on it, I've reduced it to the following repro case:

for (var a in document.adoptedStyleSheets) {}

This errors on chrome-beta 99, and succeeds on chrome 98. I'll open a bug with Chrome on the subject.

With that said, we should still work around it for now - the change I applied to get the tests passing is a good one to have in place even if chrome fixes the issue at some point.

flotwig
flotwig previously approved these changes Feb 8, 2022
@flotwig
Copy link
Contributor

flotwig commented Feb 8, 2022

I think this should also be added to the release and changelog, to call out that we fixed a bug with versions of Chrome after 99.

@BlueWinds
Copy link
Contributor

Opened https://bugs.chromium.org/p/chromium/issues/detail?id=1295410. Will update issue to include user-facing changelog.

@BlueWinds BlueWinds changed the title chore: Update Chrome (beta) to 99.0.4844.17 fix: Fix issue which Chrome-beta (chrome 99) Feb 8, 2022
@github-actions github-actions bot changed the title fix: Fix issue which Chrome-beta (chrome 99) chore: Update Chrome (beta) to 99.0.4844.17 Feb 9, 2022
@github-actions github-actions bot changed the title chore: Update Chrome (beta) to 99.0.4844.17 chore: Update Chrome (beta) to 99.0.4844.27 Feb 10, 2022
@@ -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!

// (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.

@BlueWinds BlueWinds changed the title chore: Update Chrome (beta) to 99.0.4844.27 fix: Fix issues with stack traces and command log in Chrome 99 Feb 10, 2022
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

Yah, we'll have to put these changes in the changelog for sure.

@BlueWinds BlueWinds merged commit 1db8992 into develop Feb 10, 2022
@BlueWinds BlueWinds deleted the update-chrome-stable-from-98.0.4758.80-beta-from-98.0.4758.80 branch February 10, 2022 17:43
tgriesser added a commit that referenced this pull request Feb 14, 2022
* develop:
  feat: gray out the path to system node in cypress run header (#20121)
  feat: redesign server errors (#20072)
  test: fix awesome-typescript-loader test and remove test-binary job (#20131)
  fix: Fix issues with stack traces and command log in Chrome 99 (#20049)
  fix: `cy.type(' ')` fires click event on button-like elements. (#20067)
  fix: `change`, `input` events are not fired when the same option is selected again. (#19623)
  build: publish vue3 on latest (#20099)
  chore: release @cypress/webpack-preprocessor-v5.11.1
  chore: release @cypress/webpack-dev-server-v1.8.1
  fix: detect newly added specs in dev-server compilation (#17950)
  chore: Remove pkg/driver //@ts-nocheck part 3 (#19837)
  chore: set up semantic-pull-request GitHub Action (#20091)
  chore: release @cypress/react-v5.12.2
  fix: remove nullish coalescing in js files to support node 12 (#20094)
  docs: update @cypress/webpack-preprocessor links (#19902)
  refactor: use aliases instead of meta (#19566)
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Feb 15, 2022

Released in 9.5.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v9.5.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants