Fix mapJSHandle invalid memory address or nil pointer dereference

[inancgumus]

What?

Add missing function arguments check to JSHandle mapping.

Why?

#1543

Checklist

• I have performed a self-review of my code
• I have added tests for my changes
• I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Updates #1543

[ankur22]

Thanks for the fix! I like where it is going.

Two points:

1. Can we look to fix the other evaluate/evaluateHandle methods on page and frame? In a new PR if you prefer, but happy to have them as separate commits in this PR 🙂
2. This does indeed fix the issues when evaluate() is used without pageFunc. It however breaks one thing which is that you can no longer pass in a string:

// As a string that is callable
console.log(await jsHandle.evaluate("node => node.innerText"));
// As a function
console.log(await jsHandle.evaluate(node => node.innerText));

3. Do we need tests for at least one of these methods (maybe just on page?).

[inancgumus]

@ankur22, sure thing!

1. Can we look to fix the other evaluate/evaluateHandle methods on page and frame? In a new PR if you prefer, but happy to have them as separate commits in this PR 🙂

Sure :) Can you repurpose issue #1543? Maybe move this explanation to the issue description so that we can understand what the issue entails. Currently, it looks like it's about mapJSHandle.

2. This does indeed fix the issues when evaluate() is used without pageFunc. It however breaks one thing which is that you can no longer pass in a string:

Are you sure that it doesn't work with strings? I've tried this, and it worked:

const n = await handle.evaluate('() => 5 * 42');

But this is a nice catch because now we don't accept non-functions 🙇

await handle.evaluate('document.title');

Update: After evaluation, I noticed that we already don't accept such a string. We only accept this form: () => document.title. Still, I'll modify the code to check for empty strings and nil functions instead.

3. Do we need tests for at least one of these methods (maybe just on page?).

Would adding a test to examples/ be fine?

[ankur22]

Sure :) Can you repurpose issue #1543? Maybe move #1543 (comment) to the issue description so that we can understand what the issue entails. Currently, it looks like it's about mapJSHandle.

Sure, done.

Are you sure that it doesn't work with strings?

Yeah, this script below no longer works with this branch:

import { browser } from 'k6/browser';

export const options = {
  scenarios: {
    ui: {
      executor: 'shared-iterations',
      options: {
        browser: {
            type: 'chromium',
        },
      },
    },
  },
}

export default async function() {
  const page = await browser.newPage();

  try {
    await page.goto('https://test.k6.io/my_messages.php');

    const jsHandle = await page.waitForSelector('h2');

    // As a string that is callable
    console.log(await jsHandle.evaluate("node => node.innerText"));
    
    // As a function
    console.log(await jsHandle.evaluate(node => node.innerText));
  } finally {
    await page.close();
  }
}

Would adding a test to examples/ be fine?

I think we should have table tests (integration tests) in the tests package, unless there's a reason it's not possible to create them? A test in the examples folder would be a good addition too.

[ankur22]

LGTM 🚀