Skip to content

Commit ff487bf

Browse files
fix(frames): preserve request cancellation after rebase
#43650 (review): Rebased on 1d5572d; resolved the conflict by keeping detached-request cleanup on frame removal rather than page disposal, preserving page-close request errors. #43650 (comment): Already addressed; retained frame detachment and context invalidation on page disposal. #43650 (review): Retained original close errors; EX-243 and PA-051 pass across all three browsers after the rebase correction. #43650 (comment): Already addressed; retained the pending iframe evaluation and required target-closed error.
1 parent 1d5572d commit ff487bf

3 files changed

Lines changed: 20 additions & 14 deletions

File tree

‎packages/playwright-core/src/server/frames.ts‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ import type * as channels from './channels';
5050
import type { AriaSnapshotJSON } from '@isomorphic/ariaSnapshot';
5151

5252
type ContextData = {
53-
contextPromise: ManualPromise<dom.FrameExecutionContext | { destroyedReason: string }>;
53+
contextPromise: ManualPromise<dom.FrameExecutionContext | { destroyedError: Error }>;
5454
context: dom.FrameExecutionContext | null;
5555
};
5656

@@ -141,7 +141,7 @@ export class FrameManager {
141141

142142
dispose(error: Error) {
143143
for (const frame of this._frames.values()) {
144-
frame._stopNetworkIdleTimer();
144+
frame._onDetached(error);
145145
frame.invalidateNonStallingEvaluations(error);
146146
}
147147
}
@@ -378,6 +378,7 @@ export class FrameManager {
378378
private _removeFramesRecursively(frame: Frame) {
379379
this.removeChildFramesRecursively(frame);
380380
frame._onDetached();
381+
frame._failInflightRequests(new Set());
381382
this._frames.delete(frame._id);
382383
if (!this._page.isClosed())
383384
this._page.emit(Page.Events.FrameDetached, frame);
@@ -787,10 +788,10 @@ export class Frame extends SdkObject<FrameEventMap> {
787788
context(world: types.World): Promise<dom.FrameExecutionContext> {
788789
if (this._page.delegate.noUtilityWorld?.())
789790
world = 'main';
790-
return this._contextData.get(world)!.contextPromise.then(contextOrDestroyedReason => {
791-
if (contextOrDestroyedReason instanceof js.ExecutionContext)
792-
return contextOrDestroyedReason;
793-
throw new Error(contextOrDestroyedReason.destroyedReason);
791+
return this._contextData.get(world)!.contextPromise.then(contextOrDestroyedError => {
792+
if (contextOrDestroyedError instanceof js.ExecutionContext)
793+
return contextOrDestroyedError;
794+
throw contextOrDestroyedError.destroyedError;
794795
});
795796
}
796797

@@ -1743,14 +1744,13 @@ export class Frame extends SdkObject<FrameEventMap> {
17431744
progress.setAllowConcurrentOrNestedRaces(false);
17441745
}
17451746

1746-
_onDetached() {
1747+
_onDetached(error = new Error('Frame was detached')) {
17471748
this._stopNetworkIdleTimer();
1748-
this._detachedScope.close(new Error('Frame was detached'));
1749-
this._failInflightRequests(new Set());
1749+
this._detachedScope.close(error);
17501750
for (const data of this._contextData.values()) {
17511751
if (data.context)
1752-
data.context.contextDestroyed('Frame was detached');
1753-
data.contextPromise.resolve({ destroyedReason: 'Frame was detached' });
1752+
data.context.contextDestroyed(error);
1753+
data.contextPromise.resolve({ destroyedError: error });
17541754
}
17551755
if (this._parentFrame)
17561756
this._parentFrame._childFrames.delete(this);

‎packages/playwright-core/src/server/javascript.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,8 @@ export class ExecutionContext extends SdkObject {
6969
this.delegate = delegate;
7070
}
7171

72-
contextDestroyed(reason: string) {
73-
this._contextDestroyedScope.close(new Error(reason));
72+
contextDestroyed(reason: string | Error) {
73+
this._contextDestroyedScope.close(typeof reason === 'string' ? new Error(reason) : reason);
7474
}
7575

7676
async raceAgainstContextDestroyed<T>(promise: Promise<T>): Promise<T> {

‎tests/library/page-close.spec.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,19 @@ test('PA-041 addLocatorHandler should throw when page closes', async ({ page, se
5353
expect(error.message).toContain('custom reason');
5454
});
5555

56-
test('PA-042 should reject all promises when page is closed', async ({ page }) => {
56+
test('PA-042 should reject all promises when page is closed', { annotation: { type: 'issue', description: 'https://github.com/microsoft/playwright/issues/43647' } }, async ({ page, server }) => {
57+
server.setRoute('/hang', () => {});
58+
const requestPromise = server.waitForRequest('/hang');
59+
await page.setContent(`<iframe src="${server.PREFIX}/hang"></iframe>`, { waitUntil: 'domcontentloaded' });
60+
await requestPromise;
61+
const frameErrorPromise = page.frames()[1].evaluate(() => new Promise<void>(() => {})).catch(e => e);
5762
let error = null;
5863
await Promise.all([
5964
page.evaluate(() => new Promise(r => {})).catch(e => error = e),
6065
page.close(),
6166
]);
6267
expect(error.message).toContain(kTargetClosedErrorMessage);
68+
expect((await frameErrorPromise).message).toContain(kTargetClosedErrorMessage);
6369
});
6470

6571
test('PA-043 should set the page close state', async ({ page }) => {

0 commit comments

Comments
 (0)