From 8e607d509f793a0ac30743c1e676fca6d93c2b9e Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 23 Jan 2024 11:29:40 -0800 Subject: [PATCH] fix(recorder): disallow external imports (#29129) Previously, new `Recorder` instance was given an existing `InjectedScript`. However, we built a separate source for `InjectedScript` vs `Recorder`, and both bundles contain their own copy of all helper modules, e.g. `roleUtils`. This resulted in two copies of helper modules, which is troublesome for any module-level globals like a top-level cache. Depending on whether `Recorder` or `InjectedScript` called into the helper, they would access the different value of a module global, which lead to bugs. To prevent this, we force any external dependencies to be imported through the `InjectedScript.utils`. --- .../src/server/injected/consoleApi.ts | 4 +- .../src/server/injected/injectedScript.ts | 15 +++++-- .../src/server/injected/recorder/DEPS.list | 4 ++ .../injected/{ => recorder}/recorder.ts | 43 ++++++++----------- .../playwright-core/src/server/recorder.ts | 2 +- packages/trace-viewer/src/ui/snapshotTab.tsx | 4 +- tests/library/selector-generator.spec.ts | 4 +- utils/generate_injected.js | 2 +- 8 files changed, 43 insertions(+), 35 deletions(-) create mode 100644 packages/playwright-core/src/server/injected/recorder/DEPS.list rename packages/playwright-core/src/server/injected/{ => recorder}/recorder.ts (95%) diff --git a/packages/playwright-core/src/server/injected/consoleApi.ts b/packages/playwright-core/src/server/injected/consoleApi.ts index cce789021f..57decad644 100644 --- a/packages/playwright-core/src/server/injected/consoleApi.ts +++ b/packages/playwright-core/src/server/injected/consoleApi.ts @@ -120,13 +120,13 @@ class ConsoleAPI { private _selector(element: Element) { if (!(element instanceof Element)) throw new Error(`Usage: playwright.selector(element).`); - return this._injectedScript.generateSelector(element); + return this._injectedScript.generateSelectorSimple(element); } private _generateLocator(element: Element, language?: Language) { if (!(element instanceof Element)) throw new Error(`Usage: playwright.locator(element).`); - const selector = this._injectedScript.generateSelector(element); + const selector = this._injectedScript.generateSelectorSimple(element); return asLocator(language || 'javascript', selector); } diff --git a/packages/playwright-core/src/server/injected/injectedScript.ts b/packages/playwright-core/src/server/injected/injectedScript.ts index 2efb23a081..1b8160194d 100644 --- a/packages/playwright-core/src/server/injected/injectedScript.ts +++ b/packages/playwright-core/src/server/injected/injectedScript.ts @@ -24,7 +24,7 @@ import type { NestedSelectorBody, ParsedSelector, ParsedSelectorPart } from '../ import { visitAllSelectorParts, parseSelector, stringifySelector } from '../../utils/isomorphic/selectorParser'; import { type TextMatcher, elementMatchesText, elementText, type ElementText, getElementLabels } from './selectorUtils'; import { SelectorEvaluatorImpl, sortInDOMOrder } from './selectorEvaluator'; -import { enclosingShadowRootOrDocument, isElementVisible, parentElementOrShadowHost, setBrowserName } from './domUtils'; +import { enclosingShadowRootOrDocument, isElementVisible, isInsideScope, parentElementOrShadowHost, setBrowserName } from './domUtils'; import type { CSSComplexSelectorList } from '../../utils/isomorphic/cssParser'; import { generateSelector, type GenerateSelectorOptions } from './selectorGenerator'; import type * as channels from '@protocol/channels'; @@ -66,6 +66,7 @@ export class InjectedScript { // eslint-disable-next-line no-restricted-globals readonly window: Window & typeof globalThis; readonly document: Document; + readonly utils = { isInsideScope, elementText, asLocator, normalizeWhiteSpace }; // eslint-disable-next-line no-restricted-globals constructor(window: Window & typeof globalThis, isUnderTest: boolean, sdkLanguage: Language, testIdAttributeNameForStrictErrorAndConsoleCodegen: string, stableRafCount: number, browserName: string, customEngines: { name: string, engine: SelectorEngine }[]) { @@ -140,7 +141,11 @@ export class InjectedScript { return result; } - generateSelector(targetElement: Element, options?: GenerateSelectorOptions): string { + generateSelector(targetElement: Element, options: GenerateSelectorOptions) { + return generateSelector(this, targetElement, options); + } + + generateSelectorSimple(targetElement: Element, options?: GenerateSelectorOptions): string { return generateSelector(this, targetElement, { ...options, testIdAttributeName: this._testIdAttributeNameForStrictErrorAndConsoleCodegen }).selector; } @@ -996,7 +1001,7 @@ export class InjectedScript { strictModeViolationError(selector: ParsedSelector, matches: Element[]): Error { const infos = matches.slice(0, 10).map(m => ({ preview: this.previewNode(m), - selector: this.generateSelector(m), + selector: this.generateSelectorSimple(m), })); const lines = infos.map((info, i) => `\n ${i + 1}) ${info.preview} aka ${asLocator(this._sdkLanguage, info.selector)}`); if (infos.length < matches.length) @@ -1017,6 +1022,10 @@ export class InjectedScript { return error; } + createHighlight() { + return new Highlight(this); + } + maskSelectors(selectors: ParsedSelector[], color: string) { if (this._highlight) this.hideHighlight(); diff --git a/packages/playwright-core/src/server/injected/recorder/DEPS.list b/packages/playwright-core/src/server/injected/recorder/DEPS.list new file mode 100644 index 0000000000..ee39467fea --- /dev/null +++ b/packages/playwright-core/src/server/injected/recorder/DEPS.list @@ -0,0 +1,4 @@ +# Recorder must use any external dependencies through InjectedScript. +# Otherwise it will end up with a copy of all modules it uses, and any +# module-level globals will be duplicated, which leads to subtle bugs. +[*] diff --git a/packages/playwright-core/src/server/injected/recorder.ts b/packages/playwright-core/src/server/injected/recorder/recorder.ts similarity index 95% rename from packages/playwright-core/src/server/injected/recorder.ts rename to packages/playwright-core/src/server/injected/recorder/recorder.ts index b4cf40b5d5..a5155fe8b5 100644 --- a/packages/playwright-core/src/server/injected/recorder.ts +++ b/packages/playwright-core/src/server/injected/recorder/recorder.ts @@ -14,17 +14,12 @@ * limitations under the License. */ -import type * as actions from '../recorder/recorderActions'; -import type { InjectedScript } from '../injected/injectedScript'; -import { generateSelector } from '../injected/selectorGenerator'; -import type { Point } from '../../common/types'; +import type * as actions from '../../recorder/recorderActions'; +import type { InjectedScript } from '../injectedScript'; +import type { Point } from '../../../common/types'; import type { Mode, OverlayState, UIState } from '@recorder/recorderTypes'; -import { Highlight, type HighlightOptions } from '../injected/highlight'; -import { isInsideScope } from './domUtils'; -import { elementText } from './selectorUtils'; -import type { ElementText } from './selectorUtils'; -import { asLocator } from '../../utils/isomorphic/locatorGenerators'; -import { normalizeWhiteSpace } from '@isomorphic/stringUtils'; +import type { ElementText } from '../selectorUtils'; +import type { Highlight, HighlightOptions } from '../highlight'; interface RecorderDelegate { performAction?(action: actions.Action): Promise; @@ -121,7 +116,7 @@ class InspectTool implements RecorderTool { if (this._hoveredElement === target) return; this._hoveredElement = target; - const model = this._hoveredElement ? generateSelector(this._recorder.injectedScript, this._hoveredElement, { testIdAttributeName: this._recorder.state.testIdAttributeName }) : null; + const model = this._hoveredElement ? this._recorder.injectedScript.generateSelector(this._hoveredElement, { testIdAttributeName: this._recorder.state.testIdAttributeName }) : null; if (this._hoveredModel?.selector === model?.selector) return; this._hoveredModel = model; @@ -361,7 +356,7 @@ class RecordActionTool implements RecorderTool { // We'd like to ignore this stray event. if (userGesture && activeElement === this._recorder.document.body) return; - const result = activeElement ? generateSelector(this._recorder.injectedScript, activeElement, { testIdAttributeName: this._recorder.state.testIdAttributeName }) : null; + const result = activeElement ? this._recorder.injectedScript.generateSelector(activeElement, { testIdAttributeName: this._recorder.state.testIdAttributeName }) : null; this._activeModel = result && result.selector ? result : null; if (userGesture) this._hoveredElement = activeElement as HTMLElement | null; @@ -458,7 +453,7 @@ class RecordActionTool implements RecorderTool { this._recorder.updateHighlight(null, true); return; } - const { selector, elements } = generateSelector(this._recorder.injectedScript, this._hoveredElement, { testIdAttributeName: this._recorder.state.testIdAttributeName }); + const { selector, elements } = this._recorder.injectedScript.generateSelector(this._hoveredElement, { testIdAttributeName: this._recorder.state.testIdAttributeName }); if (this._hoveredModel && this._hoveredModel.selector === selector) return; this._hoveredModel = selector ? { selector, elements } : null; @@ -531,9 +526,9 @@ class TextAssertionTool implements RecorderTool { if (this._hoverHighlight?.elements[0] === target) return; if (this._kind === 'text') - this._hoverHighlight = elementText(this._textCache, target).full ? { elements: [target], selector: '' } : null; + this._hoverHighlight = this._recorder.injectedScript.utils.elementText(this._textCache, target).full ? { elements: [target], selector: '' } : null; else - this._hoverHighlight = this._elementHasValue(target) ? generateSelector(this._recorder.injectedScript, target, { testIdAttributeName: this._recorder.state.testIdAttributeName }) : null; + this._hoverHighlight = this._elementHasValue(target) ? this._recorder.injectedScript.generateSelector(target, { testIdAttributeName: this._recorder.state.testIdAttributeName }) : null; this._recorder.updateHighlight(this._hoverHighlight, true, { color: '#8acae480' }); } @@ -559,7 +554,7 @@ class TextAssertionTool implements RecorderTool { if (this._kind === 'value') { if (!this._elementHasValue(target)) return null; - const { selector } = generateSelector(this._recorder.injectedScript, target, { testIdAttributeName: this._recorder.state.testIdAttributeName }); + const { selector } = this._recorder.injectedScript.generateSelector(target, { testIdAttributeName: this._recorder.state.testIdAttributeName }); if (target.nodeName === 'INPUT' && ['checkbox', 'radio'].includes((target as HTMLInputElement).type.toLowerCase())) { return { name: 'assertChecked', @@ -577,7 +572,7 @@ class TextAssertionTool implements RecorderTool { }; } } else { - this._hoverHighlight = generateSelector(this._recorder.injectedScript, target, { testIdAttributeName: this._recorder.state.testIdAttributeName, forTextExpect: true }); + this._hoverHighlight = this._recorder.injectedScript.generateSelector(target, { testIdAttributeName: this._recorder.state.testIdAttributeName, forTextExpect: true }); // forTextExpect can update the target, re-highlight it. this._recorder.updateHighlight(this._hoverHighlight, true, { color: '#8acae480' }); @@ -585,7 +580,7 @@ class TextAssertionTool implements RecorderTool { name: 'assertText', selector: this._hoverHighlight.selector, signals: [], - text: elementText(this._textCache, target).normalized, + text: this._recorder.injectedScript.utils.elementText(this._textCache, target).normalized, substring: true, }; } @@ -593,7 +588,7 @@ class TextAssertionTool implements RecorderTool { private _renderValue(action: actions.Action) { if (action?.name === 'assertText') - return normalizeWhiteSpace(action.text); + return this._recorder.injectedScript.utils.normalizeWhiteSpace(action.text); if (action?.name === 'assertChecked') return String(action.checked); if (action?.name === 'assertValue') @@ -648,12 +643,12 @@ class TextAssertionTool implements RecorderTool { textElement.classList.add('text-editor'); const updateAndValidate = () => { - const newValue = normalizeWhiteSpace(textElement.value); + const newValue = this._recorder.injectedScript.utils.normalizeWhiteSpace(textElement.value); const target = this._hoverHighlight?.elements[0]; if (!target) return; action.text = newValue; - const targetText = elementText(this._textCache, target).normalized; + const targetText = this._recorder.injectedScript.utils.elementText(this._textCache, target).normalized; const matches = newValue && targetText.includes(newValue); textElement.classList.toggle('does-not-match', !matches); }; @@ -771,7 +766,7 @@ class Overlay { } contains(element: Element) { - return isInsideScope(this._overlayElement, element); + return this._recorder.injectedScript.utils.isInsideScope(this._overlayElement, element); } setUIState(state: UIState) { @@ -866,7 +861,7 @@ export class Recorder { constructor(injectedScript: InjectedScript) { this.document = injectedScript.document; this.injectedScript = injectedScript; - this.highlight = new Highlight(injectedScript); + this.highlight = injectedScript.createHighlight(); this._tools = { 'none': new NoneTool(), 'standby': new NoneTool(), @@ -1077,7 +1072,7 @@ export class Recorder { updateHighlight(model: HighlightModel | null, userGesture: boolean, options: HighlightOptions = {}) { if (options.tooltipText === undefined && model?.selector) - options.tooltipText = asLocator(this.state.language, model.selector); + options.tooltipText = this.injectedScript.utils.asLocator(this.state.language, model.selector); this.highlight.updateHighlight(model?.elements || [], options); if (userGesture) this.delegate.highlightUpdated?.(); diff --git a/packages/playwright-core/src/server/recorder.ts b/packages/playwright-core/src/server/recorder.ts index bedcb7a691..d288726fcd 100644 --- a/packages/playwright-core/src/server/recorder.ts +++ b/packages/playwright-core/src/server/recorder.ts @@ -739,7 +739,7 @@ async function findFrameSelector(frame: Frame): Promise { const utility = await parent._utilityContext(); const injected = await utility.injectedScript(); const selector = await injected.evaluate((injected, element) => { - return injected.generateSelector(element as Element, { testIdAttributeName: '', omitInternalEngines: true }); + return injected.generateSelectorSimple(element as Element, { testIdAttributeName: '', omitInternalEngines: true }); }, frameElement); return selector; } catch (e) { diff --git a/packages/trace-viewer/src/ui/snapshotTab.tsx b/packages/trace-viewer/src/ui/snapshotTab.tsx index 9ae19d9adf..bdb955c13c 100644 --- a/packages/trace-viewer/src/ui/snapshotTab.tsx +++ b/packages/trace-viewer/src/ui/snapshotTab.tsx @@ -22,7 +22,7 @@ import { Toolbar } from '@web/components/toolbar'; import { ToolbarButton } from '@web/components/toolbarButton'; import { useMeasure } from '@web/uiUtils'; import { InjectedScript } from '@injected/injectedScript'; -import { Recorder } from '@injected/recorder'; +import { Recorder } from '@injected/recorder/recorder'; import ConsoleAPI from '@injected/consoleApi'; import { asLocator } from '@isomorphic/locatorGenerators'; import type { Language } from '@isomorphic/locatorGenerators'; @@ -279,7 +279,7 @@ function createRecorders(recorders: { recorder: Recorder, frameSelector: string for (let i = 0; i < frameWindow.frames.length; ++i) { const childFrame = frameWindow.frames[i]; - const frameSelector = childFrame.frameElement ? win._injectedScript.generateSelector(childFrame.frameElement, { omitInternalEngines: true, testIdAttributeName }) + ' >> internal:control=enter-frame >> ' : ''; + const frameSelector = childFrame.frameElement ? win._injectedScript.generateSelectorSimple(childFrame.frameElement, { omitInternalEngines: true, testIdAttributeName }) + ' >> internal:control=enter-frame >> ' : ''; createRecorders(recorders, sdkLanguage, testIdAttributeName, isUnderTest, parentFrameSelector + frameSelector, childFrame); } } diff --git a/tests/library/selector-generator.spec.ts b/tests/library/selector-generator.spec.ts index 6b87b6171c..f92d0393f5 100644 --- a/tests/library/selector-generator.spec.ts +++ b/tests/library/selector-generator.spec.ts @@ -519,8 +519,8 @@ it.describe('selector generator', () => { const selectors = await page.evaluate(() => { const target = document.querySelector('section > span'); const root = document.querySelector('section'); - const relative = (window as any).__injectedScript.generateSelector(target, { root }); - const absolute = (window as any).__injectedScript.generateSelector(target); + const relative = (window as any).__injectedScript.generateSelectorSimple(target, { root }); + const absolute = (window as any).__injectedScript.generateSelectorSimple(target); return { relative, absolute }; }); expect(selectors).toEqual({ diff --git a/utils/generate_injected.js b/utils/generate_injected.js index c07d9a4877..eccd7ddcad 100644 --- a/utils/generate_injected.js +++ b/utils/generate_injected.js @@ -45,7 +45,7 @@ const injectedScripts = [ true, ], [ - path.join(ROOT, 'packages', 'playwright-core', 'src', 'server', 'injected', 'recorder.ts'), + path.join(ROOT, 'packages', 'playwright-core', 'src', 'server', 'injected', 'recorder', 'recorder.ts'), path.join(ROOT, 'packages', 'playwright-core', 'lib', 'server', 'injected', 'packed'), path.join(ROOT, 'packages', 'playwright-core', 'src', 'generated'), true,