fix(firefox): assorted fixes to evaluation and remote objects (#511)

package.json

@@ -9,7 +9,7 @@
   "main": "index.js",
   "playwright": {
     "chromium_revision": "724623",
-    "firefox_revision": "1013",
+    "firefox_revision": "1014",
     "webkit_revision": "1092"
   },
   "scripts": {

src/firefox/ffExecutionContext.ts

@@ -30,25 +30,15 @@ export class FFExecutionContext implements js.ExecutionContextDelegate {
   }
 
   async evaluate(context: js.ExecutionContext, returnByValue: boolean, pageFunction: Function | string, ...args: any[]): Promise<any> {
-    if (returnByValue) {
-      try {
-        const handle = await this.evaluate(context, false /* returnByValue */, pageFunction, ...args as any);
-        const result = await handle.jsonValue();
-        await handle.dispose();
-        return result;
-      } catch (e) {
-        if (e.message.includes('cyclic object value') || e.message.includes('Object is not serializable'))
-          return undefined;
-        throw e;
-      }
-    }
-
     if (helper.isString(pageFunction)) {
       const payload = await this._session.send('Runtime.evaluate', {
         expression: pageFunction.trim(),
+        returnByValue,
         executionContextId: this._executionContextId,
       }).catch(rewriteError);
       checkException(payload.exceptionDetails);
+      if (returnByValue)
+        return deserializeValue(payload.result!);
       return context._createHandle(payload.result);
     }
     if (typeof pageFunction !== 'function')
@@ -94,6 +84,7 @@ export class FFExecutionContext implements js.ExecutionContextDelegate {
       callFunctionPromise = this._session.send('Runtime.callFunction', {
         functionDeclaration: functionText,
         args: protocolArgs,
+        returnByValue,
         executionContextId: this._executionContextId
       });
     } catch (err) {
@@ -103,9 +94,13 @@ export class FFExecutionContext implements js.ExecutionContextDelegate {
     }
     const payload = await callFunctionPromise.catch(rewriteError);
     checkException(payload.exceptionDetails);
+    if (returnByValue)
+      return deserializeValue(payload.result!);
     return context._createHandle(payload.result);
 
-    function rewriteError(error: Error) : never {
+    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!'))
         throw new Error('Execution context was destroyed, most likely because of a navigation.');
       throw error;

src/firefox/ffPage.ts

@@ -306,17 +306,23 @@ export class FFPage implements PageDelegate {
   }
 
   async getContentFrame(handle: dom.ElementHandle): Promise<frames.Frame | null> {
-    const { frameId } = await this._session.send('Page.contentFrame', {
+    const { contentFrameId } = await this._session.send('Page.describeNode', {
       frameId: handle._context.frame._id,
       objectId: toRemoteObject(handle).objectId!,
     });
-    if (!frameId)
+    if (!contentFrameId)
       return null;
-    return this._page._frameManager.frame(frameId);
+    return this._page._frameManager.frame(contentFrameId);
   }
 
   async getOwnerFrame(handle: dom.ElementHandle): Promise<frames.Frame | null> {
-    return handle._context.frame;
+    const { ownerFrameId } = await this._session.send('Page.describeNode', {
+      frameId: handle._context.frame._id,
+      objectId: toRemoteObject(handle).objectId!,
+    });
+    if (!ownerFrameId)
+      return null;
+    return this._page._frameManager.frame(ownerFrameId);
   }
 
   isElementHandle(remoteObject: any): boolean {

test/elementhandle.spec.js

@@ -134,7 +134,7 @@ module.exports.describe = function({testRunner, expect, FFOX, CHROMIUM, WEBKIT})
       const elementHandle = await frame.evaluateHandle(() => document.querySelector('#frame1'));
       expect(await elementHandle.ownerFrame()).toBe(frame);
     });
-    it.skip(FFOX)('should work for cross-frame evaluations', async({page,server}) => {
+    it('should work for cross-frame evaluations', async({page,server}) => {
       await page.goto(server.EMPTY_PAGE);
       await utils.attachFrame(page, 'frame1', server.EMPTY_PAGE);
       const frame = page.mainFrame();

test/evaluation.spec.js

@@ -61,8 +61,16 @@ module.exports.describe = function({testRunner, expect, FFOX, CHROMIUM, WEBKIT})
       await page.goto(server.PREFIX + '/global-var.html');
       expect(await page.evaluate('globalVar')).toBe(123);
     });
-    it.skip(FFOX)('should return undefined for objects with symbols', async({page, server}) => {
+    it('should return undefined for objects with symbols', async({page, server}) => {
       expect(await page.evaluate(() => [Symbol('foo4')])).toBe(undefined);
+      expect(await page.evaluate(() => {
+        const a = { };
+        a[Symbol('foo4')] = 42;
+        return a;
+      })).toEqual({});
+      expect(await page.evaluate(() => {
+        return { foo: [{ a: Symbol('foo4') }] };
+      })).toBe(undefined);
     });
     it('should work with function shorthands', async({page, server}) => {
       const a = {
@@ -166,7 +174,7 @@ module.exports.describe = function({testRunner, expect, FFOX, CHROMIUM, WEBKIT})
     it('should properly serialize undefined fields', async({page}) => {
       expect(await page.evaluate(() => ({a: undefined}))).toEqual({});
     });
-    it.skip(FFOX)('should properly serialize null arguments', async({page}) => {
+    it('should properly serialize null arguments', async({page}) => {
       expect(await page.evaluate(x => x, null)).toEqual(null);
     });
     it('should properly serialize null fields', async({page}) => {
@@ -184,7 +192,7 @@ module.exports.describe = function({testRunner, expect, FFOX, CHROMIUM, WEBKIT})
       });
       expect(result).toBe(undefined);
     });
-    it.skip(FFOX)('should be able to throw a tricky error', async({page, server}) => {
+    it('should be able to throw a tricky error', async({page, server}) => {
       const windowHandle = await page.evaluateHandle(() => window);
       const errorText = await windowHandle.jsonValue().catch(e => e.message);
       const error = await page.evaluate(errorText => {

test/jshandle.spec.js

@@ -82,7 +82,7 @@ module.exports.describe = function({testRunner, expect, CHROMIUM, FFOX, WEBKIT})
       const json = await aHandle.jsonValue();
       expect(json).toEqual({foo: 'bar'});
     });
-    it.skip(FFOX)('should not work with dates', async({page, server}) => {
+    it('should not work with dates', async({page, server}) => {
       const dateHandle = await page.evaluateHandle(() => new Date('2017-09-26T00:00:00.000Z'));
       const json = await dateHandle.jsonValue();
       expect(json).toEqual({});