From 01198f8eef63ebbf7d993fb12604c42906d3c634 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 14 Sep 2020 10:38:14 -0700 Subject: [PATCH] fix($$): use utility context when possible (#3870) This avoids the typical issue of overridden bulitins, trading it for performance of one by one node adoptions. --- src/server/dom.ts | 2 +- src/server/firefox/ffPage.ts | 2 +- src/server/frames.ts | 2 +- src/server/selectors.ts | 28 ++++++++++++++++++---------- src/server/webkit/wkPage.ts | 2 +- test/queryselector.spec.ts | 12 ++++++++++++ 6 files changed, 34 insertions(+), 14 deletions(-) diff --git a/src/server/dom.ts b/src/server/dom.ts index 1ff31b5e60..2beab23dad 100644 --- a/src/server/dom.ts +++ b/src/server/dom.ts @@ -552,7 +552,7 @@ export class ElementHandle extends js.JSHandle { } async $$(selector: string): Promise[]> { - return this._page.selectors._queryAll(this._context.frame, selector, this); + return this._page.selectors._queryAll(this._context.frame, selector, this, true /* adoptToMain */); } async _$evalExpression(selector: string, expression: string, isFunction: boolean, arg: any): Promise { diff --git a/src/server/firefox/ffPage.ts b/src/server/firefox/ffPage.ts index 665a2be191..8aa2c3bef9 100644 --- a/src/server/firefox/ffPage.ts +++ b/src/server/firefox/ffPage.ts @@ -488,7 +488,7 @@ export class FFPage implements PageDelegate { const parent = frame.parentFrame(); if (!parent) throw new Error('Frame has been detached.'); - const handles = await this._page.selectors._queryAll(parent, 'iframe', undefined, true /* allowUtilityContext */); + const handles = await this._page.selectors._queryAll(parent, 'iframe', undefined); const items = await Promise.all(handles.map(async handle => { const frame = await handle.contentFrame().catch(e => null); return { handle, frame }; diff --git a/src/server/frames.ts b/src/server/frames.ts index 8d27b62c28..cf286dcb96 100644 --- a/src/server/frames.ts +++ b/src/server/frames.ts @@ -590,7 +590,7 @@ export class Frame extends EventEmitter { } async $$(selector: string): Promise[]> { - return this._page.selectors._queryAll(this, selector); + return this._page.selectors._queryAll(this, selector, undefined, true /* adoptToMain */); } async content(): Promise { diff --git a/src/server/selectors.ts b/src/server/selectors.ts index b7cfa05c37..88faba82b2 100644 --- a/src/server/selectors.ts +++ b/src/server/selectors.ts @@ -68,11 +68,7 @@ export class Selectors { return null; } const mainContext = await frame._mainContext(); - if (elementHandle._context === mainContext) - return elementHandle; - const adopted = frame._page._delegate.adoptElementHandle(elementHandle, mainContext); - elementHandle.dispose(); - return adopted; + return this._adoptIfNeeded(elementHandle, mainContext); } async _queryArray(frame: frames.Frame, selector: string, scope?: dom.ElementHandle): Promise> { @@ -85,9 +81,9 @@ export class Selectors { return arrayHandle; } - async _queryAll(frame: frames.Frame, selector: string, scope?: dom.ElementHandle, allowUtilityContext?: boolean): Promise[]> { + async _queryAll(frame: frames.Frame, selector: string, scope?: dom.ElementHandle, adoptToMain?: boolean): Promise[]> { const info = this._parseSelector(selector); - const context = await frame._context(allowUtilityContext ? info.world : 'main'); + const context = await frame._context(info.world); const injectedScript = await context.injectedScript(); const arrayHandle = await injectedScript.evaluateHandle((injected, { parsed, scope }) => { return injected.querySelectorAll(parsed, scope || document); @@ -95,15 +91,27 @@ export class Selectors { const properties = await arrayHandle.getProperties(); arrayHandle.dispose(); - const result: dom.ElementHandle[] = []; + + // Note: adopting elements one by one may be slow. If we encounter the issue here, + // we might introduce 'useMainContext' option or similar to speed things up. + const targetContext = adoptToMain ? await frame._mainContext() : context; + const result: Promise>[] = []; for (const property of properties.values()) { const elementHandle = property.asElement() as dom.ElementHandle; if (elementHandle) - result.push(elementHandle); + result.push(this._adoptIfNeeded(elementHandle, targetContext)); else property.dispose(); } - return result; + return Promise.all(result); + } + + private async _adoptIfNeeded(handle: dom.ElementHandle, context: dom.FrameExecutionContext): Promise> { + if (handle._context === context) + return handle; + const adopted = handle._page._delegate.adoptElementHandle(handle, context); + handle.dispose(); + return adopted; } async _createSelector(name: string, handle: dom.ElementHandle): Promise { diff --git a/src/server/webkit/wkPage.ts b/src/server/webkit/wkPage.ts index 0be4526994..c72fab2241 100644 --- a/src/server/webkit/wkPage.ts +++ b/src/server/webkit/wkPage.ts @@ -854,7 +854,7 @@ export class WKPage implements PageDelegate { const parent = frame.parentFrame(); if (!parent) throw new Error('Frame has been detached.'); - const handles = await this._page.selectors._queryAll(parent, 'iframe', undefined, true /* allowUtilityContext */); + const handles = await this._page.selectors._queryAll(parent, 'iframe', undefined); const items = await Promise.all(handles.map(async handle => { const frame = await handle.contentFrame().catch(e => null); return { handle, frame }; diff --git a/test/queryselector.spec.ts b/test/queryselector.spec.ts index 53d47c83a9..f34f351ab4 100644 --- a/test/queryselector.spec.ts +++ b/test/queryselector.spec.ts @@ -114,3 +114,15 @@ it('xpath should return multiple elements', async ({page, server}) => { const elements = await page.$$('xpath=/html/body/div'); expect(elements.length).toBe(2); }); + +it('$$ should work with bogus Array.from', async ({page, server}) => { + await page.setContent('
hello
'); + const div1 = await page.evaluateHandle(() => { + Array.from = () => []; + return document.querySelector('div'); + }); + const elements = await page.$$('div'); + expect(elements.length).toBe(2); + // Check that element handle is functional and belongs to the main world. + expect(await elements[0].evaluate((div, div1) => div === div1, div1)).toBe(true); +});