browser: Fix JS native objects override #4709
Conversation
b648350 to
67bd13f
Compare
67bd13f to
a8a9c8c
Compare
| document.documentElement.appendChild(iframe); | ||
| const win = iframe.contentWindow; | ||
| document.documentElement.removeChild(iframe); |
There was a problem hiding this comment.
I am hopeful that adding and removing an iframe before it has any chance of actually redrawing is enough for this to not have side effects.
But it seems kind of risky. Unfortunately my quick search hasn't shown another way to do this, so 🤷
I guess another more involved option will be to not depend on map or set ... but to be honest someone breaking them seems pretty ... strange and likely a much bigger problem for them than this script not working.
There was a problem hiding this comment.
I am hopeful that adding and removing an iframe before it has any chance of actually redrawing is enough for this to not have side effects. But it seems kind of risky. Unfortunately my quick search hasn't shown another way to do this, so 🤷
This user can test their webpage on other test engines without issues. This never-painting iframe method resolves the user's issue, and all our integration and E2E tests pass. Other test engines use a similar approach.
I guess another more involved option will be to not depend on map or set ... but to be honest someone breaking them seems pretty ... strange and likely a much bigger problem for them than this script not working.
I tried that approach before concluding on this one. It can quickly become error-prone and complicated, since we already have a working (injected) script. Besides, users can also overwrite other native objects (e.g., not just Set), making it work fine for their webpages. We'll eventually end up using nothing native in our script 😅 Just because users shim native objects that shouldn't break k6. Still, I'm open to other solutions 🙇
a8a9c8c to
f06b891
Compare
Avoids a page under the test to overwrite native JavaScript objects, like Set and Map by injecting a temporary and invisible frame in an execution context. We can extend this to support other native objects in the future. However, for now, this solves one of the user's issue.
cc900fe to
c17431d
Compare
|
@inancgumus However, the CI seems to fail on |
c17431d to
d5b5795
Compare
|
@codebien, thanks for the review 🙇
No, it's definitely not 😅 I'm trying to fix the flakiness on CI 🤞 It runs fine locally.
I believe listing all the other objects would be cumbersome in an issue, as we would need to update that issue every time we add (or remove) a new native object in our injected script (error-prone and tedious). We used |
792b5c6 to
7eb7351
Compare
7eb7351 to
c424e2c
Compare
|
@codebien I've simplified the test and also eliminated the flakiness. |
What?
Avoids a page under the test to overwrite native JavaScript objects, like
SetandMap, by injecting a temporary and invisible frame in execution contexts. We can extend this to support other native objects in the future. However, for now, this solves crucial issues for users.Note: I will update the release notes after I get the approvals, but before the merge.
Why?
See #4687 for details.
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Updates: #4687