Skip to content

chore: replace some progress.cleanup calls with try/catch #36769

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

Merged
merged 2 commits into from
Jul 29, 2025

Conversation

dgozman
Copy link
Contributor

@dgozman dgozman commented Jul 23, 2025

  • Now that legacy timeout mechanism is gone, we can replace all kinds of cleanups with an explicit try/catch.
  • This removes progress.raceWithCleanup() due to unclear semantics, and replaces it with a helper raceUncancellableOperationWithCleanup() that should be rarely used.

@dgozman dgozman requested a review from yury-s July 23, 2025 19:41
@dgozman dgozman force-pushed the chore-remote-race-with-cleanup branch from a176838 to 2347994 Compare July 23, 2025 19:52

This comment has been minimized.

This comment has been minimized.

@dgozman dgozman force-pushed the chore-remote-race-with-cleanup branch from 2347994 to 277c207 Compare July 25, 2025 07:27

This comment has been minimized.

@dgozman dgozman changed the title chore: remove Progress.raceWithCleanup chore: replace some progress.cleanup calls with try/catch Jul 25, 2025
@dgozman dgozman force-pushed the chore-remote-race-with-cleanup branch from 7306f9d to abb91ee Compare July 25, 2025 08:36
@dgozman dgozman added the CQ1 label Jul 25, 2025

This comment has been minimized.

Copy link
Contributor

Test results for "tests others"

22255 passed, 514 skipped
✔️✔️✔️

Merge workflow run.

This comment has been minimized.

Copy link
Contributor

Test results for "tests 2"

12 fatal errors, not part of any test
5 failed
❌ [chromium-library] › library/browsertype-connect.spec.ts:634:5 › launchServer › should filter launch options @chrome-beta-macos-latest
❌ [chromium-library] › library/debug-controller.spec.ts:138:1 › should navigate all @chrome-beta-macos-latest
❌ [chromium-library] › library/browsertype-connect.spec.ts:739:5 › launchServer › setInputFiles should preserve lastModified timestamp @chrome-macos-latest
❌ [chromium-library] › library/debug-controller.spec.ts:166:1 › should highlight all @chrome-macos-latest
❌ [firefox-library] › library/video.spec.ts:187:5 › screencast › should capture static page @firefox-headed-windows-latest

