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

feat(scopes): make page a scope #4300

Merged
merged 1 commit into from
Nov 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 8 additions & 12 deletions src/dispatchers/dispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export class Dispatcher<Type, Initializer> extends EventEmitter implements chann

(object as any)[dispatcherSymbol] = this;
if (this._parent)
this._connection.sendMessageToClient(this._parent._guid, '__create__', { type, initializer, guid }, !!isScope);
this._connection.sendMessageToClient(this._parent._guid, '__create__', { type, initializer, guid });
}

_dispatchEvent(method: string, params: Dispatcher<any, any> | any = {}) {
Expand Down Expand Up @@ -126,9 +126,8 @@ export class DispatcherConnection {
private _validateParams: (type: string, method: string, params: any) => any;
private _validateMetadata: (metadata: any) => any;

sendMessageToClient(guid: string, method: string, params: any, disallowDispatchers?: boolean) {
const allowDispatchers = !disallowDispatchers;
this.onmessage({ guid, method, params: this._replaceDispatchersWithGuids(params, allowDispatchers) });
sendMessageToClient(guid: string, method: string, params: any) {
this.onmessage({ guid, method, params: this._replaceDispatchersWithGuids(params) });
}

constructor() {
Expand Down Expand Up @@ -178,26 +177,23 @@ export class DispatcherConnection {
try {
const validated = this._validateParams(dispatcher._type, method, params);
const result = await (dispatcher as any)[method](validated, this._validateMetadata(metadata));
this.onmessage({ id, result: this._replaceDispatchersWithGuids(result, true) });
this.onmessage({ id, result: this._replaceDispatchersWithGuids(result) });
} catch (e) {
this.onmessage({ id, error: serializeError(e) });
}
}

private _replaceDispatchersWithGuids(payload: any, allowDispatchers: boolean): any {
private _replaceDispatchersWithGuids(payload: any): any {
if (!payload)
return payload;
if (payload instanceof Dispatcher) {
if (!allowDispatchers)
throw new Error(`Channels are not allowed in the scope's initialzier`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This particular check was a result of the following problem:

BrowserContext had pages in the initializer, and since we send channels from the initializer before the object, these pages were created in the parent scope (the Browser), rather than BrowserContext's scope. This lead to leaks where pages were not cleaned up upon BrowserContext's dispose. I am afraid this could happen again.

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like an edge case, let's add a test instead?

if (payload instanceof Dispatcher)
return { guid: payload._guid };
}
if (Array.isArray(payload))
return payload.map(p => this._replaceDispatchersWithGuids(p, allowDispatchers));
return payload.map(p => this._replaceDispatchersWithGuids(p));
if (typeof payload === 'object') {
const result: any = {};
for (const key of Object.keys(payload))
result[key] = this._replaceDispatchersWithGuids(payload[key], allowDispatchers);
result[key] = this._replaceDispatchersWithGuids(payload[key]);
return result;
}
return payload;
Expand Down
9 changes: 6 additions & 3 deletions src/dispatchers/pageDispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,17 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageInitializer> i
videoRelativePath: page._video ? page._video._relativePath : undefined,
viewportSize: page.viewportSize() || undefined,
isClosed: page.isClosed()
});
}, true);
this._page = page;
page.on(Page.Events.Close, () => this._dispatchEvent('close'));
page.on(Page.Events.Close, () => {
this._dispatchEvent('close');
this._dispose();
});
page.on(Page.Events.Console, message => this._dispatchEvent('console', { message: new ConsoleMessageDispatcher(this._scope, message) }));
page.on(Page.Events.Crash, () => this._dispatchEvent('crash'));
page.on(Page.Events.DOMContentLoaded, () => this._dispatchEvent('domcontentloaded'));
page.on(Page.Events.Dialog, dialog => this._dispatchEvent('dialog', { dialog: new DialogDispatcher(this._scope, dialog) }));
page.on(Page.Events.Download, download => this._dispatchEvent('download', { download: new DownloadDispatcher(this._scope, download) }));
page.on(Page.Events.Download, download => this._dispatchEvent('download', { download: new DownloadDispatcher(scope, download) }));
this._page.on(Page.Events.FileChooser, (fileChooser: FileChooser) => this._dispatchEvent('fileChooser', {
element: new ElementHandleDispatcher(this._scope, fileChooser.element()),
isMultiple: fileChooser.isMultiple()
Expand Down
3 changes: 2 additions & 1 deletion src/server/frames.ts
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,8 @@ export class FrameManager {
this.removeChildFramesRecursively(frame);
frame._onDetached();
this._frames.delete(frame._id);
this._page.emit(Page.Events.FrameDetached, frame);
if (!this._page.isClosed())
this._page.emit(Page.Events.FrameDetached, frame);
}

private _inflightRequestFinished(request: network.Request) {
Expand Down
2 changes: 1 addition & 1 deletion src/utils/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class CustomError extends Error {
export class TimeoutError extends CustomError {}

export const kBrowserClosedError = 'Browser has been closed';
export const kBrowserOrContextClosedError = 'Target browser or context has been closed';
export const kBrowserOrContextClosedError = 'Target page, context or browser has been closed';

export function isSafeCloseError(error: Error) {
return error.message.endsWith(kBrowserClosedError) || error.message.endsWith(kBrowserOrContextClosedError);
Expand Down
7 changes: 4 additions & 3 deletions test/channels.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,10 @@ it('should scope context handles', async ({browser, server}) => {
{ _guid: 'Browser', objects: [
{ _guid: 'BrowserContext', objects: [
{ _guid: 'Frame', objects: [] },
{ _guid: 'Page', objects: [] },
{ _guid: 'Request', objects: [] },
{ _guid: 'Response', objects: [] },
{ _guid: 'Page', objects: [
{ _guid: 'Request', objects: [] },
{ _guid: 'Response', objects: [] },
]},
]},
] },
] },
Expand Down
2 changes: 1 addition & 1 deletion test/chromium/session.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe('session', (suite, { browserName }) => {
} catch (e) {
error = e;
}
expect(error.message).toContain('Target browser or context has been closed');
expect(error.message).toContain('Target page, context or browser has been closed');
});

it('should throw nice errors', async function({page}) {
Expand Down
12 changes: 11 additions & 1 deletion test/page-close.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* limitations under the License.
*/

import { it } from './fixtures';
import { it, expect } from './fixtures';

it('should close page with active dialog', (test, { browserName, platform }) => {
test.fixme(browserName === 'webkit' && platform === 'darwin', 'WebKit hangs on a Mac');
Expand Down Expand Up @@ -43,3 +43,13 @@ it('should access page after beforeunload', (test, { browserName }) => {
await dialog.dismiss();
await page.evaluate(() => document.title);
});

it('should not accept after close', (test, { browserName, platform }) => {
test.fixme(browserName === 'webkit' && platform === 'darwin', 'WebKit hangs on a Mac');
}, async ({page}) => {
page.evaluate(() => alert()).catch(() => {});
const dialog = await page.waitForEvent('dialog');
await page.close();
const e = await dialog.dismiss().catch(e => e);
expect(e.message).toContain('Target page, context or browser has been closed');
});