fix(Frame): postpone evaluations until execution context gets created (…

…#1415) In Blink, frames don't necesserily have execution context all the time. DevTools Protocol precisely reports this situation, which results in Puppeteer's frame.executionContext() being null occasionally. However, from puppeteer point of view every frame will have at least a default executions context, sooner or later: - frame's execution context might be created naturally to run frame's javascript - if frame has no javascript, devtools protocol will issue execution context creation This patch builds up on this assumption and makes frame.executionContext() to be a promise. As a result, all the evaluations await for the execution context to be created first. Fixes #827, #1325 BREAKING CHANGE: this patch changes frame.executionContext() method to return a promise. To migrate onto a new behavior, await the context first before using it.
puppeteer · Nov 19, 2017 · 6512ce7 · 6512ce7
1 parent 48ccf1e
commit 6512ce7
Show file tree

Hide file tree

Showing 6 changed files with 105 additions and 40 deletions.
diff --git a/docs/api.md b/docs/api.md
@@ -1514,7 +1514,7 @@ await bodyHandle.dispose();
 ```
 
 #### frame.executionContext()
-- returns: <[ExecutionContext]> Execution context associated with this frame.
+- returns: <[Promise]<[ExecutionContext]>> Execution context associated with this frame.
 
 #### frame.isDetached()
 - returns: <[boolean]>

diff --git a/lib/ExecutionContext.js b/lib/ExecutionContext.js
@@ -19,12 +19,16 @@ const {helper} = require('./helper');
 class ExecutionContext {
   /**
    * @param {!Puppeteer.Session} client
-   * @param {string} contextId
+   * @param {!Object} contextPayload
    * @param {function(*):!JSHandle} objectHandleFactory
    */
-  constructor(client, contextId, objectHandleFactory) {
+  constructor(client, contextPayload, objectHandleFactory) {
     this._client = client;
-    this._contextId = contextId;
+    this._contextId = contextPayload.id;
+
+    const auxData = contextPayload.auxData || {isDefault: true};
+    this._frameId = auxData.frameId || null;
+    this._isDefault = !!auxData.isDefault;
     this._objectHandleFactory = objectHandleFactory;
   }
 

diff --git a/lib/FrameManager.js b/lib/FrameManager.js
@@ -41,6 +41,8 @@ class FrameManager extends EventEmitter {
     this._client.on('Page.frameNavigated', event => this._onFrameNavigated(event.frame));
     this._client.on('Page.frameDetached', event => this._onFrameDetached(event.frameId));
     this._client.on('Runtime.executionContextCreated', event => this._onExecutionContextCreated(event.context));
+    this._client.on('Runtime.executionContextDestroyed', event => this._onExecutionContextDestroyed(event.executionContextId));
+    this._client.on('Runtime.executionContextsCleared', event => this._onExecutionContextsCleared());
     this._client.on('Page.lifecycleEvent', event => this._onLifecycleEvent(event));
 
     this._handleFrameTree(frameTree);
@@ -144,20 +146,38 @@ class FrameManager extends EventEmitter {
   }
 
   _onExecutionContextCreated(contextPayload) {
-    const context = new ExecutionContext(this._client, contextPayload.id, this.createJSHandle.bind(this, contextPayload.id));
+    const context = new ExecutionContext(this._client, contextPayload, this.createJSHandle.bind(this, contextPayload.id));
     this._contextIdToContext.set(contextPayload.id, context);
 
-    const frameId = contextPayload.auxData && contextPayload.auxData.isDefault ? contextPayload.auxData.frameId : null;
-    const frame = this._frames.get(frameId);
-    if (!frame)
+    const frame = context._frameId ? this._frames.get(context._frameId) : null;
+    if (frame && context._isDefault)
+      frame._setDefaultContext(context);
+  }
+
+  /**
+   * @param {!ExecutionContext} context
+   */
+  _removeContext(context) {
+    const frame = context._frameId ? this._frames.get(context._frameId) : null;
+    if (frame && context._isDefault)
+      frame._setDefaultContext(null);
+  }
+
+  /**
+   * @param {string} executionContextId
+   */
+  _onExecutionContextDestroyed(executionContextId) {
+    const context = this._contextIdToContext.get(executionContextId);
+    if (!context)
       return;
-    frame._context = context;
-    for (const waitTask of frame._waitTasks)
-      waitTask.rerun();
+    this._contextIdToContext.delete(executionContextId);
+    this._removeContext(context);
   }
 
-  _onExecutionContextDestroyed(contextPayload) {
-    this._contextIdToContext.delete(contextPayload.id);
+  _onExecutionContextsCleared() {
+    for (const context of this._contextIdToContext.values())
+      this._removeContext(context);
+    this._contextIdToContext.clear();
   }
 
   /**
@@ -208,7 +228,11 @@ class Frame {
     this._parentFrame = parentFrame;
     this._url = '';
     this._id = frameId;
-    this._context = null;
+
+    this._contextPromise = null;
+    this._contextResolveCallback = null;
+    this._setDefaultContext(null);
+
     /** @type {!Set<!WaitTask>} */
     this._waitTasks = new Set();
     this._loaderId = '';
@@ -222,10 +246,26 @@ class Frame {
   }
 
   /**
-   * @return {!ExecutionContext}
+   * @param {?ExecutionContext} context
+   */
+  _setDefaultContext(context) {
+    if (context) {
+      this._contextResolveCallback.call(null, context);
+      this._contextResolveCallback = null;
+      for (const waitTask of this._waitTasks)
+        waitTask.rerun();
+    } else {
+      this._contextPromise = new Promise(fulfill => {
+        this._contextResolveCallback = fulfill;
+      });
+    }
+  }
+
+  /**
+   * @return {!Promise<!ExecutionContext>}
    */
   executionContext() {
-    return this._context;
+    return this._contextPromise;
   }
 
   /**
@@ -234,15 +274,17 @@ class Frame {
    * @return {!Promise<*>}
    */
   async evaluate(pageFunction, ...args) {
-    return this._context.evaluate(pageFunction, ...args);
+    const context = await this._contextPromise;
+    return context.evaluate(pageFunction, ...args);
   }
 
   /**
    * @param {string} selector
    * @return {!Promise<?ElementHandle>}
    */
   async $(selector) {
-    const handle = await this._context.evaluateHandle(selector => document.querySelector(selector), selector);
+    const context = await this._contextPromise;
+    const handle = await context.evaluateHandle(selector => document.querySelector(selector), selector);
     const element = handle.asElement();
     if (element)
       return element;
@@ -272,7 +314,8 @@ class Frame {
    * @return {!Promise<(!Object|undefined)>}
    */
   async $$eval(selector, pageFunction, ...args) {
-    const arrayHandle = await this._context.evaluateHandle(selector => Array.from(document.querySelectorAll(selector)), selector);
+    const context = await this._contextPromise;
+    const arrayHandle = await context.evaluateHandle(selector => Array.from(document.querySelectorAll(selector)), selector);
     const result = await this.evaluate(pageFunction, arrayHandle, ...args);
     await arrayHandle.dispose();
     return result;
@@ -283,7 +326,8 @@ class Frame {
    * @return {!Promise<!Array<!ElementHandle>>}
    */
   async $$(selector) {
-    const arrayHandle = await this._context.evaluateHandle(selector => document.querySelectorAll(selector), selector);
+    const context = await this._contextPromise;
+    const arrayHandle = await context.evaluateHandle(selector => document.querySelectorAll(selector), selector);
     const properties = await arrayHandle.getProperties();
     await arrayHandle.dispose();
     const result = [];
@@ -338,7 +382,8 @@ class Frame {
     if (typeof options.url === 'string') {
       const url = options.url;
       try {
-        return await this._context.evaluateHandle(addScriptUrl, url);
+        const context = await this._contextPromise;
+        return await context.evaluateHandle(addScriptUrl, url);
       } catch (error) {
         throw new Error(`Loading script from ${url} failed`);
       }
@@ -347,12 +392,14 @@ class Frame {
     if (typeof options.path === 'string') {
       let contents = await readFileAsync(options.path, 'utf8');
       contents += '//# sourceURL=' + options.path.replace(/\n/g, '');
-
-      return this._context.evaluateHandle(addScriptContent, contents);
+      const context = await this._contextPromise;
+      return context.evaluateHandle(addScriptContent, contents);
     }
 
-    if (typeof options.content === 'string')
-      return this._context.evaluateHandle(addScriptContent, options.content);
+    if (typeof options.content === 'string') {
+      const context = await this._contextPromise;
+      return context.evaluateHandle(addScriptContent, options.content);
+    }
 
     throw new Error('Provide an object with a `url`, `path` or `content` property');
 
@@ -392,7 +439,8 @@ class Frame {
     if (typeof options.url === 'string') {
       const url = options.url;
       try {
-        return await this._context.evaluateHandle(addStyleUrl, url);
+        const context = await this._contextPromise;
+        return await context.evaluateHandle(addStyleUrl, url);
       } catch (error) {
         throw new Error(`Loading style from ${url} failed`);
       }
@@ -401,12 +449,14 @@ class Frame {
     if (typeof options.path === 'string') {
       let contents = await readFileAsync(options.path, 'utf8');
       contents += '/*# sourceURL=' + options.path.replace(/\n/g, '') + '*/';
-
-      return await this._context.evaluateHandle(addStyleContent, contents);
+      const context = await this._contextPromise;
+      return await context.evaluateHandle(addStyleContent, contents);
     }
 
-    if (typeof options.content === 'string')
-      return await this._context.evaluateHandle(addStyleContent, options.content);
+    if (typeof options.content === 'string') {
+      const context = await this._contextPromise;
+      return await context.evaluateHandle(addStyleContent, options.content);
+    }
 
     throw new Error('Provide an object with a `url`, `path` or `content` property');
 

diff --git a/lib/Page.js b/lib/Page.js
@@ -187,15 +187,17 @@ class Page extends EventEmitter {
    * @return {!Promise<!Puppeteer.JSHandle>}
    */
   async evaluateHandle(pageFunction, ...args) {
-    return this.mainFrame().executionContext().evaluateHandle(pageFunction, ...args);
+    const context = await this.mainFrame().executionContext();
+    return context.evaluateHandle(pageFunction, ...args);
   }
 
   /**
    * @param {!Puppeteer.JSHandle} prototypeHandle
    * @return {!Promise<!Puppeteer.JSHandle>}
    */
   async queryObjects(prototypeHandle) {
-    return this.mainFrame().executionContext().queryObjects(prototypeHandle);
+    const context = await this.mainFrame().executionContext();
+    return context.queryObjects(prototypeHandle);
   }
 
   /**

diff --git a/test/test.js b/test/test.js
@@ -311,6 +311,14 @@ describe('Page', function() {
       const result = await page.evaluate(() => Promise.resolve(8 * 7));
       expect(result).toBe(56);
     }));
+    it('should work right after framenavigated', SX(async function() {
+      let frameEvaluation = null;
+      page.on('framenavigated', async frame => {
+        frameEvaluation = frame.evaluate(() => 6 * 7);
+      });
+      await page.goto(EMPTY_PAGE);
+      expect(await frameEvaluation).toBe(42);
+    }));
     it('should work from-inside an exposed function', SX(async function() {
       // Setup inpage callback, which calls Page.evaluate
       await page.exposeFunction('callController', async function(a, b) {
@@ -559,17 +567,19 @@ describe('Page', function() {
       await FrameUtils.attachFrame(page, 'frame1', EMPTY_PAGE);
       expect(page.frames().length).toBe(2);
       const [frame1, frame2] = page.frames();
-      expect(frame1.executionContext()).toBeTruthy();
-      expect(frame2.executionContext()).toBeTruthy();
-      expect(frame1.executionContext() !== frame2.executionContext()).toBeTruthy();
+      const context1 = await frame1.executionContext();
+      const context2 = await frame2.executionContext();
+      expect(context1).toBeTruthy();
+      expect(context2).toBeTruthy();
+      expect(context1 !== context2).toBeTruthy();
 
       await Promise.all([
-        frame1.executionContext().evaluate(() => window.a = 1),
-        frame2.executionContext().evaluate(() => window.a = 2)
+        context1.evaluate(() => window.a = 1),
+        context2.evaluate(() => window.a = 2)
       ]);
       const [a1, a2] = await Promise.all([
-        frame1.executionContext().evaluate(() => window.a),
-        frame2.executionContext().evaluate(() => window.a)
+        context1.evaluate(() => window.a),
+        context2.evaluate(() => window.a)
       ]);
       expect(a1).toBe(1);
       expect(a2).toBe(2);

diff --git a/utils/doclint/check_public_api/index.js b/utils/doclint/check_public_api/index.js
@@ -28,7 +28,6 @@ const EXCLUDE_CLASSES = new Set([
   'Multimap',
   'NavigatorWatcher',
   'NetworkManager',
-  'ProxyStream',
   'Session',
   'TaskQueue',
   'WaitTask',