From 837ee08a53f205325825874bbed8b30e3eb02dd5 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 2 Jun 2021 20:17:24 -0700 Subject: [PATCH] fix(waitForSelector): retry when context is gone during node adoption (#6851) There is a small window after finishing the "rerunnable task" where we adopt the node to the main world and navigation could destroy the context. --- src/server/chromium/crExecutionContext.ts | 2 +- src/server/chromium/crPage.ts | 2 +- src/server/dom.ts | 2 ++ src/server/firefox/ffExecutionContext.ts | 2 +- src/server/firefox/ffPage.ts | 2 +- src/server/frames.ts | 34 ++++++++++++--------- src/server/javascript.ts | 24 +++++++++++++++ src/server/webkit/wkConnection.ts | 4 --- src/server/webkit/wkExecutionContext.ts | 4 +-- src/server/webkit/wkPage.ts | 6 ++-- tests/page/page-wait-for-selector-2.spec.ts | 20 ++++++++++++ 11 files changed, 75 insertions(+), 27 deletions(-) diff --git a/src/server/chromium/crExecutionContext.ts b/src/server/chromium/crExecutionContext.ts index 02b65e2fb2..8bb8c87260 100644 --- a/src/server/chromium/crExecutionContext.ts +++ b/src/server/chromium/crExecutionContext.ts @@ -109,7 +109,7 @@ function rewriteError(error: Error): Protocol.Runtime.evaluateReturnValue { if (error.message.includes('Object couldn\'t be returned by value')) return {result: {type: 'undefined'}}; - if (error.message.endsWith('Cannot find context with specified id') || error.message.endsWith('Inspected target navigated or closed') || error.message.endsWith('Execution context was destroyed.')) + if (js.isContextDestroyedError(error) || error.message.endsWith('Inspected target navigated or closed')) throw new Error('Execution context was destroyed, most likely because of a navigation.'); if (error instanceof TypeError && error.message.startsWith('Converting circular structure to JSON')) rewriteErrorMessage(error, error.message + ' Are you passing a nested JSHandle?'); diff --git a/src/server/chromium/crPage.ts b/src/server/chromium/crPage.ts index f4672059ce..9f357b00c5 100644 --- a/src/server/chromium/crPage.ts +++ b/src/server/chromium/crPage.ts @@ -1130,7 +1130,7 @@ class FrameSession { executionContextId: (to._delegate as CRExecutionContext)._contextId, }); if (!result || result.object.subtype === 'null') - throw new Error('Unable to adopt element handle from a different document'); + throw new Error(dom.kUnableToAdoptErrorMessage); return to.createHandle(result.object).asElement()!; } } diff --git a/src/server/dom.ts b/src/server/dom.ts index 17566f0d4a..e243acd330 100644 --- a/src/server/dom.ts +++ b/src/server/dom.ts @@ -1023,3 +1023,5 @@ export function elementStateTask(selector: SelectorInfo, state: ElementStateWith }); }, { parsed: selector.parsed, state }); } + +export const kUnableToAdoptErrorMessage = 'Unable to adopt element handle from a different document'; diff --git a/src/server/firefox/ffExecutionContext.ts b/src/server/firefox/ffExecutionContext.ts index d23fe1c4a2..c7aa23d131 100644 --- a/src/server/firefox/ffExecutionContext.ts +++ b/src/server/firefox/ffExecutionContext.ts @@ -111,7 +111,7 @@ function checkException(exceptionDetails?: Protocol.Runtime.ExceptionDetails) { function rewriteError(error: Error): (Protocol.Runtime.evaluateReturnValue | Protocol.Runtime.callFunctionReturnValue) { if (error.message.includes('cyclic object value') || error.message.includes('Object is not serializable')) return {result: {type: 'undefined', value: undefined}}; - if (error.message.includes('Failed to find execution context with id') || error.message.includes('Execution context was destroyed!')) + if (js.isContextDestroyedError(error)) throw new Error('Execution context was destroyed, most likely because of a navigation.'); if (error instanceof TypeError && error.message.startsWith('Converting circular structure to JSON')) rewriteErrorMessage(error, error.message + ' Are you passing a nested JSHandle?'); diff --git a/src/server/firefox/ffPage.ts b/src/server/firefox/ffPage.ts index 65ca35e69e..b4f5bef60a 100644 --- a/src/server/firefox/ffPage.ts +++ b/src/server/firefox/ffPage.ts @@ -528,7 +528,7 @@ export class FFPage implements PageDelegate { executionContextId: (to._delegate as FFExecutionContext)._executionContextId }); if (!result.remoteObject) - throw new Error('Unable to adopt element handle from a different document'); + throw new Error(dom.kUnableToAdoptErrorMessage); return to.createHandle(result.remoteObject) as dom.ElementHandle; } diff --git a/src/server/frames.ts b/src/server/frames.ts index 6df495b3b3..cdf60ec890 100644 --- a/src/server/frames.ts +++ b/src/server/frames.ts @@ -677,13 +677,26 @@ export class Frame extends SdkObject { const task = dom.waitForSelectorTask(info, state); return controller.run(async progress => { progress.log(`waiting for selector "${selector}"${state === 'attached' ? '' : ' to be ' + state}`); - const result = await this._scheduleRerunnableHandleTask(progress, info.world, task); - if (!result.asElement()) { - result.dispose(); - return null; + while (progress.isRunning()) { + const result = await this._scheduleRerunnableHandleTask(progress, info.world, task); + if (!result.asElement()) { + result.dispose(); + return null; + } + if ((options as any).__testHookBeforeAdoptNode) + await (options as any).__testHookBeforeAdoptNode(); + try { + const handle = result.asElement() as dom.ElementHandle; + const adopted = await handle._adoptTo(await this._mainContext()); + return adopted; + } catch (e) { + // Navigated while trying to adopt the node. + if (!js.isContextDestroyedError(e) && !e.message.includes(dom.kUnableToAdoptErrorMessage)) + throw e; + result.dispose(); + } } - const handle = result.asElement() as dom.ElementHandle; - return handle._adoptTo(await this._mainContext()); + return null; }, this._page._timeoutSettings.timeout(options)); } @@ -1263,16 +1276,9 @@ class RerunnableTask { this._contextData.rerunnableTasks.delete(this); this._resolve(result); } catch (e) { - // When the page is navigated, the promise is rejected. // We will try again in the new execution context. - if (e.message.includes('Execution context was destroyed')) + if (js.isContextDestroyedError(e)) return; - - // We could have tried to evaluate in a context which was already - // destroyed. - if (e.message.includes('Cannot find context with specified id')) - return; - this._contextData.rerunnableTasks.delete(this); this._reject(e); } diff --git a/src/server/javascript.ts b/src/server/javascript.ts index ce3aef5cb5..82fc630fa7 100644 --- a/src/server/javascript.ts +++ b/src/server/javascript.ts @@ -277,3 +277,27 @@ export function normalizeEvaluationExpression(expression: string, isFunction: bo expression = '(' + expression + ')'; return expression; } + +export const kSwappedOutErrorMessage = 'Target was swapped out.'; + +export function isContextDestroyedError(e: any) { + if (!e || typeof e !== 'object' || typeof e.message !== 'string') + return false; + + // Evaluating in a context which was already destroyed. + if (e.message.includes('Cannot find context with specified id') + || e.message.includes('Failed to find execution context with id') + || e.message.includes('Missing injected script for given') + || e.message.includes('Cannot find object with id')) + return true; + + // Evaluation promise is rejected when context is gone. + if (e.message.includes('Execution context was destroyed')) + return true; + + // WebKit target swap. + if (e.message.includes(kSwappedOutErrorMessage)) + return true; + + return false; +} diff --git a/src/server/webkit/wkConnection.ts b/src/server/webkit/wkConnection.ts index 894af9060f..f3987d0246 100644 --- a/src/server/webkit/wkConnection.ts +++ b/src/server/webkit/wkConnection.ts @@ -185,7 +185,3 @@ export function createProtocolError(error: Error, method: string, protocolError: message += ` ${JSON.stringify(protocolError.data)}`; return rewriteErrorMessage(error, message); } - -export function isSwappedOutError(e: Error) { - return e.message.includes('Target was swapped out.'); -} diff --git a/src/server/webkit/wkExecutionContext.ts b/src/server/webkit/wkExecutionContext.ts index f0c99869ba..4f0656ccc2 100644 --- a/src/server/webkit/wkExecutionContext.ts +++ b/src/server/webkit/wkExecutionContext.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { WKSession, isSwappedOutError } from './wkConnection'; +import { WKSession } from './wkConnection'; import { Protocol } from './protocol'; import * as js from '../javascript'; import { parseEvaluationResultValue } from '../common/utilityScriptSerializers'; @@ -143,7 +143,7 @@ function potentiallyUnserializableValue(remoteObject: Protocol.Runtime.RemoteObj } function rewriteError(error: Error): Error { - if (isSwappedOutError(error) || error.message.includes('Missing injected script for given')) + if (js.isContextDestroyedError(error)) return new Error('Execution context was destroyed, most likely because of a navigation.'); return error; } diff --git a/src/server/webkit/wkPage.ts b/src/server/webkit/wkPage.ts index b3d7d74737..5bb11a27c1 100644 --- a/src/server/webkit/wkPage.ts +++ b/src/server/webkit/wkPage.ts @@ -26,7 +26,7 @@ import * as dialog from '../dialog'; import * as dom from '../dom'; import * as frames from '../frames'; import { helper, RegisteredListener } from '../helper'; -import { JSHandle } from '../javascript'; +import { JSHandle, kSwappedOutErrorMessage } from '../javascript'; import * as network from '../network'; import { Page, PageBinding, PageDelegate } from '../page'; import { Progress } from '../progress'; @@ -213,7 +213,7 @@ export class WKPage implements PageDelegate { assert(this._provisionalPage); assert(this._provisionalPage._session.sessionId === newTargetId, 'Unknown new target: ' + newTargetId); assert(this._session.sessionId === oldTargetId, 'Unknown old target: ' + oldTargetId); - this._session.errorText = 'Target was swapped out.'; + this._session.errorText = kSwappedOutErrorMessage; const newSession = this._provisionalPage._session; this._provisionalPage.commit(); this._provisionalPage.dispose(); @@ -896,7 +896,7 @@ export class WKPage implements PageDelegate { executionContextId: (to._delegate as WKExecutionContext)._contextId }); if (!result || result.object.subtype === 'null') - throw new Error('Unable to adopt element handle from a different document'); + throw new Error(dom.kUnableToAdoptErrorMessage); return to.createHandle(result.object) as dom.ElementHandle; } diff --git a/tests/page/page-wait-for-selector-2.spec.ts b/tests/page/page-wait-for-selector-2.spec.ts index b5e049c810..bf48b3f1a3 100644 --- a/tests/page/page-wait-for-selector-2.spec.ts +++ b/tests/page/page-wait-for-selector-2.spec.ts @@ -268,3 +268,23 @@ it('should correctly handle hidden shadow host', async ({page, server}) => { expect(await page.textContent('div')).toBe('Find me'); await page.waitForSelector('div', { state: 'hidden' }); }); + +it('should work when navigating before node adoption', async ({page, mode, server}) => { + it.skip(mode !== 'default'); + + await page.goto(server.EMPTY_PAGE); + await page.setContent(`
Hello
`); + + let navigatedOnce = false; + const __testHookBeforeAdoptNode = async () => { + if (!navigatedOnce) { + navigatedOnce = true; + await page.goto(server.PREFIX + '/one-style.html'); + } + }; + + const div = await page.waitForSelector('div', { __testHookBeforeAdoptNode } as any); + + // This text is coming from /one-style.html + expect(await div.textContent()).toBe('hello, world!'); +});