fix(frames): make sure evaluation does not hang in detached iframes (#3770)

Fix #3261

lib/FrameManager.js

@@ -424,6 +424,8 @@ class Frame {
    * @return {!Promise<!ExecutionContext>}
    */
   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;
   }
 
@@ -433,7 +435,7 @@ class Frame {
    * @return {!Promise<!Puppeteer.JSHandle>}
    */
   async evaluateHandle(pageFunction, ...args) {
-    const context = await this._contextPromise;
+    const context = await this.executionContext();
     return context.evaluateHandle(pageFunction, ...args);
   }
 
@@ -443,7 +445,7 @@ class Frame {
    * @return {!Promise<*>}
    */
   async evaluate(pageFunction, ...args) {
-    const context = await this._contextPromise;
+    const context = await this.executionContext();
     return context.evaluate(pageFunction, ...args);
   }
 
@@ -463,7 +465,7 @@ class Frame {
   async _document() {
     if (this._documentPromise)
       return this._documentPromise;
-    this._documentPromise = this._contextPromise.then(async context => {
+    this._documentPromise = this.executionContext().then(async context => {
       const document = await context.evaluateHandle('document');
       return document.asElement();
     });
@@ -601,7 +603,7 @@ class Frame {
     } = options;
     if (url !== null) {
       try {
-        const context = await this._contextPromise;
+        const context = await this.executionContext();
         return (await context.evaluateHandle(addScriptUrl, url, type)).asElement();
       } catch (error) {
         throw new Error(`Loading script from ${url} failed`);
@@ -611,12 +613,12 @@ class Frame {
     if (path !== null) {
       let contents = await readFileAsync(path, 'utf8');
       contents += '//# sourceURL=' + path.replace(/\n/g, '');
-      const context = await this._contextPromise;
+      const context = await this.executionContext();
       return (await context.evaluateHandle(addScriptContent, contents, type)).asElement();
     }
 
     if (content !== null) {
-      const context = await this._contextPromise;
+      const context = await this.executionContext();
       return (await context.evaluateHandle(addScriptContent, content, type)).asElement();
     }
 
@@ -671,7 +673,7 @@ class Frame {
     } = options;
     if (url !== null) {
       try {
-        const context = await this._contextPromise;
+        const context = await this.executionContext();
         return (await context.evaluateHandle(addStyleUrl, url)).asElement();
       } catch (error) {
         throw new Error(`Loading style from ${url} failed`);
@@ -681,12 +683,12 @@ class Frame {
     if (path !== null) {
       let contents = await readFileAsync(path, 'utf8');
       contents += '/*# sourceURL=' + path.replace(/\n/g, '') + '*/';
-      const context = await this._contextPromise;
+      const context = await this.executionContext();
       return (await context.evaluateHandle(addStyleContent, contents)).asElement();
     }
 
     if (content !== null) {
-      const context = await this._contextPromise;
+      const context = await this.executionContext();
       return (await context.evaluateHandle(addStyleContent, content)).asElement();
     }
 

test/frame.spec.js

@@ -57,6 +57,16 @@ module.exports.addTests = function({testRunner, expect}) {
     });
   });
 
+  describe('Frame.evaluate', function() {
+    it('should throw for detached frames', async({page, server}) => {
+      const frame1 = await utils.attachFrame(page, 'frame1', server.EMPTY_PAGE);
+      await utils.detachFrame(page, 'frame1');
+      let error = null;
+      await frame1.evaluate(() => 7 * 8).catch(e => error = e);
+      expect(error.message).toContain('Execution Context is not available in detached frame');
+    });
+  });
+
   describe('Frame Management', function() {
     it('should handle nested frames', async({page, server}) => {
       await page.goto(server.PREFIX + '/frames/nested-frames.html');
@@ -146,5 +156,19 @@ module.exports.addTests = function({testRunner, expect}) {
       expect(page.frames()[1].parentFrame()).toBe(page.mainFrame());
       expect(page.frames()[2].parentFrame()).toBe(page.mainFrame());
     });
+    it('should report different frame instance when frame re-attaches', async({page, server}) => {
+      const frame1 = await utils.attachFrame(page, 'frame1', server.EMPTY_PAGE);
+      await page.evaluate(() => {
+        window.frame = document.querySelector('#frame1');
+        window.frame.remove();
+      });
+      expect(frame1.isDetached()).toBe(true);
+      const [frame2] = await Promise.all([
+        utils.waitEvent(page, 'frameattached'),
+        page.evaluate(() => document.body.appendChild(window.frame)),
+      ]);
+      expect(frame2.isDetached()).toBe(false);
+      expect(frame1).not.toBe(frame2);
+    });
   });
 };