From 4ecbd91e4b16caf96a112cdd65372900a12d2fae Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Sun, 24 Feb 2019 23:07:24 -0800 Subject: [PATCH] refactor(firefox): migrate onto ExecutionContext events (#4064) Juggler now has Runtime domain that emits Execution Context events "ExecutionContextCreated" and "ExecutionContextDestroyed". --- .../puppeteer-firefox/lib/ExecutionContext.js | 19 ++++-- .../puppeteer-firefox/lib/FrameManager.js | 60 ++++++++++++++++--- .../puppeteer-firefox/lib/JSHandle.js | 12 ++-- experimental/puppeteer-firefox/lib/Page.js | 9 +-- experimental/puppeteer-firefox/package.json | 2 +- test/evaluation.spec.js | 2 +- 6 files changed, 81 insertions(+), 23 deletions(-) diff --git a/experimental/puppeteer-firefox/lib/ExecutionContext.js b/experimental/puppeteer-firefox/lib/ExecutionContext.js index 29cff77093d..d74c12fc2e5 100644 --- a/experimental/puppeteer-firefox/lib/ExecutionContext.js +++ b/experimental/puppeteer-firefox/lib/ExecutionContext.js @@ -15,10 +15,10 @@ class ExecutionContext { async evaluateHandle(pageFunction, ...args) { if (helper.isString(pageFunction)) { - const payload = await this._session.send('Page.evaluate', { - script: pageFunction, + const payload = await this._session.send('Runtime.evaluate', { + expression: pageFunction, executionContextId: this._executionContextId, - }); + }).catch(rewriteError); return createHandle(this, payload.result, payload.exceptionDetails); } if (typeof pageFunction !== 'function') @@ -59,10 +59,10 @@ class ExecutionContext { return {unserializableValue: 'NaN'}; return {value: arg}; }); - let payload = null; + let callFunctionPromise; try { - payload = await this._session.send('Page.evaluate', { - functionText, + callFunctionPromise = this._session.send('Runtime.callFunction', { + functionDeclaration: functionText, args, executionContextId: this._executionContextId }); @@ -71,7 +71,14 @@ class ExecutionContext { err.message += ' Are you passing a nested JSHandle?'; throw err; } + const payload = await callFunctionPromise.catch(rewriteError); return createHandle(this, payload.result, payload.exceptionDetails); + + function rewriteError(error) { + if (error.message.includes('Failed to find execution context with id')) + throw new Error('Execution context was destroyed, most likely because of a navigation.'); + throw error; + } } frame() { diff --git a/experimental/puppeteer-firefox/lib/FrameManager.js b/experimental/puppeteer-firefox/lib/FrameManager.js index c10b78fef2a..564ce8b4637 100644 --- a/experimental/puppeteer-firefox/lib/FrameManager.js +++ b/experimental/puppeteer-firefox/lib/FrameManager.js @@ -22,15 +22,41 @@ class FrameManager extends EventEmitter { this._timeoutSettings = timeoutSettings; this._mainFrame = null; this._frames = new Map(); + /** @type {!Map} */ + this._contextIdToContext = new Map(); this._eventListeners = [ helper.addEventListener(this._session, 'Page.eventFired', this._onEventFired.bind(this)), helper.addEventListener(this._session, 'Page.frameAttached', this._onFrameAttached.bind(this)), helper.addEventListener(this._session, 'Page.frameDetached', this._onFrameDetached.bind(this)), helper.addEventListener(this._session, 'Page.navigationCommitted', this._onNavigationCommitted.bind(this)), helper.addEventListener(this._session, 'Page.sameDocumentNavigation', this._onSameDocumentNavigation.bind(this)), + helper.addEventListener(this._session, 'Runtime.executionContextCreated', this._onExecutionContextCreated.bind(this)), + helper.addEventListener(this._session, 'Runtime.executionContextDestroyed', this._onExecutionContextDestroyed.bind(this)), ]; } + executionContextById(executionContextId) { + return this._contextIdToContext.get(executionContextId) || null; + } + + _onExecutionContextCreated({executionContextId, auxData}) { + const frameId = auxData ? auxData.frameId : null; + const frame = this._frames.get(frameId) || null; + const context = new ExecutionContext(this._session, frame, executionContextId); + if (frame) + frame._setContext(context); + this._contextIdToContext.set(executionContextId, context); + } + + _onExecutionContextDestroyed({executionContextId}) { + const context = this._contextIdToContext.get(executionContextId); + if (!context) + return; + this._contextIdToContext.delete(executionContextId); + if (context._frame) + context._frame._setContext(null); + } + frame(frameId) { return this._frames.get(frameId); } @@ -122,19 +148,37 @@ class Frame { this._name = ''; /** @type {!Set} */ this._children = new Set(); - this._isDetached = false; + this._detached = false; + this._firedEvents = new Set(); /** @type {!Set} */ this._waitTasks = new Set(); this._documentPromise = null; + this._contextPromise; + this._contextResolveCallback = null; + this._setContext(null); + } - this._executionContext = new ExecutionContext(this._session, this, this._frameId); + _setContext(context) { + if (context) { + this._contextResolveCallback.call(null, context); + this._contextResolveCallback = null; + for (const waitTask of this._waitTasks) + waitTask.rerun(); + } else { + this._documentPromise = null; + this._contextPromise = new Promise(fulfill => { + this._contextResolveCallback = fulfill; + }); + } } async executionContext() { - return this._executionContext; + if (this._detached) + throw new Error(`Execution Context is not available in detached frame "${this.url()}" (are you trying to evaluate?)`); + return this._contextPromise; } /** @@ -267,7 +311,7 @@ class Frame { _detach() { this._parentFrame._children.delete(this); this._parentFrame = null; - this._isDetached = true; + this._detached = true; for (const waitTask of this._waitTasks) waitTask.terminate(new Error('waitForFunction failed: frame got detached.')); } @@ -438,7 +482,8 @@ class Frame { } async evaluate(pageFunction, ...args) { - return this._executionContext.evaluate(pageFunction, ...args); + const context = await this.executionContext(); + return context.evaluate(pageFunction, ...args); } _document() { @@ -497,7 +542,8 @@ class Frame { } async evaluateHandle(pageFunction, ...args) { - return this._executionContext.evaluateHandle(pageFunction, ...args); + const context = await this.executionContext(); + return context.evaluateHandle(pageFunction, ...args); } /** @@ -636,7 +682,7 @@ class Frame { } isDetached() { - return this._isDetached; + return this._detached; } childFrames() { diff --git a/experimental/puppeteer-firefox/lib/JSHandle.js b/experimental/puppeteer-firefox/lib/JSHandle.js index c665724ff61..c6445cdaf88 100644 --- a/experimental/puppeteer-firefox/lib/JSHandle.js +++ b/experimental/puppeteer-firefox/lib/JSHandle.js @@ -59,7 +59,7 @@ class JSHandle { * @return {!Promise>} */ async getProperties() { - const response = await this._session.send('Page.getObjectProperties', { + const response = await this._session.send('Runtime.getObjectProperties', { executionContextId: this._executionContextId, objectId: this._objectId, }); @@ -85,10 +85,10 @@ class JSHandle { async jsonValue() { if (!this._objectId) return this._deserializeValue(this._protocolValue); - const simpleValue = await this._session.send('Page.evaluate', { + const simpleValue = await this._session.send('Runtime.callFunction', { executionContextId: this._executionContextId, returnByValue: true, - functionText: (e => e).toString(), + functionDeclaration: (e => e).toString(), args: [this._protocolValue], }); return this._deserializeValue(simpleValue.result); @@ -105,9 +105,13 @@ class JSHandle { if (!this._objectId) return; this._disposed = true; - await this._session.send('Page.disposeObject', { + await this._session.send('Runtime.disposeObject', { executionContextId: this._executionContextId, objectId: this._objectId, + }).catch(error => { + // Exceptions might happen in case of a page been navigated or closed. + // Swallow these since they are harmless and we don't leak anything in this case. + debugError(error); }); } } diff --git a/experimental/puppeteer-firefox/lib/Page.js b/experimental/puppeteer-firefox/lib/Page.js index 91c355355d4..55b4a6f4ce6 100644 --- a/experimental/puppeteer-firefox/lib/Page.js +++ b/experimental/puppeteer-firefox/lib/Page.js @@ -27,6 +27,7 @@ class Page extends EventEmitter { await Promise.all([ session.send('Page.enable'), session.send('Network.enable'), + session.send('Runtime.enable'), ]); if (defaultViewport) @@ -139,7 +140,7 @@ class Page extends EventEmitter { else expression = helper.evaluationString(deliverErrorValue, name, seq, error); } - this._session.send('Page.evaluate', { script: expression, executionContextId: event.frameId }).catch(debugError); + this._session.send('Runtime.evaluate', { expression, executionContextId: event.executionContextId }).catch(debugError); /** * @param {string} name @@ -651,9 +652,9 @@ class Page extends EventEmitter { _onClosed() { } - _onConsole({type, args, frameId, location}) { - const frame = this._frameManager.frame(frameId); - this.emit(Events.Page.Console, new ConsoleMessage(type, args.map(arg => createHandle(frame._executionContext, arg)), location)); + _onConsole({type, args, executionContextId, location}) { + const context = this._frameManager.executionContextById(executionContextId); + this.emit(Events.Page.Console, new ConsoleMessage(type, args.map(arg => createHandle(context, arg)), location)); } /** diff --git a/experimental/puppeteer-firefox/package.json b/experimental/puppeteer-firefox/package.json index 7acdf661ec2..cd6f591e2e8 100644 --- a/experimental/puppeteer-firefox/package.json +++ b/experimental/puppeteer-firefox/package.json @@ -9,7 +9,7 @@ "node": ">=8.9.4" }, "puppeteer": { - "firefox_revision": "3ba79216e3c5ae4e85006047cdd93eac4197427d" + "firefox_revision": "764023af0aa07d232984dec6bc81d9e904f25ddb" }, "scripts": { "install": "node install.js", diff --git a/test/evaluation.spec.js b/test/evaluation.spec.js index ddab30b50fd..044823e308d 100644 --- a/test/evaluation.spec.js +++ b/test/evaluation.spec.js @@ -209,7 +209,7 @@ module.exports.addTests = function({testRunner, expect}) { const result = await page.evaluate(() => document.execCommand('copy')); expect(result).toBe(true); }); - it_fails_ffox('should throw a nice error after a navigation', async({page, server}) => { + it('should throw a nice error after a navigation', async({page, server}) => { const executionContext = await page.mainFrame().executionContext(); await Promise.all([