From b54303a3869b9f3e34a0a96bd33589eb71a12998 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 26 Jun 2020 16:32:42 -0700 Subject: [PATCH] fix(textContent): make page.textContent(selector) atomic (#2717) We now query selector and take textContent synchronously. This avoids any issues with async processing: node being recycled, detached, etc. More methods will follow with the same atomic pattern. Drive-by: fixed selector engine names being sometimes case-sensitive and sometimes not. --- src/common/selectorParser.ts | 1 - src/dom.ts | 54 ++++++++++++++++++++++++- src/frames.ts | 78 +++++++++++++++++++++--------------- src/selectors.ts | 48 ---------------------- test/dispatchevent.spec.js | 21 ++++++++++ test/elementhandle.spec.js | 22 ++++++++++ test/queryselector.spec.js | 28 ++++++------- test/utils.js | 9 +++++ 8 files changed, 163 insertions(+), 98 deletions(-) diff --git a/src/common/selectorParser.ts b/src/common/selectorParser.ts index 917626d836..89e0ad3f68 100644 --- a/src/common/selectorParser.ts +++ b/src/common/selectorParser.ts @@ -52,7 +52,6 @@ export function parseSelector(selector: string): ParsedSelector { name = 'css'; body = part; } - name = name.toLowerCase(); let capture = false; if (name[0] === '*') { capture = true; diff --git a/src/dom.ts b/src/dom.ts index 0f800108c2..69f1cc8412 100644 --- a/src/dom.ts +++ b/src/dom.ts @@ -25,7 +25,7 @@ import * as injectedScriptSource from './generated/injectedScriptSource'; import * as debugScriptSource from './generated/debugScriptSource'; import * as js from './javascript'; import { Page } from './page'; -import { selectors } from './selectors'; +import { selectors, SelectorInfo } from './selectors'; import * as types from './types'; import { Progress } from './progress'; import DebugScript from './debug/injected/debugScript'; @@ -790,3 +790,55 @@ function roundPoint(point: types.Point): types.Point { y: (point.y * 100 | 0) / 100, }; } + +export type SchedulableTask = (injectedScript: js.JSHandle) => Promise>>; + +export function waitForSelectorTask(selector: SelectorInfo, state: 'attached' | 'detached' | 'visible' | 'hidden'): SchedulableTask { + return injectedScript => injectedScript.evaluateHandle((injected, { parsed, state }) => { + let lastElement: Element | undefined; + + return injected.pollRaf((progress, continuePolling) => { + const element = injected.querySelector(parsed, document); + const visible = element ? injected.isVisible(element) : false; + + if (lastElement !== element) { + lastElement = element; + if (!element) + progress.log(` selector did not resolve to any element`); + else + progress.log(` selector resolved to ${visible ? 'visible' : 'hidden'} ${injected.previewNode(element)}`); + } + + switch (state) { + case 'attached': + return element ? element : continuePolling; + case 'detached': + return !element ? undefined : continuePolling; + case 'visible': + return visible ? element : continuePolling; + case 'hidden': + return !visible ? undefined : continuePolling; + } + }); + }, { parsed: selector.parsed, state }); +} + +export function dispatchEventTask(selector: SelectorInfo, type: string, eventInit: Object): SchedulableTask { + return injectedScript => injectedScript.evaluateHandle((injected, { parsed, type, eventInit }) => { + return injected.pollRaf((progress, continuePolling) => { + const element = injected.querySelector(parsed, document); + if (element) + injected.dispatchEvent(element, type, eventInit); + return element ? undefined : continuePolling; + }); + }, { parsed: selector.parsed, type, eventInit }); +} + +export function textContentTask(selector: SelectorInfo): SchedulableTask { + return injectedScript => injectedScript.evaluateHandle((injected, parsed) => { + return injected.pollRaf((progress, continuePolling) => { + const element = injected.querySelector(parsed, document); + return element ? element.textContent : continuePolling; + }); + }, selector.parsed); +} diff --git a/src/frames.ts b/src/frames.ts index 87120d8b65..65d3958fad 100644 --- a/src/frames.ts +++ b/src/frames.ts @@ -33,7 +33,7 @@ type ContextData = { contextPromise: Promise; contextResolveCallback: (c: dom.FrameExecutionContext) => void; context: dom.FrameExecutionContext | null; - rerunnableTasks: Set>; + rerunnableTasks: Set; }; export type GotoResult = { @@ -456,10 +456,10 @@ export class Frame { if (!['attached', 'detached', 'visible', 'hidden'].includes(state)) throw new Error(`Unsupported state option "${state}"`); const info = selectors._parseSelector(selector); - const task = selectors._waitForSelectorTask(info, state); + const task = dom.waitForSelectorTask(info, state); return this._page._runAbortableTask(async progress => { progress.logger.info(`waiting for selector "${selector}"${state === 'attached' ? '' : ' to be ' + state}`); - const result = await this._scheduleRerunnableTask(progress, info.world, task); + const result = await this._scheduleRerunnableHandleTask(progress, info.world, task); if (!result.asElement()) { result.dispose(); return null; @@ -477,11 +477,11 @@ export class Frame { async dispatchEvent(selector: string, type: string, eventInit?: Object, options: types.TimeoutOptions = {}): Promise { const info = selectors._parseSelector(selector); - const task = selectors._dispatchEventTask(info, type, eventInit || {}); + const task = dom.dispatchEventTask(info, type, eventInit || {}); return this._page._runAbortableTask(async progress => { progress.logger.info(`Dispatching "${type}" event on selector "${selector}"...`); - const result = await this._scheduleRerunnableTask(progress, 'main', task); - result.dispose(); + // Note: we always dispatch events in the main world. + await this._scheduleRerunnableTask(progress, 'main', task); }, this._page._timeoutSettings.timeout(options), this._apiName('dispatchEvent')); } @@ -723,8 +723,8 @@ export class Frame { return this._page._runAbortableTask(async progress => { while (progress.isRunning()) { progress.logger.info(`waiting for selector "${selector}"`); - const task = selectors._waitForSelectorTask(info, 'attached'); - const handle = await this._scheduleRerunnableTask(progress, info.world, task); + const task = dom.waitForSelectorTask(info, 'attached'); + const handle = await this._scheduleRerunnableHandleTask(progress, info.world, task); const element = handle.asElement() as dom.ElementHandle; progress.cleanupWhenAborted(() => element.dispose()); const result = await action(progress, element); @@ -755,8 +755,13 @@ export class Frame { await this._retryWithSelectorIfNotConnected(selector, options, (progress, handle) => handle._focus(progress), this._apiName('focus')); } - async textContent(selector: string, options: types.TimeoutOptions = {}): Promise { - return await this._retryWithSelectorIfNotConnected(selector, options, (progress, handle) => handle.textContent(), this._apiName('textContent')); + async textContent(selector: string, options: types.TimeoutOptions = {}): Promise { + const info = selectors._parseSelector(selector); + const task = dom.textContentTask(info); + return this._page._runAbortableTask(async progress => { + progress.logger.info(`Retrieving text context from "${selector}"...`); + return this._scheduleRerunnableTask(progress, info.world, task); + }, this._page._timeoutSettings.timeout(options), this._apiName('textContent')); } async innerText(selector: string, options: types.TimeoutOptions = {}): Promise { @@ -809,7 +814,6 @@ export class Frame { return this._waitForFunctionExpression(String(pageFunction), typeof pageFunction === 'function', arg, options); } - async _waitForFunctionExpression(expression: string, isFunction: boolean, arg: any, options: types.WaitForFunctionOptions = {}): Promise> { const { polling = 'raf' } = options; if (helper.isString(polling)) @@ -819,17 +823,14 @@ export class Frame { else throw new Error('Unknown polling option: ' + polling); const predicateBody = isFunction ? 'return (' + expression + ')(arg)' : 'return (' + expression + ')'; - const task = async (context: dom.FrameExecutionContext) => { - const injectedScript = await context.injectedScript(); - return context.evaluateHandleInternal(({ injectedScript, predicateBody, polling, arg }) => { - const innerPredicate = new Function('arg', predicateBody) as (arg: any) => R; - if (polling === 'raf') - return injectedScript.pollRaf((progress, continuePolling) => innerPredicate(arg) || continuePolling); - return injectedScript.pollInterval(polling, (progress, continuePolling) => innerPredicate(arg) || continuePolling); - }, { injectedScript, predicateBody, polling, arg }); - }; + const task: dom.SchedulableTask = injectedScript => injectedScript.evaluateHandle((injectedScript, { predicateBody, polling, arg }) => { + const innerPredicate = new Function('arg', predicateBody) as (arg: any) => R; + if (polling === 'raf') + return injectedScript.pollRaf((progress, continuePolling) => innerPredicate(arg) || continuePolling); + return injectedScript.pollInterval(polling, (progress, continuePolling) => innerPredicate(arg) || continuePolling); + }, { predicateBody, polling, arg }); return this._page._runAbortableTask( - progress => this._scheduleRerunnableTask(progress, 'main', task), + progress => this._scheduleRerunnableHandleTask(progress, 'main', task), this._page._timeoutSettings.timeout(options), this._apiName('waitForFunction')); } @@ -850,9 +851,19 @@ export class Frame { this._parentFrame = null; } - private _scheduleRerunnableTask(progress: Progress, world: types.World, task: SchedulableTask): Promise> { + private _scheduleRerunnableTask(progress: Progress, world: types.World, task: dom.SchedulableTask): Promise { const data = this._contextData.get(world)!; - const rerunnableTask = new RerunnableTask(data, progress, task); + const rerunnableTask = new RerunnableTask(data, progress, task, true /* returnByValue */); + if (this._detached) + rerunnableTask.terminate(new Error('waitForFunction failed: frame got detached.')); + if (data.context) + rerunnableTask.rerun(data.context); + return rerunnableTask.promise; + } + + private _scheduleRerunnableHandleTask(progress: Progress, world: types.World, task: dom.SchedulableTask): Promise> { + const data = this._contextData.get(world)!; + const rerunnableTask = new RerunnableTask(data, progress, task, false /* returnByValue */); if (this._detached) rerunnableTask.terminate(new Error('waitForFunction failed: frame got detached.')); if (data.context) @@ -905,20 +916,20 @@ export class Frame { } } -export type SchedulableTask = (context: dom.FrameExecutionContext) => Promise>>; - -class RerunnableTask { - readonly promise: Promise>; - private _task: SchedulableTask; - private _resolve: (result: js.SmartHandle) => void = () => {}; +class RerunnableTask { + readonly promise: Promise; + private _task: dom.SchedulableTask; + private _resolve: (result: any) => void = () => {}; private _reject: (reason: Error) => void = () => {}; private _progress: Progress; + private _returnByValue: boolean; - constructor(data: ContextData, progress: Progress, task: SchedulableTask) { + constructor(data: ContextData, progress: Progress, task: dom.SchedulableTask, returnByValue: boolean) { this._task = task; this._progress = progress; + this._returnByValue = returnByValue; data.rerunnableTasks.add(this); - this.promise = new Promise>((resolve, reject) => { + this.promise = new Promise((resolve, reject) => { // The task is either resolved with a value, or rejected with a meaningful evaluation error. this._resolve = resolve; this._reject = reject; @@ -931,8 +942,9 @@ class RerunnableTask { async rerun(context: dom.FrameExecutionContext) { try { - const pollHandler = new dom.InjectedScriptPollHandler(this._progress, await this._task(context)); - const result = await pollHandler.finishHandle(); + const injectedScript = await context.injectedScript(); + const pollHandler = new dom.InjectedScriptPollHandler(this._progress, await this._task(injectedScript)); + const result = this._returnByValue ? await pollHandler.finish() : await pollHandler.finishHandle(); this._resolve(result); } catch (e) { // When the page is navigated, the promise is rejected. diff --git a/src/selectors.ts b/src/selectors.ts index 3b0e12ec65..ba237d900f 100644 --- a/src/selectors.ts +++ b/src/selectors.ts @@ -109,54 +109,6 @@ export class Selectors { return result; } - _waitForSelectorTask(selector: SelectorInfo, state: 'attached' | 'detached' | 'visible' | 'hidden'): frames.SchedulableTask { - return async (context: dom.FrameExecutionContext) => { - const injectedScript = await context.injectedScript(); - return injectedScript.evaluateHandle((injected, { parsed, state }) => { - let lastElement: Element | undefined; - - return injected.pollRaf((progress, continuePolling) => { - const element = injected.querySelector(parsed, document); - const visible = element ? injected.isVisible(element) : false; - - if (lastElement !== element) { - lastElement = element; - if (!element) - progress.log(` selector did not resolve to any element`); - else - progress.log(` selector resolved to ${visible ? 'visible' : 'hidden'} ${injected.previewNode(element)}`); - } - - switch (state) { - case 'attached': - return element ? element : continuePolling; - case 'detached': - return !element ? undefined : continuePolling; - case 'visible': - return visible ? element : continuePolling; - case 'hidden': - return !visible ? undefined : continuePolling; - } - }); - }, { parsed: selector.parsed, state }); - }; - } - - _dispatchEventTask(selector: SelectorInfo, type: string, eventInit: Object): frames.SchedulableTask { - const task = async (context: dom.FrameExecutionContext) => { - const injectedScript = await context.injectedScript(); - return injectedScript.evaluateHandle((injected, { parsed, type, eventInit }) => { - return injected.pollRaf((progress, continuePolling) => { - const element = injected.querySelector(parsed, document); - if (element) - injected.dispatchEvent(element, type, eventInit); - return element ? undefined : continuePolling; - }); - }, { parsed: selector.parsed, type, eventInit }); - }; - return task; - } - async _createSelector(name: string, handle: dom.ElementHandle): Promise { const mainContext = await handle._page.mainFrame()._mainContext(); const injectedScript = await mainContext.injectedScript(); diff --git a/test/dispatchevent.spec.js b/test/dispatchevent.spec.js index ccd5663576..1b2b679e20 100644 --- a/test/dispatchevent.spec.js +++ b/test/dispatchevent.spec.js @@ -97,6 +97,27 @@ describe('Page.dispatchEvent(click)', function() { await watchdog; expect(await page.evaluate(() => window.clicked)).toBe(true); }); + it('should be atomic', async({page}) => { + const createDummySelector = () => ({ + create(root, target) {}, + query(root, selector) { + const result = root.querySelector(selector); + if (result) + Promise.resolve().then(() => result.onclick = ""); + return result; + }, + queryAll(root, selector) { + const result = Array.from(root.querySelectorAll(selector)); + for (const e of result) + Promise.resolve().then(() => result.onclick = ""); + return result; + } + }); + await utils.registerEngine('dispatchEvent', createDummySelector); + await page.setContent(`
Hello
`); + await page.dispatchEvent('dispatchEvent=div', 'click'); + expect(await page.evaluate(() => window._clicked)).toBe(true); + }); }); describe('Page.dispatchEvent(drag)', function() { diff --git a/test/elementhandle.spec.js b/test/elementhandle.spec.js index 5dbba0bb9c..6c17de70bb 100644 --- a/test/elementhandle.spec.js +++ b/test/elementhandle.spec.js @@ -476,6 +476,28 @@ describe('ElementHandle convenience API', function() { expect(await handle.textContent()).toBe('Text,\nmore text'); expect(await page.textContent('#inner')).toBe('Text,\nmore text'); }); + it('textContent should be atomic', async({page}) => { + const createDummySelector = () => ({ + create(root, target) {}, + query(root, selector) { + const result = root.querySelector(selector); + if (result) + Promise.resolve().then(() => result.textContent = 'modified'); + return result; + }, + queryAll(root, selector) { + const result = Array.from(root.querySelectorAll(selector)); + for (const e of result) + Promise.resolve().then(() => result.textContent = 'modified'); + return result; + } + }); + await utils.registerEngine('textContent', createDummySelector); + await page.setContent(`
Hello
`); + const tc = await page.textContent('textContent=div'); + expect(tc).toBe('Hello'); + expect(await page.evaluate(() => document.querySelector('div').textContent)).toBe('modified'); + }); }); describe('ElementHandle.check', () => { diff --git a/test/queryselector.spec.js b/test/queryselector.spec.js index 3fd646de25..6ec24f4b61 100644 --- a/test/queryselector.spec.js +++ b/test/queryselector.spec.js @@ -16,16 +16,8 @@ */ const path = require('path'); -const {FFOX, CHROMIUM, WEBKIT} = require('./utils').testOptions(browserType); - -async function registerEngine(name, script, options) { - try { - await playwright.selectors.register(name, script, options); - } catch (e) { - if (!e.message.includes('has been already registered')) - throw e; - } -} +const utils = require('./utils'); +const {FFOX, CHROMIUM, WEBKIT} = utils.testOptions(browserType); describe('Page.$eval', function() { it('should work with css selector', async({page, server}) => { @@ -764,15 +756,19 @@ describe('selectors.register', () => { return Array.from(root.querySelectorAll(selector)); } }); - await registerEngine('tag', `(${createTagSelector.toString()})()`); + await utils.registerEngine('tag', `(${createTagSelector.toString()})()`); await page.setContent('
'); expect(await playwright.selectors._createSelector('tag', await page.$('div'))).toBe('DIV'); expect(await page.$eval('tag=DIV', e => e.nodeName)).toBe('DIV'); expect(await page.$eval('tag=SPAN', e => e.nodeName)).toBe('SPAN'); expect(await page.$$eval('tag=DIV', es => es.length)).toBe(2); + + // Selector names are case-sensitive. + const error = await page.$('tAG=DIV').catch(e => e); + expect(error.message).toBe('Unknown engine "tAG" while parsing selector tAG=DIV'); }); it('should work with path', async ({page}) => { - await registerEngine('foo', { path: path.join(__dirname, 'assets/sectionselectorengine.js') }); + await utils.registerEngine('foo', { path: path.join(__dirname, 'assets/sectionselectorengine.js') }); await page.setContent('
'); expect(await page.$eval('foo=whatever', e => e.nodeName)).toBe('SECTION'); }); @@ -786,8 +782,8 @@ describe('selectors.register', () => { return [document.body, document.documentElement, window.__answer]; } }); - await registerEngine('main', createDummySelector); - await registerEngine('isolated', createDummySelector, { contentScript: true }); + await utils.registerEngine('main', createDummySelector); + await utils.registerEngine('isolated', createDummySelector, { contentScript: true }); await page.setContent('
'); await page.evaluate(() => window.__answer = document.querySelector('span')); // Works in main if asked. @@ -826,7 +822,9 @@ describe('selectors.register', () => { error = await playwright.selectors.register('$', createDummySelector).catch(e => e); expect(error.message).toBe('Selector engine name may only contain [a-zA-Z0-9_] characters'); - await registerEngine('dummy', createDummySelector); + // Selector names are case-sensitive. + await utils.registerEngine('dummy', createDummySelector); + await utils.registerEngine('duMMy', createDummySelector); error = await playwright.selectors.register('dummy', createDummySelector).catch(e => e); expect(error.message).toBe('"dummy" selector engine has been already registered'); diff --git a/test/utils.js b/test/utils.js index 3e3600b370..db34709b5b 100644 --- a/test/utils.js +++ b/test/utils.js @@ -94,6 +94,15 @@ const utils = module.exports = { expect(await page.evaluate('window.innerHeight')).toBe(height); }, + registerEngine: async (name, script, options) => { + try { + await playwright.selectors.register(name, script, options); + } catch (e) { + if (!e.message.includes('has been already registered')) + throw e; + } + }, + initializeFlakinessDashboardIfNeeded: async function(testRunner) { // Generate testIDs for all tests and verify they don't clash. // This will add |test.testId| for every test.