Skip to content

fix(screencast): await pending auto-save in page.close()#1

Open
Git-Raini wants to merge 1 commit into
sarang-code2:fix/screencast-auto-save-on-page-closefrom
Git-Raini:contrib-41744-await-autosave
Open

fix(screencast): await pending auto-save in page.close()#1
Git-Raini wants to merge 1 commit into
sarang-code2:fix/screencast-auto-save-on-page-closefrom
Git-Raini:contrib-41744-await-autosave

Conversation

@Git-Raini

Copy link
Copy Markdown

Follow-up to review comment on microsoft#41744 (comment)

The auto-save triggered from the once(Events.Page.Close, ...) listener was fire-and-forget - artifact.saveAs(savePath).catch(() => {}) with nothing awaiting it. That meant page.close() could resolve before the video was actually on disk, and a failed save produced no signal at all.

Screencast now tracks the in-flight save as _pendingAutoSave, and Page.close() awaits it in a finally block. This is safe: I traced PageDispatcher and confirmed the server dispatches the 'close' event (which triggers the auto-save) before it sends the response to the close() call itself - so _pendingAutoSave is always already assigned by the time page.close() reaches its finally block, regardless of which close path was taken (_ownedContext.close(), runBeforeUnload, or direct _channel.close()).

Diff:

--- a/packages/playwright-core/src/client/screencast.ts
+++ b/packages/playwright-core/src/client/screencast.ts
@@ private _artifact: Artifact | undefined;
+  private _pendingAutoSave: Promise<void> | undefined;
 ...
-        artifact.saveAs(savePath).catch(() => {});
+        this._pendingAutoSave = artifact.saveAs(savePath).catch(() => {});
 ...
+  async _waitForPendingAutoSave(): Promise<void> {
+    await this._pendingAutoSave;
+  }

--- a/packages/playwright-core/src/client/page.ts
+++ b/packages/playwright-core/src/client/page.ts
@@ close() { ... } catch (e) { ... }
+    } finally {
+      await this.screencast._waitForPendingAutoSave();
+    }
Test results - screencast.spec.ts, 195 runs (13 tests × 15 repeats), all passing
[182/195] [chromium-library] › tests\library\screencast.spec.ts:237:5 › start dispose stops recording
[183/195] [chromium-library] › tests\library\screencast.spec.ts:28:5 › screencast.start delivers frames via onFrame callback
[184/195] [chromium-library] › tests\library\screencast.spec.ts:57:5 › onFrame receives viewport size
[185/195] [chromium-library] › tests\library\screencast.spec.ts:108:5 › start returns a disposable that stops screencast
[186/195] [chromium-library] › tests\library\screencast.spec.ts:83:5 › start throws if screencast is already started
[187/195] [chromium-library] › tests\library\screencast.spec.ts:131:5 › start/stop twice without path creates two files in artifactsDir
[188/195] [chromium-library] › tests\library\screencast.spec.ts:94:5 › start allows restart with different options after stop
[189/195] [chromium-library] › tests\library\screencast.spec.ts:156:5 › start should work when recordVideo is set
[190/195] [chromium-library] › tests\library\screencast.spec.ts:179:5 › start should fail when another recording is in progress
[191/195] [chromium-library] › tests\library\screencast.spec.ts:186:5 › stop should not fail when no recording is in progress
[192/195] [chromium-library] › tests\library\screencast.spec.ts:194:5 › start should finish when page is closed
[193/195] [chromium-library] › tests\library\screencast.spec.ts:207:5 › should auto-save video when page closes without stop
[194/195] [chromium-library] › tests\library\screencast.spec.ts:225:5 › empty video
[195/195] [chromium-library] › tests\library\screencast.spec.ts:237:5 › start dispose stops recording
  195 passed (3.7m)
Test results - full page-close.spec.ts suite (18 tests), no regressions
[5/18] [chromium-library] › tests\library\page-close.spec.ts:65:5 › should set the page close state
[6/18] [chromium-library] › tests\library\page-close.spec.ts:71:5 › should pass page to close event
[7/18] [chromium-library] › tests\library\page-close.spec.ts:79:5 › should terminate network waiters
[8/18] [chromium-library] › tests\library\page-close.spec.ts:100:5 › should return null if parent page has been closed
[9/18] [chromium-library] › tests\library\page-close.spec.ts:92:5 › should be callable twice
[10/18] [chromium-library] › tests\library\page-close.spec.ts:110:5 › should fail with error upon disconnect
[11/18] [chromium-library] › tests\library\page-close.spec.ts:118:5 › page.close should work with window.close
[12/18] [chromium-library] › tests\library\page-close.spec.ts:124:5 › should not throw UnhandledPromiseRejection when page closes
[13/18] [chromium-library] › tests\library\page-close.spec.ts:133:5 › interrupt request.response() and request.allHeaders() on page.close
[14/18] [chromium-library] › tests\library\page-close.spec.ts:152:5 › should not treat navigations as new popups
[15/18] [chromium-library] › tests\library\page-close.spec.ts:166:5 › should not result in unhandled rejection
[16/18] [chromium-library] › tests\library\page-close.spec.ts:180:5 › should reject response.finished if page closes
[17/18] [chromium-library] › tests\library\page-close.spec.ts:200:5 › should not throw when continuing while page is closing
[18/18] [chromium-library] › tests\library\page-close.spec.ts:212:5 › should not throw when continuing after page is closed
  18 passed (3.3s)

Also confirmed tsc --noEmit and eslint clean on both changed files.

Addresses review feedback: microsoft#41744 (comment)

The auto-save triggered from the once(Events.Page.Close) listener was
fire-and-forget, so page.close() could resolve before the video was
actually written to disk - nothing guaranteed ordering, and a failed
save was silently swallowed with no signal at all.

Screencast now tracks the in-flight save as _pendingAutoSave, and
Page.close() awaits it in a finally block. This is safe because the
server dispatches the 'close' event (which triggers the auto-save)
before it sends the response to the close() call itself, so the
promise is always already assigned by the time page.close() reaches
its finally block.

Verified with 195 runs (13 tests x 15 repeats) of screencast.spec.ts,
all passing, plus the full page-close.spec.ts suite (47 tests) with no
regressions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant