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: performance regression due to enabling ScopeMemoryCachePerContext #28327

Merged
merged 6 commits into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ _Released 11/21/2023 (PENDING)_

**Bugfixes:**

- Fixed a regression in [`13.5.0`](https://docs.cypress.io/guides/references/changelog/13.5.0) where requests cached within a given spec may take longer to load than they did previously. Addresses [#28295](https://github.com/cypress-io/cypress/issues/28295).
- Fixed an issue where pages opened in a new tab were missing response headers, causing them not to load properly. Fixes [#28293](https://github.com/cypress-io/cypress/issues/28293) and [#28303](https://github.com/cypress-io/cypress/issues/28303).
- We now pass a flag to Chromium browsers to disable default component extensions. This is a common flag passed during browser automation. Fixed in [#28294](https://github.com/cypress-io/cypress/pull/28294).

Expand Down
13 changes: 13 additions & 0 deletions packages/driver/cypress/e2e/commands/actions/click.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -1678,12 +1678,25 @@ describe('src/cy/commands/actions/click', () => {
it('can scroll to and click elements in html with scroll-behavior: smooth', () => {
cy.get('html').invoke('css', 'scrollBehavior', 'smooth')
cy.get('#table tr:first').click()
// Validate that the scrollBehavior is still smooth even after the actionability fixes we do
cy.get('html').invoke('css', 'scrollBehavior').then((scrollBehavior) => expect(scrollBehavior).to.eq('smooth'))
})

// https://github.com/cypress-io/cypress/issues/28150
it('can scroll to and click elements in html with scroll-behavior: smooth and overflow-y: auto', () => {
cy.get('html').invoke('css', 'scrollBehavior', 'smooth')
cy.get('body').invoke('css', 'overflow-y', 'auto')
cy.get('#table tr:first').click()
// Validate that the scrollBehavior is still smooth even after the actionability fixes we do
cy.get('html').invoke('css', 'scrollBehavior').then((scrollBehavior) => expect(scrollBehavior).to.eq('smooth'))
})

// https://github.com/cypress-io/cypress/issues/3200
it('can scroll to and click elements in ancestor element with scroll-behavior: smooth', () => {
cy.get('#dom').invoke('css', 'scrollBehavior', 'smooth')
cy.get('#table tr:first').click()
// Validate that the scrollBehavior is still smooth even after the actionability fixes we do
cy.get('#dom').invoke('css', 'scrollBehavior').then((scrollBehavior) => expect(scrollBehavior).to.eq('smooth'))
})
})
})
Expand Down
25 changes: 24 additions & 1 deletion packages/driver/cypress/e2e/dom/visibility.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('src/cypress/dom/visibility', () => {
expect(fn()).to.be.true
})

it('returns false window and body > window height', () => {
it('returns false if window and body < window height', () => {
Copy link
Member

Choose a reason for hiding this comment

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

what this title wrong before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Things should only be scrollable if they're > than the height not <.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically this is the opposite of line 45.

cy.$$('body').html('<div>foo</div>')

const win = cy.state('window')
Expand All @@ -65,6 +65,29 @@ describe('src/cypress/dom/visibility', () => {
expect(fn()).to.be.false
})

it('returns true if document element and body > window height', function () {
this.add('<div style="height: 1000px; width: 10px;" />')
const documentElement = Cypress.dom.wrap(cy.state('document').documentElement)

const fn = () => {
return dom.isScrollable(documentElement)
}

expect(fn()).to.be.true
})

it('returns false if document element and body < window height', () => {
cy.$$('body').html('<div>foo</div>')

const documentElement = Cypress.dom.wrap(cy.state('document').documentElement)

const fn = () => {
return dom.isScrollable(documentElement)
}

expect(fn()).to.be.false
})

it('returns false el is not scrollable', function () {
const noScroll = this.add(`\
<div style="height: 100px; overflow: auto;">
Expand Down
44 changes: 33 additions & 11 deletions packages/driver/src/cy/actionability.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import $utils from './../cypress/utils'
import type { ElWindowPostion, ElViewportPostion, ElementPositioning } from '../dom/coordinates'
import $elements from '../dom/elements'
import $errUtils from '../cypress/error_utils'
import { callNativeMethod, getNativeProp } from '../dom/elements/nativeProps'
const debug = debugFn('cypress:driver:actionability')

const delay = 50
Expand Down Expand Up @@ -460,24 +461,46 @@ const verify = function (cy, $el, config, options, callbacks: VerifyCallbacks) {
// make scrolling occur instantly. we do this by adding a style tag
// and then removing it after we finish scrolling
// https://github.com/cypress-io/cypress/issues/3200
const addScrollBehaviorFix = () => {
let style
const addScrollBehaviorFix = (element: JQuery<HTMLElement>) => {
const affectedParents: Map<HTMLElement, string> = new Map()

try {
const doc = $el.get(0).ownerDocument
let parent: JQuery<HTMLElement> | null = element

style = doc.createElement('style')
style.innerHTML = '* { scroll-behavior: inherit !important; }'
// there's guaranteed to be a <script> tag, so that's the safest thing
// to query for and add the style tag after
doc.querySelector('script').after(style)
do {
if ($dom.isScrollable(parent)) {
const parentElement = parent[0]
const style = getNativeProp(parentElement, 'style')
const styles = getComputedStyle(parentElement)

if (styles.scrollBehavior === 'smooth') {
affectedParents.set(parentElement, callNativeMethod(style, 'getStyleProperty', 'scroll-behavior'))
callNativeMethod(style, 'setStyleProperty', 'scroll-behavior', 'auto')
}
}

parent = $dom.getParent(parent)
} while (parent.length)
} catch (err) {
// the above shouldn't error, but out of an abundance of caution, we
// ignore any errors since this fix isn't worth failing the test over
}

return () => {
if (style) style.remove()
for (const [parent, value] of affectedParents) {
const style = getNativeProp(parent, 'style')

if (value === '') {
if (callNativeMethod(style, 'getStyleProperty', 'length') === 1) {
callNativeMethod(parent, 'removeAttribute', 'style')
} else {
callNativeMethod(style, 'removeProperty', 'scroll-behavior')
}
} else {
callNativeMethod(style, 'setStyleProperty', 'scroll-behavior', value)
}
}
affectedParents.clear()
}
}

Expand All @@ -500,8 +523,7 @@ const verify = function (cy, $el, config, options, callbacks: VerifyCallbacks) {
if (options.scrollBehavior !== false) {
// scroll the element into view
const scrollBehavior = scrollBehaviorOptionsMap[options.scrollBehavior]

const removeScrollBehaviorFix = addScrollBehaviorFix()
const removeScrollBehaviorFix = addScrollBehaviorFix($el)

debug('scrollIntoView:', $el[0])
$el.get(0).scrollIntoView({ block: scrollBehavior })
Expand Down
6 changes: 6 additions & 0 deletions packages/driver/src/dom/elements/complexElements.ts
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,12 @@ export const isScrollable = ($el) => {
return false
}

const documentElement = $document.getDocumentFromElement(el).documentElement
ryanthemanuel marked this conversation as resolved.
Show resolved Hide resolved

if (el === documentElement) {
ryanthemanuel marked this conversation as resolved.
Show resolved Hide resolved
return checkDocumentElement($window.getWindowByElement(el), el)
}

// if we're any other element, we do some css calculations
// to see that the overflow is correct and the scroll
// area is larger than the actual height or width
Expand Down
5 changes: 5 additions & 0 deletions packages/driver/src/dom/elements/nativeProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ const nativeGetters = {
body: descriptor('Document', 'body').get,
frameElement: Object.getOwnPropertyDescriptor(window, 'frameElement')!.get,
maxLength: _getMaxLength,
style: descriptor('HTMLElement', 'style').get,
}

const nativeSetters = {
Expand All @@ -224,12 +225,16 @@ const nativeMethods = {
execCommand: window.document.execCommand,
getAttribute: window.Element.prototype.getAttribute,
setAttribute: window.Element.prototype.setAttribute,
removeAttribute: window.Element.prototype.removeAttribute,
setSelectionRange: _nativeSetSelectionRange,
modify: window.Selection.prototype.modify,
focus: _nativeFocus,
hasFocus: window.document.hasFocus,
blur: _nativeBlur,
select: _nativeSelect,
getStyleProperty: window.CSSStyleDeclaration.prototype.getPropertyValue,
setStyleProperty: window.CSSStyleDeclaration.prototype.setProperty,
removeStyleProperty: window.CSSStyleDeclaration.prototype.removeProperty,
}

export const getNativeProp = function<T, K extends keyof T> (obj: T, prop: K): T[K] {
Expand Down
3 changes: 0 additions & 3 deletions packages/server/lib/browsers/chrome.ts
Original file line number Diff line number Diff line change
Expand Up @@ -438,9 +438,6 @@ export = {
args.push(`--remote-debugging-port=${port}`)
args.push('--remote-debugging-address=127.0.0.1')

// control memory caching per execution context so that font flooding does not occur: https://github.com/cypress-io/cypress/issues/28215
args.push('--enable-features=ScopeMemoryCachePerContext')

return args
},

Expand Down
9 changes: 2 additions & 7 deletions packages/server/lib/cypress.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,24 +164,19 @@ module.exports = {
options.headed = false
}

const electronApp = require('./util/electron-app')

if (options.runProject && !options.headed) {
debug('scaling electron app in headless mode')
// scale the electron browser window
// to force retina screens to not
// upsample their images when offscreen
// rendering
electronApp.scale()
require('./util/electron-app').scale()
}

// control memory caching per execution context so that font flooding does not occur: https://github.com/cypress-io/cypress/issues/28215
electronApp.setScopeMemoryCachePerContext()

// make sure we have the appData folder
return Promise.all([
require('./util/app_data').ensure(),
electronApp.setRemoteDebuggingPort(),
require('./util/electron-app').setRemoteDebuggingPort(),
])
.then(() => {
// else determine the mode by
Expand Down
13 changes: 0 additions & 13 deletions packages/server/lib/util/electron-app.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,6 @@ const setRemoteDebuggingPort = async () => {
}
}

const setScopeMemoryCachePerContext = () => {
try {
const { app } = require('electron')

app.commandLine.appendSwitch('enable-features', 'ScopeMemoryCachePerContext')
} catch (err) {
// Catch errors for when we're running outside of electron in development
return
}
}

const isRunning = () => {
// are we in the electron or the node process?
return Boolean(process.env.ELECTRON_RUN_AS_NODE || process.versions && process.versions.electron)
Expand All @@ -71,8 +60,6 @@ const isRunningAsElectronProcess = ({ debug } = {}) => {
module.exports = {
scale,

setScopeMemoryCachePerContext,

getRemoteDebuggingPort,

setRemoteDebuggingPort,
Expand Down
44 changes: 44 additions & 0 deletions system-tests/__snapshots__/protocol_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -6678,6 +6678,50 @@ exports['component events - experimentalSingleTabRunMode: true'] = `
"pageLoading": [],
"resetTest": [],
"responseEndedWithEmptyBody": [
{
ryanthemanuel marked this conversation as resolved.
Show resolved Hide resolved
"requestId": "Any.Number",
"isCached": true,
"timings": {
"cdpRequestWillBeSentTimestamp": "Any.Number",
"cdpRequestWillBeSentReceivedTimestamp": "Any.Number",
"proxyRequestReceivedTimestamp": "Any.Number",
"cdpLagDuration": "Any.Number",
"proxyRequestCorrelationDuration": "Any.Number"
}
},
{
"requestId": "Any.Number",
"isCached": true,
"timings": {
"cdpRequestWillBeSentTimestamp": "Any.Number",
"cdpRequestWillBeSentReceivedTimestamp": "Any.Number",
"proxyRequestReceivedTimestamp": "Any.Number",
"cdpLagDuration": "Any.Number",
"proxyRequestCorrelationDuration": "Any.Number"
}
},
{
"requestId": "Any.Number",
"isCached": true,
"timings": {
"cdpRequestWillBeSentTimestamp": "Any.Number",
"cdpRequestWillBeSentReceivedTimestamp": "Any.Number",
"proxyRequestReceivedTimestamp": "Any.Number",
"cdpLagDuration": "Any.Number",
"proxyRequestCorrelationDuration": "Any.Number"
}
},
{
"requestId": "Any.Number",
"isCached": true,
"timings": {
"cdpRequestWillBeSentTimestamp": "Any.Number",
"cdpRequestWillBeSentReceivedTimestamp": "Any.Number",
"proxyRequestReceivedTimestamp": "Any.Number",
"cdpLagDuration": "Any.Number",
"proxyRequestCorrelationDuration": "Any.Number"
}
},
{
"requestId": "Any.Number",
"isCached": true,
Expand Down
2 changes: 1 addition & 1 deletion system-tests/test/protocol_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ describe('capture-protocol', () => {
return systemTests.exec(this, {
key: 'f858a2bc-b469-4e48-be67-0876339ee7e1',
project: 'protocol',
spec: 'protocol.cy.js,test-isolation.cy.js',
record: true,
expectedExitCode: 0,
port: 2121,
spec: 'protocol.cy.js,test-isolation.cy.js',
config: {
hosts: {
'*foobar.com': '127.0.0.1',
Expand Down