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

@@ -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;

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<T> = (injectedScript: js.JSHandle<InjectedScript>) => Promise<js.JSHandle<types.InjectedScriptPoll<T>>>;
+
+export function waitForSelectorTask(selector: SelectorInfo, state: 'attached' | 'detached' | 'visible' | 'hidden'): SchedulableTask<Element | undefined> {
+  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<undefined> {
+  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<string | null> {
+  return injectedScript => injectedScript.evaluateHandle((injected, parsed) => {
+    return injected.pollRaf((progress, continuePolling) => {
+      const element = injected.querySelector(parsed, document);
+      return element ? element.textContent : continuePolling;
+    });
+  }, selector.parsed);
+}

src/frames.ts

@@ -33,7 +33,7 @@ type ContextData = {
   contextPromise: Promise<dom.FrameExecutionContext>;
   contextResolveCallback: (c: dom.FrameExecutionContext) => void;
   context: dom.FrameExecutionContext | null;
-  rerunnableTasks: Set<RerunnableTask<any>>;
+  rerunnableTasks: Set<RerunnableTask>;
 };
 
 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<void> {
     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);
+      // Note: we always dispatch events in the main world.
-      result.dispose();
+      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 task = dom.waitForSelectorTask(info, 'attached');
-        const handle = await this._scheduleRerunnableTask(progress, info.world, task);
+        const handle = await this._scheduleRerunnableHandleTask(progress, info.world, task);
         const element = handle.asElement() as dom.ElementHandle<Element>;
         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<null|string> {
+  async textContent(selector: string, options: types.TimeoutOptions = {}): Promise<string | null> {
-    return await this._retryWithSelectorIfNotConnected(selector, options, (progress, handle) => handle.textContent(), this._apiName('textContent'));
+    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<string> {
@@ -809,7 +814,6 @@ export class Frame {
     return this._waitForFunctionExpression(String(pageFunction), typeof pageFunction === 'function', arg, options);
   }
 
-
   async _waitForFunctionExpression<R>(expression: string, isFunction: boolean, arg: any, options: types.WaitForFunctionOptions = {}): Promise<js.SmartHandle<R>> {
     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 task: dom.SchedulableTask<R> = injectedScript => injectedScript.evaluateHandle((injectedScript, { predicateBody, polling, arg }) => {
-      const injectedScript = await context.injectedScript();
+      const innerPredicate = new Function('arg', predicateBody) as (arg: any) => R;
-      return context.evaluateHandleInternal(({ injectedScript, predicateBody, polling, arg }) => {
+      if (polling === 'raf')
-        const innerPredicate = new Function('arg', predicateBody) as (arg: any) => R;
+        return injectedScript.pollRaf((progress, continuePolling) => innerPredicate(arg) || continuePolling);
-        if (polling === 'raf')
+      return injectedScript.pollInterval(polling, (progress, continuePolling) => innerPredicate(arg) || continuePolling);
-          return injectedScript.pollRaf((progress, continuePolling) => innerPredicate(arg) || continuePolling);
+    }, { predicateBody, polling, arg });
-        return injectedScript.pollInterval(polling, (progress, continuePolling) => innerPredicate(arg) || continuePolling);
-      }, { injectedScript, 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<T>(progress: Progress, world: types.World, task: SchedulableTask<T>): Promise<js.SmartHandle<T>> {
+  private _scheduleRerunnableTask<T>(progress: Progress, world: types.World, task: dom.SchedulableTask<T>): Promise<T> {
     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<T>(progress: Progress, world: types.World, task: dom.SchedulableTask<T>): Promise<js.SmartHandle<T>> {
+    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<T> = (context: dom.FrameExecutionContext) => Promise<js.JSHandle<types.InjectedScriptPoll<T>>>;
+class RerunnableTask {
-
+  readonly promise: Promise<any>;
-class RerunnableTask<T> {
+  private _task: dom.SchedulableTask<any>;
-  readonly promise: Promise<js.SmartHandle<T>>;
+  private _resolve: (result: any) => void = () => {};
-  private _task: SchedulableTask<T>;
-  private _resolve: (result: js.SmartHandle<T>) => void = () => {};
   private _reject: (reason: Error) => void = () => {};
   private _progress: Progress;
+  private _returnByValue: boolean;
 
-  constructor(data: ContextData, progress: Progress, task: SchedulableTask<T>) {
+  constructor(data: ContextData, progress: Progress, task: dom.SchedulableTask<any>, returnByValue: boolean) {
     this._task = task;
     this._progress = progress;
+    this._returnByValue = returnByValue;
     data.rerunnableTasks.add(this);
-    this.promise = new Promise<js.SmartHandle<T>>((resolve, reject) => {
+    this.promise = new Promise<any>((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<T> {
 
   async rerun(context: dom.FrameExecutionContext) {
     try {
-      const pollHandler = new dom.InjectedScriptPollHandler(this._progress, await this._task(context));
+      const injectedScript = await context.injectedScript();
-      const result = await pollHandler.finishHandle();
+      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.

src/selectors.ts

@@ -109,54 +109,6 @@ export class Selectors {
     return result;
   }
 
-  _waitForSelectorTask(selector: SelectorInfo, state: 'attached' | 'detached' | 'visible' | 'hidden'): frames.SchedulableTask<Element | undefined> {
-    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<undefined> {
-    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<Element>): Promise<string | undefined> {
     const mainContext = await handle._page.mainFrame()._mainContext();
     const injectedScript = await mainContext.injectedScript();

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(`<div onclick="window._clicked=true">Hello</div>`);
+    await page.dispatchEvent('dispatchEvent=div', 'click');
+    expect(await page.evaluate(() => window._clicked)).toBe(true);
+  });
 });
 
 describe('Page.dispatchEvent(drag)', function() {

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(`<div>Hello</div>`);
+    const tc = await page.textContent('textContent=div');
+    expect(tc).toBe('Hello');
+    expect(await page.evaluate(() => document.querySelector('div').textContent)).toBe('modified');
+  });
 });
 
 describe('ElementHandle.check', () => {

test/queryselector.spec.js

@@ -16,16 +16,8 @@
  */
 
 const path = require('path');
-const {FFOX, CHROMIUM, WEBKIT} = require('./utils').testOptions(browserType);
+const utils = require('./utils');
-
+const {FFOX, CHROMIUM, WEBKIT} = 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;
-  }
-}
 
 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('<div><span></span></div><div></div>');
     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('<section></section>');
     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 utils.registerEngine('main', createDummySelector);
-    await registerEngine('isolated', createDummySelector, { contentScript: true });
+    await utils.registerEngine('isolated', createDummySelector, { contentScript: true });
     await page.setContent('<div><span><section></section></span></div>');
     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');

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.