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.

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

chore: builtins #35197

Merged
merged 2 commits into from
Mar 25, 2025
Merged

chore: builtins #35197

merged 2 commits into from
Mar 25, 2025

Conversation

dgozman
Copy link
Contributor

@dgozman dgozman commented Mar 14, 2025

This introduces a "script to evaluate on new document" that saves certain builtin objects like Map as an unlisted global property. Later on, every injected script will try to use the saved builtins.

The idea is to workaround pages that mess around with certain objects like Set or Map and break Playwright's injected code.

All injected source files are now subject to an eslint rule that forces the use of Builtins.Map instead of Map or similar. Many utility functions got a builtins argument to be able to construct non-overridden builtins.

Pros:

  • Playwright should work on a broader set of web pages.
  • Over time, we can cover more builtin classes/functions.

Cons:

  • Passing around builtins object in the codebase.
  • Extra eslint rule.
  • A tiny but mandatory script evaluated on each page load.

References #34443. References #34628.

@dgozman dgozman added the CQ1 label Mar 14, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@dgozman dgozman added CQ1 and removed CQ1 labels Mar 16, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@pavelfeldman pavelfeldman left a comment

Choose a reason for hiding this comment

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

I would rather const { Map } = window.builtins;

This comment has been minimized.

dgozman added 2 commits March 25, 2025 11:33
This introduces a "script to evaluate on new document" that
saves certain builtin objects like `Map` into an unguessable
global property. Later on, every injected script will try to
use the saved builtins.

The idea is to workaround pages that mess around with certain
objects like `Set` or `Map` and break Playwright's injected code.

All injected source files are now subject to an eslint rule
that forces the use of `Builtins.Map` instead of `Map` or similar.
Many utility functions got a `builtins` argument to be able to
construct non-overridden builtins.

Pros:
- Playwright should work on a broader set of web pages.
- Over time, we can cover more builtin classes/functions.

Cons:
- Passing around `builtins` object in the codebase.
- Extra eslint rule.
- A tiny but mandatory script evaluated on each page load.
@dgozman dgozman added CQ1 and removed CQ1 labels Mar 25, 2025
Copy link
Contributor

Test results for "tests others"

4 flaky ⚠️ [electron-page] › tests/page/page-request-continue.spec.ts:72:3 › should delete header with undefined value @electron-macos-latest
⚠️ [electron-page] › tests/page/page-request-continue.spec.ts:72:3 › should delete header with undefined value @electron-ubuntu-latest
⚠️ [electron-page] › tests/page/page-request-continue.spec.ts:72:3 › should delete header with undefined value @electron-windows-latest
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @realtime-time-runner-chromium-linux

21793 passed, 512 skipped
✔️✔️✔️

Merge workflow run.

Copy link
Contributor

Test results for "tests 1"

2 flaky ⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18

38839 passed, 809 skipped
✔️✔️✔️

Merge workflow run.

Copy link
Contributor

Test results for "tests 2"

9 failed
❌ [chromium-library] › tests/library/browsertype-connect.spec.ts:439:5 › run-server › should respect selectors @chrome-beta-macos-latest
❌ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:171:5 › should work with --save-har @chrome-beta-macos-latest
❌ [chromium-library] › tests/library/browsertype-connect.spec.ts:670:5 › run-server › should fulfill with global fetch result @chrome-macos-latest
❌ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:171:5 › should work with --save-har @chrome-macos-latest
❌ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:182:5 › should work with --save-har and --save-har-glob @chrome-macos-latest
❌ [chromium-library] › tests/library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @msedge-dev-macos-latest
❌ [webkit-library] › tests/library/browsercontext-viewport-mobile.spec.ts:116:5 › mobile viewport › default mobile viewports to 980 width @tracing-webkit
❌ [webkit-library] › tests/library/video.spec.ts:441:5 › screencast › should work for popups @tracing-webkit
❌ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames crop @webkit-macos-13-xlarge

