Filip/playwright
1
0
Fork 0
Code Issues Pull requests Actions Packages Projects Releases Wiki Activity

fix(recorder): disallow external imports (#29129)

Browse source 
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`.
This commit is contained in:
Dmitry Gozman 2024-01-23 11:29:40 -08:00 committed by GitHub
parent 5bc5056a1f
commit 8e607d509f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 43 additions and 35 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
packages/playwright-core/src/server/injected/consoleApi.ts
View file

@ -120,13 +120,13 @@ class ConsoleAPI {
private _selector(element: Element) { private _selector(element: Element) {
if (!(element instanceof Element)) if (!(element instanceof Element))
throw new Error(`Usage: playwright.selector(element).`); throw new Error(`Usage: playwright.selector(element).`);
return this._injectedScript.generateSelector(element); return this._injectedScript.generateSelectorSimple(element);
} }
private _generateLocator(element: Element, language?: Language) { private _generateLocator(element: Element, language?: Language) {
if (!(element instanceof Element)) if (!(element instanceof Element))
throw new Error(`Usage: playwright.locator(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); return asLocator(language || 'javascript', selector);
} }

15
packages/playwright-core/src/server/injected/injectedScript.ts
View file

@ -24,7 +24,7 @@ import type { NestedSelectorBody, ParsedSelector, ParsedSelectorPart } from '../
import { visitAllSelectorParts, parseSelector, stringifySelector } from '../../utils/isomorphic/selectorParser'; import { visitAllSelectorParts, parseSelector, stringifySelector } from '../../utils/isomorphic/selectorParser';
import { type TextMatcher, elementMatchesText, elementText, type ElementText, getElementLabels } from './selectorUtils'; import { type TextMatcher, elementMatchesText, elementText, type ElementText, getElementLabels } from './selectorUtils';
import { SelectorEvaluatorImpl, sortInDOMOrder } from './selectorEvaluator'; 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 type { CSSComplexSelectorList } from '../../utils/isomorphic/cssParser';
import { generateSelector, type GenerateSelectorOptions } from './selectorGenerator'; import { generateSelector, type GenerateSelectorOptions } from './selectorGenerator';
import type * as channels from '@protocol/channels'; import type * as channels from '@protocol/channels';
@ -66,6 +66,7 @@ export class InjectedScript {
// eslint-disable-next-line no-restricted-globals // eslint-disable-next-line no-restricted-globals
readonly window: Window & typeof globalThis; readonly window: Window & typeof globalThis;
readonly document: Document; readonly document: Document;
readonly utils = { isInsideScope, elementText, asLocator, normalizeWhiteSpace };
// eslint-disable-next-line no-restricted-globals // 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 }[]) { 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; 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; return generateSelector(this, targetElement, { ...options, testIdAttributeName: this._testIdAttributeNameForStrictErrorAndConsoleCodegen }).selector;
} }
@ -996,7 +1001,7 @@ export class InjectedScript {
strictModeViolationError(selector: ParsedSelector, matches: Element[]): Error { strictModeViolationError(selector: ParsedSelector, matches: Element[]): Error {
const infos = matches.slice(0, 10).map(m => ({ const infos = matches.slice(0, 10).map(m => ({
preview: this.previewNode(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)}`); const lines = infos.map((info, i) => `\n ${i + 1}) ${info.preview} aka ${asLocator(this._sdkLanguage, info.selector)}`);
if (infos.length < matches.length) if (infos.length < matches.length)
@ -1017,6 +1022,10 @@ export class InjectedScript {
return error; return error;
} }
createHighlight() {
return new Highlight(this);
}
maskSelectors(selectors: ParsedSelector[], color: string) { maskSelectors(selectors: ParsedSelector[], color: string) {
if (this._highlight) if (this._highlight)
this.hideHighlight(); this.hideHighlight();

4
packages/playwright-core/src/server/injected/recorder/DEPS.list Normal file
View file

@ -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.
[*]

43
packages/playwright-core/src/server/injected/recorder.ts → packages/playwright-core/src/server/injected/recorder/recorder.ts
View file

@ -14,17 +14,12 @@
* limitations under the License. * limitations under the License.
*/ */
import type * as actions from '../recorder/recorderActions'; import type * as actions from '../../recorder/recorderActions';
import type { InjectedScript } from '../injected/injectedScript'; import type { InjectedScript } from '../injectedScript';
import { generateSelector } from '../injected/selectorGenerator'; import type { Point } from '../../../common/types';
import type { Point } from '../../common/types';
import type { Mode, OverlayState, UIState } from '@recorder/recorderTypes'; import type { Mode, OverlayState, UIState } from '@recorder/recorderTypes';
import { Highlight, type HighlightOptions } from '../injected/highlight'; import type { ElementText } from '../selectorUtils';
import { isInsideScope } from './domUtils'; import type { Highlight, HighlightOptions } from '../highlight';
import { elementText } from './selectorUtils';
import type { ElementText } from './selectorUtils';
import { asLocator } from '../../utils/isomorphic/locatorGenerators';
import { normalizeWhiteSpace } from '@isomorphic/stringUtils';
interface RecorderDelegate { interface RecorderDelegate {
performAction?(action: actions.Action): Promise<void>; performAction?(action: actions.Action): Promise<void>;
@ -121,7 +116,7 @@ class InspectTool implements RecorderTool {
if (this._hoveredElement === target) if (this._hoveredElement === target)
return; return;
this._hoveredElement = target; 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) if (this._hoveredModel?.selector === model?.selector)
return; return;
this._hoveredModel = model; this._hoveredModel = model;
@ -361,7 +356,7 @@ class RecordActionTool implements RecorderTool {
// We'd like to ignore this stray event. // We'd like to ignore this stray event.
if (userGesture && activeElement === this._recorder.document.body) if (userGesture && activeElement === this._recorder.document.body)
return; 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; this._activeModel = result && result.selector ? result : null;
if (userGesture) if (userGesture)
this._hoveredElement = activeElement as HTMLElement | null; this._hoveredElement = activeElement as HTMLElement | null;
@ -458,7 +453,7 @@ class RecordActionTool implements RecorderTool {
this._recorder.updateHighlight(null, true); this._recorder.updateHighlight(null, true);
return; 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) if (this._hoveredModel && this._hoveredModel.selector === selector)
return; return;
this._hoveredModel = selector ? { selector, elements } : null; this._hoveredModel = selector ? { selector, elements } : null;
@ -531,9 +526,9 @@ class TextAssertionTool implements RecorderTool {
if (this._hoverHighlight?.elements[0] === target) if (this._hoverHighlight?.elements[0] === target)
return; return;
if (this._kind === 'text') 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 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' }); this._recorder.updateHighlight(this._hoverHighlight, true, { color: '#8acae480' });
} }
@ -559,7 +554,7 @@ class TextAssertionTool implements RecorderTool {
if (this._kind === 'value') { if (this._kind === 'value') {
if (!this._elementHasValue(target)) if (!this._elementHasValue(target))
return null; 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())) { if (target.nodeName === 'INPUT' && ['checkbox', 'radio'].includes((target as HTMLInputElement).type.toLowerCase())) {
return { return {
name: 'assertChecked', name: 'assertChecked',
@ -577,7 +572,7 @@ class TextAssertionTool implements RecorderTool {
}; };
} }
} else { } 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. // forTextExpect can update the target, re-highlight it.
this._recorder.updateHighlight(this._hoverHighlight, true, { color: '#8acae480' }); this._recorder.updateHighlight(this._hoverHighlight, true, { color: '#8acae480' });
@ -585,7 +580,7 @@ class TextAssertionTool implements RecorderTool {
name: 'assertText', name: 'assertText',
selector: this._hoverHighlight.selector, selector: this._hoverHighlight.selector,
signals: [], signals: [],
text: elementText(this._textCache, target).normalized, text: this._recorder.injectedScript.utils.elementText(this._textCache, target).normalized,
substring: true, substring: true,
}; };
} }
@ -593,7 +588,7 @@ class TextAssertionTool implements RecorderTool {
private _renderValue(action: actions.Action) { private _renderValue(action: actions.Action) {
if (action?.name === 'assertText') if (action?.name === 'assertText')
return normalizeWhiteSpace(action.text); return this._recorder.injectedScript.utils.normalizeWhiteSpace(action.text);
if (action?.name === 'assertChecked') if (action?.name === 'assertChecked')
return String(action.checked); return String(action.checked);
if (action?.name === 'assertValue') if (action?.name === 'assertValue')
@ -648,12 +643,12 @@ class TextAssertionTool implements RecorderTool {
textElement.classList.add('text-editor'); textElement.classList.add('text-editor');
const updateAndValidate = () => { const updateAndValidate = () => {
const newValue = normalizeWhiteSpace(textElement.value); const newValue = this._recorder.injectedScript.utils.normalizeWhiteSpace(textElement.value);
const target = this._hoverHighlight?.elements[0]; const target = this._hoverHighlight?.elements[0];
if (!target) if (!target)
return; return;
action.text = newValue; 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); const matches = newValue && targetText.includes(newValue);
textElement.classList.toggle('does-not-match', !matches); textElement.classList.toggle('does-not-match', !matches);
}; };
@ -771,7 +766,7 @@ class Overlay {
} }
contains(element: Element) { contains(element: Element) {
return isInsideScope(this._overlayElement, element); return this._recorder.injectedScript.utils.isInsideScope(this._overlayElement, element);
} }
setUIState(state: UIState) { setUIState(state: UIState) {
@ -866,7 +861,7 @@ export class Recorder {
constructor(injectedScript: InjectedScript) { constructor(injectedScript: InjectedScript) {
this.document = injectedScript.document; this.document = injectedScript.document;
this.injectedScript = injectedScript; this.injectedScript = injectedScript;
this.highlight = new Highlight(injectedScript); this.highlight = injectedScript.createHighlight();
this._tools = { this._tools = {
'none': new NoneTool(), 'none': new NoneTool(),
'standby': new NoneTool(), 'standby': new NoneTool(),
@ -1077,7 +1072,7 @@ export class Recorder {
updateHighlight(model: HighlightModel | null, userGesture: boolean, options: HighlightOptions = {}) { updateHighlight(model: HighlightModel | null, userGesture: boolean, options: HighlightOptions = {}) {
if (options.tooltipText === undefined && model?.selector) 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); this.highlight.updateHighlight(model?.elements || [], options);
if (userGesture) if (userGesture)
this.delegate.highlightUpdated?.(); this.delegate.highlightUpdated?.();

2
packages/playwright-core/src/server/recorder.ts
View file

@ -739,7 +739,7 @@ async function findFrameSelector(frame: Frame): Promise<string | undefined> {
const utility = await parent._utilityContext(); const utility = await parent._utilityContext();
const injected = await utility.injectedScript(); const injected = await utility.injectedScript();
const selector = await injected.evaluate((injected, element) => { 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); }, frameElement);
return selector; return selector;
} catch (e) { } catch (e) {

4
packages/trace-viewer/src/ui/snapshotTab.tsx
View file

@ -22,7 +22,7 @@ import { Toolbar } from '@web/components/toolbar';
import { ToolbarButton } from '@web/components/toolbarButton'; import { ToolbarButton } from '@web/components/toolbarButton';
import { useMeasure } from '@web/uiUtils'; import { useMeasure } from '@web/uiUtils';
import { InjectedScript } from '@injected/injectedScript'; import { InjectedScript } from '@injected/injectedScript';
import { Recorder } from '@injected/recorder'; import { Recorder } from '@injected/recorder/recorder';
import ConsoleAPI from '@injected/consoleApi'; import ConsoleAPI from '@injected/consoleApi';
import { asLocator } from '@isomorphic/locatorGenerators'; import { asLocator } from '@isomorphic/locatorGenerators';
import type { Language } 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) { for (let i = 0; i < frameWindow.frames.length; ++i) {
const childFrame = frameWindow.frames[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); createRecorders(recorders, sdkLanguage, testIdAttributeName, isUnderTest, parentFrameSelector + frameSelector, childFrame);
} }
} }

4
tests/library/selector-generator.spec.ts
View file

@ -519,8 +519,8 @@ it.describe('selector generator', () => {
const selectors = await page.evaluate(() => { const selectors = await page.evaluate(() => {
const target = document.querySelector('section > span'); const target = document.querySelector('section > span');
const root = document.querySelector('section'); const root = document.querySelector('section');
const relative = (window as any).__injectedScript.generateSelector(target, { root }); const relative = (window as any).__injectedScript.generateSelectorSimple(target, { root });
const absolute = (window as any).__injectedScript.generateSelector(target); const absolute = (window as any).__injectedScript.generateSelectorSimple(target);
return { relative, absolute }; return { relative, absolute };
}); });
expect(selectors).toEqual({ expect(selectors).toEqual({

2
utils/generate_injected.js
View file

@ -45,7 +45,7 @@ const injectedScripts = [
true, 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', 'lib', 'server', 'injected', 'packed'),
path.join(ROOT, 'packages', 'playwright-core', 'src', 'generated'), path.join(ROOT, 'packages', 'playwright-core', 'src', 'generated'),
true, true,