Skip to content

Commit

Permalink
chore: do not leak internal page handles after closing page (microsof…
Browse files Browse the repository at this point in the history 
…t#24169)

Partial fix for microsoft#6319

After this fix, the following scenario won't leak and the context state
(cookies, storage, etc) can be reused by the new page sessions:

```js
  for (let i = 0; i < 1000; ++i) {
    const page = await context.newPage();
    await page.goto('...');
    await page.close('...');
  }
```
  • Loading branch information
@pavelfeldman
pavelfeldman committed Jul 12, 2023
1 parent ea1ec11 commit 53bf199
Show file tree
Hide file tree
Showing 12 changed files with 148 additions and 42 deletions.
35 changes: 35 additions & 0 deletions package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
"eslint-plugin-notice": "^0.9.10",
"eslint-plugin-react-hooks": "^4.3.0",
"formidable": "^2.1.1",
"heapdump": "^0.3.15",
"license-checker": "^25.0.1",
"mime": "^3.0.0",
"ncp": "^2.0.0",
Expand Down
2 changes: 2 additions & 0 deletions packages/playwright-core/src/server/dispatchers/dispatcher.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeT
}

adopt(child: DispatcherScope) {
if (child._parent === this)
return;
const oldParent = child._parent!;
oldParent._dispatchers.delete(child._guid);
this._dispatchers.set(child._guid, child);
Expand Down
23 changes: 20 additions & 3 deletions packages/playwright-core/src/server/dispatchers/elementHandlerDispatcher.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ import type { CallMetadata } from '../instrumentation';
import type { WritableStreamDispatcher } from './writableStreamDispatcher';
import { assert } from '../../utils';
import path from 'path';
import type { PageDispatcher } from './pageDispatcher';
import { BrowserContextDispatcher } from './browserContextDispatcher';
import { PageDispatcher, WorkerDispatcher } from './pageDispatcher';

export class ElementHandleDispatcher extends JSHandleDispatcher implements channels.ElementHandleChannel {
_type_ElementHandle = true;
Expand Down Expand Up @@ -57,12 +58,12 @@ export class ElementHandleDispatcher extends JSHandleDispatcher implements chann

async ownerFrame(params: channels.ElementHandleOwnerFrameParams, metadata: CallMetadata): Promise<channels.ElementHandleOwnerFrameResult> {
const frame = await this._elementHandle.ownerFrame();
return { frame: frame ? FrameDispatcher.from(this.parentScope() as PageDispatcher, frame) : undefined };
return { frame: frame ? FrameDispatcher.from(this._browserContextDispatcher(), frame) : undefined };
}

async contentFrame(params: channels.ElementHandleContentFrameParams, metadata: CallMetadata): Promise<channels.ElementHandleContentFrameResult> {
const frame = await this._elementHandle.contentFrame();
return { frame: frame ? FrameDispatcher.from(this.parentScope() as PageDispatcher, frame) : undefined };
return { frame: frame ? FrameDispatcher.from(this._browserContextDispatcher(), frame) : undefined };
}

async getAttribute(params: channels.ElementHandleGetAttributeParams, metadata: CallMetadata): Promise<channels.ElementHandleGetAttributeResult> {
Expand Down Expand Up @@ -223,4 +224,20 @@ export class ElementHandleDispatcher extends JSHandleDispatcher implements chann
async waitForSelector(params: channels.ElementHandleWaitForSelectorParams, metadata: CallMetadata): Promise<channels.ElementHandleWaitForSelectorResult> {
return { element: ElementHandleDispatcher.fromNullable(this.parentScope(), await this._elementHandle.waitForSelector(metadata, params.selector, params)) };
}

private _browserContextDispatcher(): BrowserContextDispatcher {
const scope = this.parentScope();
if (scope instanceof BrowserContextDispatcher)
return scope;
if (scope instanceof PageDispatcher)
return scope.parentScope();
if ((scope instanceof WorkerDispatcher) || (scope instanceof FrameDispatcher)) {
const parentScope = scope.parentScope();
if (parentScope instanceof BrowserContextDispatcher)
return parentScope;
return parentScope.parentScope();
}
throw new Error('ElementHandle belongs to unexpected scope');
}

}
34 changes: 19 additions & 15 deletions packages/playwright-core/src/server/dispatchers/frameDispatcher.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,30 +26,34 @@ import type { CallMetadata } from '../instrumentation';
import type { WritableStreamDispatcher } from './writableStreamDispatcher';
import { assert } from '../../utils';
import path from 'path';
import type { BrowserContextDispatcher } from './browserContextDispatcher';
import type { PageDispatcher } from './pageDispatcher';

export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, PageDispatcher> implements channels.FrameChannel {
export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, BrowserContextDispatcher | PageDispatcher> implements channels.FrameChannel {
_type_Frame = true;
private _frame: Frame;
private _browserContextDispatcher: BrowserContextDispatcher;

static from(scope: PageDispatcher, frame: Frame): FrameDispatcher {
static from(scope: BrowserContextDispatcher, frame: Frame): FrameDispatcher {
const result = existingDispatcher<FrameDispatcher>(frame);
return result || new FrameDispatcher(scope, frame);
}

static fromNullable(scope: PageDispatcher, frame: Frame | null): FrameDispatcher | undefined {
static fromNullable(scope: BrowserContextDispatcher, frame: Frame | null): FrameDispatcher | undefined {
if (!frame)
return;
return FrameDispatcher.from(scope, frame);
}

private constructor(scope: PageDispatcher, frame: Frame) {
super(scope, frame, 'Frame', {
private constructor(scope: BrowserContextDispatcher, frame: Frame) {
const pageDispatcher = existingDispatcher<PageDispatcher>(frame._page);
super(pageDispatcher || scope, frame, 'Frame', {
url: frame.url(),
name: frame.name(),
parentFrame: FrameDispatcher.fromNullable(scope, frame.parentFrame()),
loadStates: Array.from(frame._firedLifecycleEvents),
});
this._browserContextDispatcher = scope;
this._frame = frame;
this.addObjectListener(Frame.Events.AddLifecycle, lifecycleEvent => {
this._dispatchEvent('loadstate', { add: lifecycleEvent });
Expand All @@ -62,29 +66,29 @@ export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, Pa
return;
const params = { url: event.url, name: event.name, error: event.error ? event.error.message : undefined };
if (event.newDocument)
(params as any).newDocument = { request: RequestDispatcher.fromNullable(this.parentScope().parentScope(), event.newDocument.request || null) };
(params as any).newDocument = { request: RequestDispatcher.fromNullable(this._browserContextDispatcher, event.newDocument.request || null) };
this._dispatchEvent('navigated', params);
});
}

async goto(params: channels.FrameGotoParams, metadata: CallMetadata): Promise<channels.FrameGotoResult> {
return { response: ResponseDispatcher.fromNullable(this.parentScope().parentScope(), await this._frame.goto(metadata, params.url, params)) };
return { response: ResponseDispatcher.fromNullable(this._browserContextDispatcher, await this._frame.goto(metadata, params.url, params)) };
}

async frameElement(): Promise<channels.FrameFrameElementResult> {
return { element: ElementHandleDispatcher.from(this.parentScope(), await this._frame.frameElement()) };
return { element: ElementHandleDispatcher.from(this, await this._frame.frameElement()) };
}

async evaluateExpression(params: channels.FrameEvaluateExpressionParams, metadata: CallMetadata): Promise<channels.FrameEvaluateExpressionResult> {
return { value: serializeResult(await this._frame.evaluateExpression(params.expression, { isFunction: params.isFunction, exposeUtilityScript: params.exposeUtilityScript }, parseArgument(params.arg))) };
}

async evaluateExpressionHandle(params: channels.FrameEvaluateExpressionHandleParams, metadata: CallMetadata): Promise<channels.FrameEvaluateExpressionHandleResult> {
return { handle: ElementHandleDispatcher.fromJSHandle(this.parentScope(), await this._frame.evaluateExpressionHandle(params.expression, { isFunction: params.isFunction }, parseArgument(params.arg))) };
return { handle: ElementHandleDispatcher.fromJSHandle(this, await this._frame.evaluateExpressionHandle(params.expression, { isFunction: params.isFunction }, parseArgument(params.arg))) };
}

async waitForSelector(params: channels.FrameWaitForSelectorParams, metadata: CallMetadata): Promise<channels.FrameWaitForSelectorResult> {
return { element: ElementHandleDispatcher.fromNullable(this.parentScope(), await this._frame.waitForSelector(metadata, params.selector, params)) };
return { element: ElementHandleDispatcher.fromNullable(this, await this._frame.waitForSelector(metadata, params.selector, params)) };
}

async dispatchEvent(params: channels.FrameDispatchEventParams, metadata: CallMetadata): Promise<void> {
Expand All @@ -100,12 +104,12 @@ export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, Pa
}

async querySelector(params: channels.FrameQuerySelectorParams, metadata: CallMetadata): Promise<channels.FrameQuerySelectorResult> {
return { element: ElementHandleDispatcher.fromNullable(this.parentScope(), await this._frame.querySelector(params.selector, params)) };
return { element: ElementHandleDispatcher.fromNullable(this, await this._frame.querySelector(params.selector, params)) };
}

async querySelectorAll(params: channels.FrameQuerySelectorAllParams, metadata: CallMetadata): Promise<channels.FrameQuerySelectorAllResult> {
const elements = await this._frame.querySelectorAll(params.selector);
return { elements: elements.map(e => ElementHandleDispatcher.from(this.parentScope(), e)) };
return { elements: elements.map(e => ElementHandleDispatcher.from(this, e)) };
}

async queryCount(params: channels.FrameQueryCountParams): Promise<channels.FrameQueryCountResult> {
Expand All @@ -121,11 +125,11 @@ export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, Pa
}

async addScriptTag(params: channels.FrameAddScriptTagParams, metadata: CallMetadata): Promise<channels.FrameAddScriptTagResult> {
return { element: ElementHandleDispatcher.from(this.parentScope(), await this._frame.addScriptTag(params)) };
return { element: ElementHandleDispatcher.from(this, await this._frame.addScriptTag(params)) };
}

async addStyleTag(params: channels.FrameAddStyleTagParams, metadata: CallMetadata): Promise<channels.FrameAddStyleTagResult> {
return { element: ElementHandleDispatcher.from(this.parentScope(), await this._frame.addStyleTag(params)) };
return { element: ElementHandleDispatcher.from(this, await this._frame.addStyleTag(params)) };
}

async click(params: channels.FrameClickParams, metadata: CallMetadata): Promise<void> {
Expand Down Expand Up @@ -249,7 +253,7 @@ export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, Pa
}

async waitForFunction(params: channels.FrameWaitForFunctionParams, metadata: CallMetadata): Promise<channels.FrameWaitForFunctionResult> {
return { handle: ElementHandleDispatcher.fromJSHandle(this.parentScope(), await this._frame._waitForFunctionExpression(metadata, params.expression, params.isFunction, parseArgument(params.arg), params)) };
return { handle: ElementHandleDispatcher.fromJSHandle(this, await this._frame._waitForFunctionExpression(metadata, params.expression, params.isFunction, parseArgument(params.arg), params)) };
}

async title(params: channels.FrameTitleParams, metadata: CallMetadata): Promise<channels.FrameTitleResult> {
Expand Down
3 changes: 2 additions & 1 deletion packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ import { ElementHandleDispatcher } from './elementHandlerDispatcher';
import { parseSerializedValue, serializeValue } from '../../protocol/serializers';
import type { PageDispatcher, WorkerDispatcher } from './pageDispatcher';
import type { ElectronApplicationDispatcher } from './electronDispatcher';
import type { FrameDispatcher } from './frameDispatcher';

export type JSHandleDispatcherParentScope = PageDispatcher | WorkerDispatcher | ElectronApplicationDispatcher;
export type JSHandleDispatcherParentScope = PageDispatcher | FrameDispatcher | WorkerDispatcher | ElectronApplicationDispatcher;

export class JSHandleDispatcher extends Dispatcher<js.JSHandle, channels.JSHandleChannel, JSHandleDispatcherParentScope> implements channels.JSHandleChannel {
_type_JSHandle = true;
Expand Down
24 changes: 16 additions & 8 deletions packages/playwright-core/src/server/dispatchers/networkDispatchers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ import type { PageDispatcher } from './pageDispatcher';
import { FrameDispatcher } from './frameDispatcher';
import { WorkerDispatcher } from './pageDispatcher';

export class RequestDispatcher extends Dispatcher<Request, channels.RequestChannel, BrowserContextDispatcher> implements channels.RequestChannel {
export class RequestDispatcher extends Dispatcher<Request, channels.RequestChannel, BrowserContextDispatcher | PageDispatcher | FrameDispatcher> implements channels.RequestChannel {
_type_Request: boolean;
private _browserContextDispatcher: BrowserContextDispatcher;

static from(scope: BrowserContextDispatcher, request: Request): RequestDispatcher {
const result = existingDispatcher<RequestDispatcher>(request);
Expand All @@ -41,8 +42,13 @@ export class RequestDispatcher extends Dispatcher<Request, channels.RequestChann

private constructor(scope: BrowserContextDispatcher, request: Request) {
const postData = request.postDataBuffer();
super(scope, request, 'Request', {
frame: FrameDispatcher.fromNullable(scope as any as PageDispatcher, request.frame()),
// Always try to attach request to the page, if not, frame.
const frame = request.frame();
const page = request.frame()?._page;
const pageDispatcher = page ? existingDispatcher<PageDispatcher>(page) : null;
const frameDispatcher = frame ? FrameDispatcher.from(scope, frame) : null;
super(pageDispatcher || frameDispatcher || scope, request, 'Request', {
frame: FrameDispatcher.fromNullable(scope, request.frame()),
serviceWorker: WorkerDispatcher.fromNullable(scope, request.serviceWorker()),
url: request.url(),
resourceType: request.resourceType(),
Expand All @@ -53,33 +59,35 @@ export class RequestDispatcher extends Dispatcher<Request, channels.RequestChann
redirectedFrom: RequestDispatcher.fromNullable(scope, request.redirectedFrom()),
});
this._type_Request = true;
this._browserContextDispatcher = scope;
}

async rawRequestHeaders(params?: channels.RequestRawRequestHeadersParams): Promise<channels.RequestRawRequestHeadersResult> {
return { headers: await this._object.rawRequestHeaders() };
}

async response(): Promise<channels.RequestResponseResult> {
return { response: ResponseDispatcher.fromNullable(this.parentScope(), await this._object.response()) };
return { response: ResponseDispatcher.fromNullable(this._browserContextDispatcher, await this._object.response()) };
}
}

export class ResponseDispatcher extends Dispatcher<Response, channels.ResponseChannel, BrowserContextDispatcher> implements channels.ResponseChannel {
export class ResponseDispatcher extends Dispatcher<Response, channels.ResponseChannel, RequestDispatcher> implements channels.ResponseChannel {
_type_Response = true;

static from(scope: BrowserContextDispatcher, response: Response): ResponseDispatcher {
const result = existingDispatcher<ResponseDispatcher>(response);
return result || new ResponseDispatcher(scope, response);
const requestDispatcher = RequestDispatcher.from(scope, response.request());
return result || new ResponseDispatcher(requestDispatcher, response);
}

static fromNullable(scope: BrowserContextDispatcher, response: Response | null): ResponseDispatcher | undefined {
return response ? ResponseDispatcher.from(scope, response) : undefined;
}

private constructor(scope: BrowserContextDispatcher, response: Response) {
private constructor(scope: RequestDispatcher, response: Response) {
super(scope, response, 'Response', {
// TODO: responses in popups can point to non-reported requests.
request: RequestDispatcher.from(scope, response.request()),
request: scope,
url: response.url(),
status: response.status(),
statusText: response.statusText(),
Expand Down
10 changes: 5 additions & 5 deletions packages/playwright-core/src/server/dispatchers/pageDispatcher.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageChannel, Brows
// If we split pageCreated and pageReady, there should be no main frame during pageCreated.

// We will reparent it to the page below using adopt.
const mainFrame = FrameDispatcher.from(parentScope as any as PageDispatcher, page.mainFrame());
const mainFrame = FrameDispatcher.from(parentScope, page.mainFrame());

super(parentScope, page, 'Page', {
mainFrame,
Expand All @@ -80,7 +80,7 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageChannel, Brows
this._dispatchEvent('download', { url: download.url, suggestedFilename: download.suggestedFilename(), artifact: ArtifactDispatcher.from(parentScope, download.artifact) });
});
this.addObjectListener(Page.Events.FileChooser, (fileChooser: FileChooser) => this._dispatchEvent('fileChooser', {
element: ElementHandleDispatcher.from(this, fileChooser.element()),
element: ElementHandleDispatcher.from(mainFrame, fileChooser.element()),
isMultiple: fileChooser.isMultiple()
}));
this.addObjectListener(Page.Events.FrameAttached, frame => this._onFrameAttached(frame));
Expand Down Expand Up @@ -297,11 +297,11 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageChannel, Brows
}

_onFrameAttached(frame: Frame) {
this._dispatchEvent('frameAttached', { frame: FrameDispatcher.from(this, frame) });
this._dispatchEvent('frameAttached', { frame: FrameDispatcher.from(this.parentScope(), frame) });
}

_onFrameDetached(frame: Frame) {
this._dispatchEvent('frameDetached', { frame: FrameDispatcher.from(this, frame) });
this._dispatchEvent('frameDetached', { frame: FrameDispatcher.from(this.parentScope(), frame) });
}

override _onDispose() {
Expand Down Expand Up @@ -346,7 +346,7 @@ export class BindingCallDispatcher extends Dispatcher<{ guid: string }, channels

constructor(scope: PageDispatcher, name: string, needsHandle: boolean, source: { context: BrowserContext, page: Page, frame: Frame }, args: any[]) {
super(scope, { guid: 'bindingCall@' + createGuid() }, 'BindingCall', {
frame: FrameDispatcher.from(scope, source.frame),
frame: FrameDispatcher.from(scope.parentScope(), source.frame),
name,
args: needsHandle ? undefined : args.map(serializeResult),
handle: needsHandle ? ElementHandleDispatcher.fromJSHandle(scope, args[0] as JSHandle) : undefined,
Expand Down
Loading

0 comments on commit 53bf199

Please sign in to comment.