131 flaky ⚠️ [chromium-library] › library/beforeunload.spec.ts:20:3 › should close browser with beforeunload page @channel-chromium-macos-latest
⚠️ [chromium-library] › 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] › library/browsertype-connect.spec.ts:600:5 › launchServer › should be able to connect when the wsEndpoint is passed as an option @channel-chromium-macos-latest
⚠️ [chromium-library] › library/browsertype-connect.spec.ts:1044:3 › launchServer only › cannot launch another browser @channel-chromium-macos-latest
⚠️ [chromium-library] › library/debug-controller.spec.ts:138:1 › should navigate all @channel-chromium-macos-latest
⚠️ [chromium-library] › library/defaultbrowsercontext-1.spec.ts:28:3 › context.cookies() should work @smoke @channel-chromium-macos-latest
⚠️ [chromium-library] › library/har.spec.ts:78:3 › should have pages in persistent context @channel-chromium-macos-latest
⚠️ [chromium-library] › library/popup.spec.ts:258:3 › should not throw when click closes popup @channel-chromium-ubuntu-latest
⚠️ [chromium-library] › 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-beta-macos-latest
⚠️ [chromium-library] › library/browsercontext-reuse.spec.ts:195:5 › reuse launch › should not cache resources @chrome-beta-macos-latest
⚠️ [chromium-library] › library/browsertype-connect.spec.ts:647:5 › launchServer › should record trace with sources @chrome-beta-macos-latest
⚠️ [chromium-library] › library/debug-controller.spec.ts:149:1 › should reset for reuse @chrome-beta-macos-latest
⚠️ [chromium-library] › library/defaultbrowsercontext-2.spec.ts:174:3 › should handle timeout @chrome-beta-macos-latest
⚠️ [chromium-library] › library/har.spec.ts:78:3 › should have pages in persistent context @chrome-beta-macos-latest
⚠️ [chromium-library] › library/har.spec.ts:541:3 › should have popup requests @chrome-beta-macos-latest
⚠️ [chromium-library] › library/inspector/cli-codegen-2.spec.ts:93:7 › cli codegen › should not lead to an error if html gets clicked @chrome-beta-macos-latest
⚠️ [chromium-library] › library/inspector/cli-codegen-aria.spec.ts:76:7 › should update aria snapshot highlight @chrome-beta-macos-latest
⚠️ [chromium-page] › page/workers.spec.ts:25:3 › Page.workers @smoke @chrome-beta-macos-latest
⚠️ [chromium-page] › page/workers.spec.ts:150:3 › should report network activity @chrome-beta-macos-latest
⚠️ [chromium-page] › page/workers.spec.ts:271:3 › should support extra http headers @chrome-beta-macos-latest
⚠️ [chromium-library] › library/browser.spec.ts:54:5 › should dispatch page.on(close) upon browser.close and reject evaluate @chrome-macos-latest
⚠️ [chromium-library] › library/browsercontext-reuse.spec.ts:303:5 › reuse launch › should reset tracing @chrome-macos-latest
⚠️ [chromium-library] › library/browsertype-connect.spec.ts:142:5 › launchServer › should be able to reconnect to a browser @chrome-macos-latest
⚠️ [chromium-library] › library/chromium/chromium.spec.ts:151:15 › should close service worker together with the context @chrome-macos-latest
⚠️ [chromium-library] › library/debug-controller.spec.ts:138:1 › should navigate all @chrome-macos-latest
⚠️ [chromium-library] › library/defaultbrowsercontext-1.spec.ts:28:3 › context.cookies() should work @smoke @chrome-macos-latest
⚠️ [chromium-library] › library/inspector/cli-codegen-2.spec.ts:93:7 › cli codegen › should not lead to an error if html gets clicked @chrome-macos-latest
⚠️ [chromium-page] › page/workers.spec.ts:25:3 › Page.workers @smoke @chrome-macos-latest
⚠️ [chromium-library] › library/debug-controller.spec.ts:149:1 › should reset for reuse @chromium-headed-macos-14-xlarge
⚠️ [chromium-library] › library/defaultbrowsercontext-1.spec.ts:28:3 › context.cookies() should work @smoke @chromium-headed-macos-14-xlarge
⚠️ [chromium-library] › library/inspector/cli-codegen-pick-locator.spec.ts:23:7 › should inspect locator @chromium-headed-ubuntu-24.04
⚠️ [chromium-library] › library/video.spec.ts:580:5 › screencast › should capture static page in persistent context @smoke @chromium-headed-windows-latest
⚠️ [chromium-page] › page/page-drag.spec.ts:290:5 › Drag and drop › should work with the helper method @chromium-headed-windows-latest
⚠️ [chromium-library] › library/chromium/tracing.spec.ts:99:3 › should support a buffer without a path @chromium-macos-13-xlarge
⚠️ [chromium-library] › library/chromium/tracing.spec.ts:99:3 › should support a buffer without a path @chromium-macos-14-xlarge
⚠️ [chromium-library] › library/inspector/cli-codegen-3.spec.ts:226:7 › cli codegen › should generate frame locators (4) @chromium-macos-14-xlarge
⚠️ [chromium-library] › library/video.spec.ts:207:5 › screencast › should continue recording main page after popup closes @chromium-tip-of-tree-macos-13
⚠️ [chromium-page] › page/workers.spec.ts:25:3 › Page.workers @smoke @chromium-tip-of-tree-macos-13
⚠️ [chromium-page] › page/workers.spec.ts:150:3 › should report network activity @chromium-tip-of-tree-macos-13
⚠️ [chromium-library] › library/video.spec.ts:379:5 › screencast › should capture navigation @chromium-tip-of-tree-macos-13--headed
⚠️ [chromium-page] › page/workers.spec.ts:150:3 › should report network activity @chromium-tip-of-tree-macos-13--headed
⚠️ [chromium-library] › library/inspector/cli-codegen-pick-locator.spec.ts:35:7 › should update locator highlight @chromium-tip-of-tree-windows-latest--headed
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-beta-macos-latest
⚠️ [firefox-page] › page/page-event-request.spec.ts:182:3 › should return response body when Cross-Origin-Opener-Policy is set @firefox-beta-macos-latest
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-beta-ubuntu-22.04
⚠️ [firefox-page] › page/page-goto.spec.ts:467:3 › js redirect overrides url bar navigation @firefox-beta-ubuntu-22.04
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @firefox-beta-ubuntu-22.04
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-beta-windows-latest
⚠️ [firefox-library] › library/inspector/cli-codegen-pytest.spec.ts:54:5 › should save the codegen output to a file if specified @firefox-beta-windows-latest
⚠️ [firefox-library] › library/capabilities.spec.ts:244:3 › requestFullscreen @firefox-headed-macos-14-xlarge
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-headed-macos-14-xlarge
⚠️ [firefox-library] › library/browsercontext-basic.spec.ts:36:3 › should be able to click across browser contexts @firefox-headed-ubuntu-24.04
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-headed-ubuntu-24.04
⚠️ [firefox-library] › library/browsercontext-add-cookies.spec.ts:207:3 › should isolate persistent cookies @firefox-headed-windows-latest
⚠️ [firefox-library] › library/browsercontext-add-cookies.spec.ts:263:3 › should set multiple cookies @firefox-headed-windows-latest
⚠️ [firefox-library] › library/browsercontext-har.spec.ts:33:3 › should page.routeFromHAR, matching the method and following redirects @firefox-headed-windows-latest
⚠️ [firefox-library] › library/browsertype-launch.spec.ts:22:3 › should reject all promises when browser is closed @firefox-headed-windows-latest
⚠️ [firefox-library] › library/browsertype-launch.spec.ts:133:3 › should allow await using @firefox-headed-windows-latest
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-headed-windows-latest
⚠️ [firefox-library] › library/inspector/cli-codegen-pytest.spec.ts:70:5 › should work with --save-har @firefox-headed-windows-latest
⚠️ [firefox-library] › library/selector-generator.spec.ts:355:7 › selector generator › should prioritize attributes correctly › role @firefox-headed-windows-latest
⚠️ [firefox-library] › library/selector-generator.spec.ts:359:7 › selector generator › should prioritize attributes correctly › placeholder @firefox-headed-windows-latest
⚠️ [firefox-library] › library/selector-generator.spec.ts:363:7 › selector generator › should prioritize attributes correctly › name @firefox-headed-windows-latest
⚠️ [firefox-library] › library/trace-viewer.spec.ts:112:1 › should show tracing.group in the action list with location @firefox-headed-windows-latest
⚠️ [firefox-library] › library/video.spec.ts:379:5 › screencast › should capture navigation @firefox-headed-windows-latest
⚠️ [firefox-page] › page/page-click-scroll.spec.ts:55:3 › should scroll into view display:contents with a child @firefox-headed-windows-latest
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-macos-13-large
⚠️ [firefox-library] › library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @firefox-macos-13-large
⚠️ [firefox-page] › page/page-event-request.spec.ts:182:3 › should return response body when Cross-Origin-Opener-Policy is set @firefox-macos-13-large
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @firefox-macos-13-large
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-macos-13-xlarge
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @firefox-macos-13-xlarge
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-macos-14-large
⚠️ [firefox-page] › page/page-event-request.spec.ts:182:3 › should return response body when Cross-Origin-Opener-Policy is set @firefox-macos-14-large
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-macos-14-xlarge
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @firefox-macos-14-xlarge
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-ubuntu-24.04
⚠️ [firefox-library] › library/inspector/cli-codegen-aria.spec.ts:76:7 › should update aria snapshot highlight @firefox-ubuntu-24.04
⚠️ [firefox-page] › page/page-event-request.spec.ts:182:3 › should return response body when Cross-Origin-Opener-Policy is set @firefox-ubuntu-24.04
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @firefox-ubuntu-24.04
⚠️ [firefox-library] › library/debug-controller.spec.ts:79:1 › should pick element @firefox-windows-latest
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-windows-latest
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @firefox-windows-latest
⚠️ [chromium-library] › library/beforeunload.spec.ts:20:3 › should close browser with beforeunload page @msedge-beta-macos-latest
⚠️ [chromium-library] › library/browsercontext-proxy.spec.ts:27:3 › should work when passing the proxy only on the context level @msedge-beta-macos-latest
⚠️ [chromium-library] › library/browsercontext-reuse.spec.ts:94:5 › reuse launch › should re-add binding after reset @msedge-beta-macos-latest
⚠️ [chromium-library] › library/browsercontext-reuse.spec.ts:150:5 › reuse launch › should reset serviceworker that hangs in importScripts @msedge-beta-macos-latest
⚠️ [chromium-library] › library/defaultbrowsercontext-2.spec.ts:48:3 › should support forcedColors option @msedge-beta-macos-latest
⚠️ [chromium-library] › library/defaultbrowsercontext-2.spec.ts:229:3 › should connect to a browser with the default page @msedge-beta-macos-latest
⚠️ [chromium-library] › library/download.spec.ts:389:5 › download event › should delete downloads on browser gone @msedge-beta-macos-latest
⚠️ [chromium-library] › library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @msedge-beta-macos-latest
⚠️ [chromium-library] › library/permissions.spec.ts:244:5 › should be able to use the local-fonts API @msedge-beta-macos-latest
⚠️ [chromium-library] › library/signals.spec.ts:78:7 › signals › should close the browser on SIGINT @msedge-beta-macos-latest
⚠️ [chromium-library] › library/permissions.spec.ts:244:5 › should be able to use the local-fonts API @msedge-beta-ubuntu-22.04
⚠️ [chromium-library] › library/permissions.spec.ts:244:5 › should be able to use the local-fonts API @msedge-beta-windows-latest
⚠️ [chromium-library] › library/beforeunload.spec.ts:20:3 › should close browser with beforeunload page @msedge-dev-macos-latest
⚠️ [chromium-library] › library/browser.spec.ts:54:5 › should dispatch page.on(close) upon browser.close and reject evaluate @msedge-dev-macos-latest
⚠️ [chromium-library] › 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] › library/browsercontext-fetch.spec.ts:1262:3 › should work with connectOverCDP @msedge-dev-macos-latest
⚠️ [chromium-library] › library/browsercontext-proxy.spec.ts:27:3 › should work when passing the proxy only on the context level @msedge-dev-macos-latest
⚠️ [chromium-library] › library/browsercontext-reuse.spec.ts:94:5 › reuse launch › should re-add binding after reset @msedge-dev-macos-latest
⚠️ [chromium-library] › library/debug-controller.spec.ts:249:1 › should reset routes before reuse @msedge-dev-macos-latest
⚠️ [chromium-library] › library/defaultbrowsercontext-1.spec.ts:48:3 › context.addCookies() should work @msedge-dev-macos-latest
⚠️ [chromium-library] › library/defaultbrowsercontext-2.spec.ts:22:3 › should support hasTouch option @msedge-dev-macos-latest
⚠️ [chromium-library] › library/permissions.spec.ts:244:5 › should be able to use the local-fonts API @msedge-dev-macos-latest
⚠️ [chromium-library] › library/video.spec.ts:207:5 › screencast › should continue recording main page after popup closes @msedge-dev-macos-latest
⚠️ [chromium-page] › page/workers.spec.ts:150:3 › should report network activity @msedge-dev-macos-latest
⚠️ [chromium-library] › library/permissions.spec.ts:244:5 › should be able to use the local-fonts API @msedge-dev-ubuntu-22.04
⚠️ [chromium-library] › library/debug-controller.spec.ts:249:1 › should reset routes before reuse @msedge-macos-latest
⚠️ [chromium-library] › library/defaultbrowsercontext-1.spec.ts:48:3 › context.addCookies() should work @msedge-macos-latest
⚠️ [chromium-library] › library/defaultbrowsercontext-2.spec.ts:96:3 › should accept userDataDir @msedge-macos-latest
⚠️ [chromium-library] › library/defaultbrowsercontext-2.spec.ts:249:3 › user agent is up to date @msedge-macos-latest
⚠️ [chromium-library] › library/permissions.spec.ts:244:5 › should be able to use the local-fonts API @msedge-macos-latest
⚠️ [chromium-library] › library/permissions.spec.ts:244:5 › should be able to use the local-fonts API @msedge-ubuntu-22.04
⚠️ [chromium-library] › library/popup.spec.ts:258:3 › should not throw when click closes popup @tracing-chromium
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @tracing-firefox
⚠️ [firefox-page] › page/page-event-request.spec.ts:182:3 › should return response body when Cross-Origin-Opener-Policy is set @tracing-firefox
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @tracing-firefox
⚠️ [webkit-library] › library/inspector/cli-codegen-1.spec.ts:94:7 › cli codegen › should click twice @webkit-headed-ubuntu-22.04
⚠️ [webkit-library] › library/inspector/cli-codegen-aria.spec.ts:43:7 › should generate regex in aria snapshot @webkit-headed-ubuntu-22.04
⚠️ [webkit-page] › page/page-click.spec.ts:251:3 › should click on checkbox input and toggle @webkit-headed-ubuntu-24.04
⚠️ [webkit-library] › library/browsercontext-reuse.spec.ts:114:5 › reuse launch › should reset serviceworker @webkit-headed-windows-latest
⚠️ [webkit-library] › library/inspector/cli-codegen-3.spec.ts:25:7 › cli codegen › should click locator.first @webkit-headed-windows-latest
⚠️ [webkit-library] › library/inspector/cli-codegen-3.spec.ts:74:7 › cli codegen › should click locator.nth @webkit-headed-windows-latest
⚠️ [webkit-library] › library/inspector/cli-codegen-3.spec.ts:582:7 › cli codegen › should generate getByLabel @webkit-headed-windows-latest
⚠️ [webkit-library] › library/inspector/cli-codegen-3.spec.ts:743:7 › cli codegen › should assert value @webkit-headed-windows-latest
⚠️ [webkit-library] › library/inspector/cli-codegen-aria.spec.ts:43:7 › should generate regex in aria snapshot @webkit-headed-windows-latest
⚠️ [webkit-page] › page/wheel.spec.ts:70:3 › should dispatch wheel events after context menu was opened @webkit-macos-14-xlarge
⚠️ [webkit-library] › library/trace-viewer.spec.ts:1313:1 › should pick locator in iframe @webkit-macos-15-large
⚠️ [webkit-library] › library/browsercontext-reuse.spec.ts:114:5 › reuse launch › should reset serviceworker @webkit-windows-latest
⚠️ [webkit-library] › library/inspector/cli-codegen-pick-locator.spec.ts:35:7 › should update locator highlight @webkit-windows-latest

240016 passed, 9596 skipped
✔️✔️✔️

Merge workflow run.

dgozman added 2 commits July 28, 2025 16:09
Now that legacy timeout mechanism is gone, we can replace
all kinds of cleanups with an explicit try/catch.
@dgozman dgozman force-pushed the chore-remote-race-with-cleanup branch from abb91ee to 0bd01ea Compare July 28, 2025 15:09
Copy link
Contributor

Test results for "tests 1"

5 flaky ⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079:7 › cli codegen › should not throw csp directive violation errors @firefox-ubuntu-22.04-node18
⚠️ [firefox-page] › page/page-goto.spec.ts:81:3 › should work with Cross-Origin-Opener-Policy @firefox-ubuntu-22.04-node18
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @firefox-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-trace.spec.ts:397:5 › should work behind reverse proxy @macos-latest-node18-1
⚠️ [webkit-page] › page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18

46499 passed, 804 skipped
✔️✔️✔️

Merge workflow run.

@dgozman dgozman merged commit 63d898d into microsoft:main Jul 29, 2025
32 of 34 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