113 flaky ⚠️ [chromium-library] › tests/library/browser.spec.ts:54:5 › should dispatch page.on(close) upon browser.close and reject evaluate @channel-chromium-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-base-url.spec.ts:37:3 › should construct a new URL when a baseURL in browserType.launchPersistentContext is passed to page.goto @channel-chromium-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-proxy.spec.ts:27:3 › should work when passing the proxy only on the context level @channel-chromium-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:386:5 › run-server › should emit close events on pages and contexts @channel-chromium-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @channel-chromium-macos-latest
⚠️ [chromium-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @channel-chromium-macos-latest
⚠️ [chromium-library] › tests/library/browser.spec.ts:54:5 › should dispatch page.on(close) upon browser.close and reject evaluate @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:267:5 › launchServer › should support slowmo option @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:400:5 › run-server › should terminate network waiters @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:484:5 › run-server › should not throw on close after disconnect @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-launch-server.spec.ts:29:5 › launch server › should work with host @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/chromium/chromium.spec.ts:151:15 › should close service worker together with the context @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/chromium/connect-over-cdp.spec.ts:26:5 › should connect to an existing cdp session @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:182:5 › should work with --save-har and --save-har-glob @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-base-url.spec.ts:37:3 › should construct a new URL when a baseURL in browserType.launchPersistentContext is passed to page.goto @chrome-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:199:5 › launchServer › should be able to connect two browsers at the same time @chrome-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:439:5 › run-server › should respect selectors @chrome-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-launch-server.spec.ts:23:5 › launch server › should work @chrome-macos-latest
⚠️ [chromium-library] › tests/library/chromium/chromium.spec.ts:151:15 › should close service worker together with the context @chrome-macos-latest
⚠️ [chromium-library] › tests/library/har.spec.ts:85:3 › should have pages in persistent context @chrome-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @chrome-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:216:7 › should work with --save-har in nunit @chrome-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-test.spec.ts:88:5 › should not generate recordHAR with --save-har @chrome-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-test.spec.ts:112:5 › should generate routeFromHAR with --save-har and --save-har-glob @chrome-macos-latest
⚠️ [chromium-library] › tests/library/inspector/pause.spec.ts:23:3 › should resume when closing inspector @chrome-macos-latest
⚠️ [chromium-library] › tests/library/proxy.spec.ts:31:3 › should use proxy @smoke @chrome-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-fetch.spec.ts:125:5 › put should support params passed as object @chromium-headed-macos-14-xlarge
⚠️ [chromium-library] › tests/library/popup.spec.ts:264:3 › should not throw when click closes popup @chromium-headed-ubuntu-24.04
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-2.spec.ts:403:7 › cli codegen › should reset hover model on action when element detaches @chromium-headed-windows-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-3.spec.ts:222:7 › cli codegen › should generate frame locators (4) @chromium-headed-windows-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-3.spec.ts:636:7 › cli codegen › should consume pointer events @chromium-headed-windows-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-pick-locator.spec.ts:35:7 › should update locator highlight @chromium-headed-windows-latest
⚠️ [chromium-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @chromium-macos-14-xlarge
⚠️ [chromium-library] › tests/library/video.spec.ts:580:5 › screencast › should capture static page in persistent context @smoke @chromium-tip-of-tree-macos-13
⚠️ [chromium-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @chromium-tip-of-tree-macos-13--headed
⚠️ [chromium-library] › tests/library/trace-viewer.spec.ts:90:1 › should show empty trace viewer @chromium-tip-of-tree-windows-latest--headed
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-beta-ubuntu-22.04
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-beta-windows-latest
⚠️ [firefox-library] › tests/library/capabilities.spec.ts:252:3 › requestFullscreen @firefox-headed-macos-14-xlarge
⚠️ [firefox-library] › tests/library/video.spec.ts:306:5 › screencast › should expose video path blank page @firefox-headed-macos-14-xlarge
⚠️ [firefox-library] › tests/library/video.spec.ts:163:5 › screencast › should work with old options @firefox-headed-ubuntu-24.04
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-headed-ubuntu-24.04
⚠️ [firefox-library] › tests/library/browsercontext-csp.spec.ts:21:3 › should bypass CSP meta tag @smoke @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/browsercontext-har.spec.ts:487:3 › should update extracted har.zip for page @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/browsercontext-viewport.spec.ts:109:3 › should throw on tap if hasTouch is not enabled @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/browsercontext-viewport.spec.ts:123:12 › should support touch with null viewport @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/download.spec.ts:54:5 › download event › should report download when navigation turns into download @smoke @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/download.spec.ts:199:5 › download event › should save to overwritten filepath @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/hit-target.spec.ts:398:3 › should click in iframe with padding @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/inspector/cli-codegen-1.spec.ts:23:7 › cli codegen › should click @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/inspector/cli-codegen-3.spec.ts:222:7 › cli codegen › should generate frame locators (4) @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/inspector/title.spec.ts:35:5 › should update primary page URL when original primary closes @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/tracing.spec.ts:263:5 › should not include trace resources from the previous chunks @firefox-headed-windows-latest
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-headed-windows-latest
⚠️ [firefox-page] › tests/page/page-event-request.spec.ts:182:3 › should return response body when Cross-Origin-Opener-Policy is set @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/video.spec.ts:163:5 › screencast › should work with old options @firefox-macos-13-large
⚠️ [firefox-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @firefox-macos-13-large
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-macos-13-large
⚠️ [firefox-library] › tests/library/video.spec.ts:163:5 › screencast › should work with old options @firefox-macos-13-xlarge
⚠️ [firefox-library] › tests/library/video.spec.ts:163:5 › screencast › should work with old options @firefox-macos-14-large
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-macos-14-large
⚠️ [firefox-library] › tests/library/video.spec.ts:163:5 › screencast › should work with old options @firefox-macos-14-xlarge
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-24.04
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-windows-latest
⚠️ [chromium-library] › tests/library/beforeunload.spec.ts:20:3 › should close browser with beforeunload page @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/browser.spec.ts:54:5 › should dispatch page.on(close) upon browser.close and reject evaluate @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-base-url.spec.ts:37:3 › should construct a new URL when a baseURL in browserType.launchPersistentContext is passed to page.goto @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-proxy.spec.ts:27:3 › should work when passing the proxy only on the context level @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-beta-ubuntu-22.04
⚠️ [chromium-library] › tests/library/browsercontext-proxy.spec.ts:264:3 › should isolate proxy credentials between contexts @msedge-beta-windows-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-beta-windows-latest
⚠️ [chromium-library] › tests/library/beforeunload.spec.ts:20:3 › should close browser with beforeunload page @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-base-url.spec.ts:37:3 › should construct a new URL when a baseURL in browserType.launchPersistentContext is passed to page.goto @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-test.spec.ts:88:5 › should not generate recordHAR with --save-har @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-dev-ubuntu-22.04
⚠️ [chromium-library] › tests/library/beforeunload.spec.ts:20:3 › should close browser with beforeunload page @msedge-macos-latest
⚠️ [chromium-library] › tests/library/browser.spec.ts:54:5 › should dispatch page.on(close) upon browser.close and reject evaluate @msedge-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-macos-latest
⚠️ [chromium-library] › tests/library/chromium/connect-over-cdp.spec.ts:43:5 › should use logger in default context @msedge-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @msedge-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-aria.spec.ts:76:7 › should update aria snapshot highlight @msedge-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:171:5 › should work with --save-har @msedge-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-java.spec.ts:92:5 › should work with --save-har and --save-har-glob as java-library @msedge-macos-latest
⚠️ [chromium-library] › tests/library/proxy.spec.ts:93:11 › should proxy local network requests › by default › link-local @msedge-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-ubuntu-22.04
⚠️ [chromium-library] › tests/library/proxy.spec.ts:93:11 › should proxy local network requests › with other bypasses › localhost @msedge-ubuntu-22.04
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @tracing-firefox
⚠️ [webkit-library] › tests/library/browsercontext-viewport-mobile.spec.ts:124:5 › mobile viewport › respect meta viewport tag @tracing-webkit
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:276:14 › element screenshot › should restore viewport after page screenshot and exception @tracing-webkit
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:289:14 › element screenshot › should restore viewport after page screenshot and timeout @tracing-webkit
⚠️ [webkit-library] › tests/library/video.spec.ts:207:5 › screencast › should continue recording main page after popup closes @tracing-webkit
⚠️ [webkit-library] › tests/library/video.spec.ts:411:5 › screencast › should capture css transformation @tracing-webkit
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:468:7 › page screenshot › mask option › should work with elementhandle @tracing-webkit
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-1.spec.ts:610:7 › cli codegen › should select @webkit-headed-macos-14-xlarge
⚠️ [webkit-library] › tests/library/browsercontext-reuse.spec.ts:206:1 › should reset mouse position @webkit-headed-ubuntu-24.04
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-1.spec.ts:298:7 › cli codegen › should fill japanese text @webkit-headed-ubuntu-24.04
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-1.spec.ts:483:7 › cli codegen › should emit single keyup on ArrowDown @webkit-headed-ubuntu-24.04
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-1.spec.ts:116:7 › cli codegen › should click after same-document navigation @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-1.spec.ts:993:7 › cli codegen › should clear when recording is disabled @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-3.spec.ts:520:7 › cli codegen › should generate getByPlaceholder @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-3.spec.ts:607:7 › cli codegen › should generate getByLabel without regex @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-3.spec.ts:667:7 › cli codegen › should consume contextmenu events, despite a custom context menu @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-aria.spec.ts:76:7 › should update aria snapshot highlight @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-pick-locator.spec.ts:35:7 › should update locator highlight @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/popup.spec.ts:264:3 › should not throw when click closes popup @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames scale @webkit-macos-13-xlarge
⚠️ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames crop @webkit-macos-14-xlarge
⚠️ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames scale @webkit-macos-14-xlarge
⚠️ [webkit-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @webkit-macos-15-large
⚠️ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames scale @webkit-macos-15-xlarge
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-javascript.spec.ts:87:5 › should save the codegen output to a file if specified @webkit-windows-latest

236883 passed, 9312 skipped
✔️✔️✔️

Merge workflow run.

@dgozman dgozman merged commit 0467c28 into microsoft:main Mar 25, 2025
98 of 108 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants