Skip to content
This repository was archived by the owner on Jan 30, 2025. It is now read-only.

Fix mapJSHandle invalid memory address or nil pointer dereference #1544

Merged
merged 6 commits into from
Nov 26, 2024
Merged

Fix mapJSHandle invalid memory address or nil pointer dereference #1544

merged 6 commits into from
Nov 26, 2024

Conversation

inancgumus
Copy link
Collaborator

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

@inancgumus inancgumus added the mapping k6 browser to Goja mapping related. label Nov 19, 2024
@inancgumus inancgumus requested a review from a team as a code owner November 19, 2024 20:09
@inancgumus inancgumus requested review from olegbespalov and oleiade and removed request for a team November 19, 2024 20:09
@inancgumus inancgumus mentioned this pull request Nov 19, 2024
Closed
@inancgumus inancgumus self-assigned this Nov 19, 2024
@inancgumus inancgumus requested a review from ankur22 November 19, 2024 20:11
ankur22
ankur22 reviewed Nov 20, 2024
View reviewed changes
Copy link
Collaborator

@ankur22 ankur22 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
  1. Do we need tests for at least one of these methods (maybe just on page?).

@inancgumus
Copy link
Collaborator Author

inancgumus commented Nov 20, 2024 •
edited
Loading

@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.

  1. 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.

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

Would adding a test to examples/ be fine?