Fixes an NPD by not disposing of the original handle #4280
Conversation
Call the defer function with the non nil handle, and allow the handle to be overwritten later. Now the handle in the defer will still be non-nil.
ankur22
requested review from
mstoykov,
oleiade and
inancgumus
and removed request for
a team
| defer func(handle *ElementHandle) { | ||
| if err := handle.Dispose(); err != nil { | ||
| err = fmt.Errorf("disposing element handle: %w", err) | ||
| rerr = errors.Join(err, rerr) | ||
| } | ||
| }() | ||
| }(handle) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- The old handle is disposed only when;
- 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?
There was a problem hiding this comment.
@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 won't adopt it. Erroring out if the handle is in the same execution context is just a safety check.
ankur22
force-pushed
the
fix/npd-frame-waitForSelector
branch
from
3f48b12 to
089ddf0
Compare
We don't want to dispose the new handle, only the original one, and only when the handle is successfully adopted. We also don't want to end the iteration if we fail to dispose of the original handle since we're not going to be working with it. Chances are that chrome will have dealt with it already.
ankur22
force-pushed
the
fix/npd-frame-waitForSelector
branch
from
089ddf0 to
ff4fcad
Compare
When i was looking into another issue I was unable to find a website or create a test site where it would find a handle in a different execution context and therefore need to perform an |
ankur22
changed the title
ankur22
changed the title
What?
If the handle needed to be adopted to the mainWorld execution context, it would dispose of the original and adopted handle. If the adoption failed then an NPD would occur as it was trying to call dispose on a nil.
Why?
The fix is to actually only dispose of the original handle and not the adopted handle since we want to work with the latest handle.
Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)
Linked to: #4279