Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes an NPD by not disposing of the original handle #4280

Merged
merged 2 commits into from
Jan 27, 2025
Merged

Fixes an NPD by not disposing of the original handle #4280

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86e90c4
Fix an NPD when handle is overwritten with nil
ankur22 Jan 24, 2025
ff4fcad
Fix dispose of original handle
ankur22 Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions internal/js/modules/k6/browser/common/frame.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,12 +500,12 @@ func (f *Frame) waitForSelector(selector string, opts *FrameWaitForSelectorOptio
// an element should belong to the current execution context.
// otherwise, we should adopt it to this execution context.
if ec != handle.execCtx {
defer func() {
defer func(handle *ElementHandle) {
if err := handle.Dispose(); err != nil {
err = fmt.Errorf("disposing element handle: %w", err)
rerr = errors.Join(err, rerr)
}
}()
}(handle)
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this fix strange. IMO either this fixes an additional bug, it creates a new bug ... or is unnecessary 😅

So previously we got a handle, and then we adopted, and then we Disposed of it - which seems like ... not what we want?

We are now no longer going to do this - we will Dispose of the not adopted handle each time. Regardless of whether the adoption error-ed or not.

So are we fixing an additonal bug? Should we have always been disposing the previous handle?
Or should we dispose the newly created handle if it was actually created or the old one if it error-ed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good spot. It doesn't make sense to dispose of the new handle. Instead the previous handle needs to be disposed of when we have the adopted handle.

I'll change it so that:

  1. The old handle is disposed only when;
  2. the adoption was successful.

It looks like we currently error on when the handle is already part of the correct execution context, which doesn't seem right. We should probably catch that error here; not dispose (since it is the original handle) and carry on with the process as normal. However, we also perform the same check before trying to adopt, so i will leave it as it is.

Tbh, i'm not entirely sure of the logic here, why we need to adopt and why we need to be explicit in disposing of the older handle. @inancgumus do you have an idea of why we need to do all this in the first place?

Copy link
Contributor Author

@ankur22 ankur22 Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed in ff4fcad

Copy link
Member

@inancgumus inancgumus Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@ankur22 We adopt a new handle because the older one's execution context is no longer valid (e.g., its frame might have been navigated away). We dispose of the older handle because we now know that it's no longer valid. If the element belongs to the same execution context, we don't need to adopt it. Erroring out if the handle is in the same execution context is just a safety check.

if handle, err = ec.adoptElementHandle(handle); err != nil {
return nil, fmt.Errorf("waiting for selector %q: adopting element handle: %w", selector, err)
}
Expand Down
Loading