From 928d14ac84e46bbdad816686236312822105efbe Mon Sep 17 00:00:00 2001 From: jrandolf <101637635+jrandolf@users.noreply.github.com> Date: Tue, 13 Feb 2024 12:35:10 +0100 Subject: [PATCH] chore: use internal event emitter for trusted events (#11898) --- packages/puppeteer-core/src/bidi/Browser.ts | 36 +++++++--- .../puppeteer-core/src/bidi/BrowserContext.ts | 36 ++++++---- packages/puppeteer-core/src/bidi/Frame.ts | 26 ++++---- .../puppeteer-core/src/bidi/HTTPRequest.ts | 2 +- .../puppeteer-core/src/bidi/HTTPResponse.ts | 2 +- packages/puppeteer-core/src/bidi/Page.ts | 15 ++++- .../src/bidi/core/Connection.ts | 3 - .../puppeteer-core/src/bidi/core/Session.ts | 15 ++--- .../src/util/decorators.test.ts | 48 +++++++++++++- .../puppeteer-core/src/util/decorators.ts | 66 +++++++++++++++++++ 10 files changed, 196 insertions(+), 53 deletions(-) diff --git a/packages/puppeteer-core/src/bidi/Browser.ts b/packages/puppeteer-core/src/bidi/Browser.ts index aaef567def4..8798d8325d9 100644 --- a/packages/puppeteer-core/src/bidi/Browser.ts +++ b/packages/puppeteer-core/src/bidi/Browser.ts @@ -8,6 +8,7 @@ import type {ChildProcess} from 'child_process'; import type * as Bidi from 'chromium-bidi/lib/cjs/protocol/protocol.js'; +import type {BrowserEvents} from '../api/Browser.js'; import { Browser, BrowserEvent, @@ -19,8 +20,10 @@ import {BrowserContextEvent} from '../api/BrowserContext.js'; import type {Page} from '../api/Page.js'; import type {Target} from '../api/Target.js'; import {UnsupportedOperation} from '../common/Errors.js'; +import {EventEmitter} from '../common/EventEmitter.js'; import {debugError} from '../common/util.js'; import type {Viewport} from '../common/Viewport.js'; +import {bubble} from '../util/decorators.js'; import {BidiBrowserContext} from './BrowserContext.js'; import type {BidiConnection} from './Connection.js'; @@ -85,6 +88,9 @@ export class BidiBrowser extends Browser { return browser; } + @bubble() + accessor #trustedEmitter = new EventEmitter(); + #process?: ChildProcess; #closeCallback?: BrowserCloseCallback; #browserCore: BrowserCore; @@ -107,7 +113,8 @@ export class BidiBrowser extends Browser { } this.#browserCore.once('disconnected', () => { - this.emit(BrowserEvent.Disconnected, undefined); + this.#trustedEmitter.emit(BrowserEvent.Disconnected, undefined); + this.#trustedEmitter.removeAllListeners(); }); this.#process?.once('close', () => { this.#browserCore.dispose('Browser process exited.', true); @@ -136,15 +143,24 @@ export class BidiBrowser extends Browser { }); this.#browserContexts.set(userContext, browserContext); - browserContext.on(BrowserContextEvent.TargetCreated, target => { - this.emit(BrowserEvent.TargetCreated, target); - }); - browserContext.on(BrowserContextEvent.TargetChanged, target => { - this.emit(BrowserEvent.TargetChanged, target); - }); - browserContext.on(BrowserContextEvent.TargetDestroyed, target => { - this.emit(BrowserEvent.TargetDestroyed, target); - }); + browserContext.trustedEmitter.on( + BrowserContextEvent.TargetCreated, + target => { + this.#trustedEmitter.emit(BrowserEvent.TargetCreated, target); + } + ); + browserContext.trustedEmitter.on( + BrowserContextEvent.TargetChanged, + target => { + this.#trustedEmitter.emit(BrowserEvent.TargetChanged, target); + } + ); + browserContext.trustedEmitter.on( + BrowserContextEvent.TargetDestroyed, + target => { + this.#trustedEmitter.emit(BrowserEvent.TargetDestroyed, target); + } + ); return browserContext; } diff --git a/packages/puppeteer-core/src/bidi/BrowserContext.ts b/packages/puppeteer-core/src/bidi/BrowserContext.ts index 769a2d5baee..42ba308d31e 100644 --- a/packages/puppeteer-core/src/bidi/BrowserContext.ts +++ b/packages/puppeteer-core/src/bidi/BrowserContext.ts @@ -6,20 +6,22 @@ import * as Bidi from 'chromium-bidi/lib/cjs/protocol/protocol.js'; +import type {BrowserContextEvents} from '../api/BrowserContext.js'; import {BrowserContext, BrowserContextEvent} from '../api/BrowserContext.js'; import {PageEvent, type Page} from '../api/Page.js'; import type {Target} from '../api/Target.js'; import {UnsupportedOperation} from '../common/Errors.js'; +import {EventEmitter} from '../common/EventEmitter.js'; import {debugError} from '../common/util.js'; import type {Viewport} from '../common/Viewport.js'; +import {bubble} from '../util/decorators.js'; import type {BidiBrowser} from './Browser.js'; import type {BrowsingContext} from './core/BrowsingContext.js'; import {UserContext} from './core/UserContext.js'; import type {BidiFrame} from './Frame.js'; import {BidiPage} from './Page.js'; -import {BidiPageTarget} from './Target.js'; -import {BidiFrameTarget} from './Target.js'; +import {BidiFrameTarget, BidiPageTarget} from './Target.js'; /** * @internal @@ -42,6 +44,9 @@ export class BidiBrowserContext extends BrowserContext { return context; } + @bubble() + accessor trustedEmitter = new EventEmitter(); + readonly #browser: BidiBrowser; readonly #defaultViewport: Viewport | null; // This is public because of cookies. @@ -52,7 +57,7 @@ export class BidiBrowserContext extends BrowserContext { [BidiPageTarget, Map] >(); - constructor( + private constructor( browser: BidiBrowser, userContext: UserContext, options: BidiBrowserContextOptions @@ -72,12 +77,15 @@ export class BidiBrowserContext extends BrowserContext { this.userContext.on('browsingcontext', ({browsingContext}) => { this.#createPage(browsingContext); }); + this.userContext.on('closed', () => { + this.trustedEmitter.removeAllListeners(); + }); } #createPage(browsingContext: BrowsingContext): BidiPage { const page = BidiPage.from(this, browsingContext); this.#pages.set(browsingContext, page); - page.on(PageEvent.Close, () => { + page.trustedEmitter.on(PageEvent.Close, () => { this.#pages.delete(browsingContext); }); @@ -85,36 +93,36 @@ export class BidiBrowserContext extends BrowserContext { const pageTarget = new BidiPageTarget(page); const frameTargets = new Map(); this.#targets.set(page, [pageTarget, frameTargets]); - page.on(PageEvent.FrameAttached, frame => { + page.trustedEmitter.on(PageEvent.FrameAttached, frame => { const bidiFrame = frame as BidiFrame; const target = new BidiFrameTarget(bidiFrame); frameTargets.set(bidiFrame, target); - this.emit(BrowserContextEvent.TargetCreated, target); + this.trustedEmitter.emit(BrowserContextEvent.TargetCreated, target); }); - page.on(PageEvent.FrameNavigated, frame => { + page.trustedEmitter.on(PageEvent.FrameNavigated, frame => { const bidiFrame = frame as BidiFrame; const target = frameTargets.get(bidiFrame); // If there is no target, then this is the page's frame. if (target === undefined) { - this.emit(BrowserContextEvent.TargetChanged, pageTarget); + this.trustedEmitter.emit(BrowserContextEvent.TargetChanged, pageTarget); } else { - this.emit(BrowserContextEvent.TargetChanged, target); + this.trustedEmitter.emit(BrowserContextEvent.TargetChanged, target); } }); - page.on(PageEvent.FrameDetached, frame => { + page.trustedEmitter.on(PageEvent.FrameDetached, frame => { const bidiFrame = frame as BidiFrame; const target = frameTargets.get(bidiFrame); if (target === undefined) { return; } frameTargets.delete(bidiFrame); - this.emit(BrowserContextEvent.TargetDestroyed, target); + this.trustedEmitter.emit(BrowserContextEvent.TargetDestroyed, target); }); - page.on(PageEvent.Close, () => { + page.trustedEmitter.on(PageEvent.Close, () => { this.#targets.delete(page); - this.emit(BrowserContextEvent.TargetDestroyed, pageTarget); + this.trustedEmitter.emit(BrowserContextEvent.TargetDestroyed, pageTarget); }); - this.emit(BrowserContextEvent.TargetCreated, pageTarget); + this.trustedEmitter.emit(BrowserContextEvent.TargetCreated, pageTarget); // -- Target stuff ends here -- return page; diff --git a/packages/puppeteer-core/src/bidi/Frame.ts b/packages/puppeteer-core/src/bidi/Frame.ts index d03625ce273..ed834514ac0 100644 --- a/packages/puppeteer-core/src/bidi/Frame.ts +++ b/packages/puppeteer-core/src/bidi/Frame.ts @@ -101,38 +101,40 @@ export class BidiFrame extends Frame { void session.detach().catch(debugError); } } - this.page().emit(PageEvent.FrameDetached, this); - this.removeAllListeners(); + this.page().trustedEmitter.emit(PageEvent.FrameDetached, this); }); this.browsingContext.on('request', ({request}) => { const httpRequest = BidiHTTPRequest.from(request, this); request.once('success', () => { // SAFETY: BidiHTTPRequest will create this before here. - this.page().emit(PageEvent.RequestFinished, httpRequest); + this.page().trustedEmitter.emit(PageEvent.RequestFinished, httpRequest); }); request.once('error', () => { - this.page().emit(PageEvent.RequestFailed, httpRequest); + this.page().trustedEmitter.emit(PageEvent.RequestFailed, httpRequest); }); }); this.browsingContext.on('navigation', ({navigation}) => { navigation.once('fragment', () => { - this.page().emit(PageEvent.FrameNavigated, this); + this.page().trustedEmitter.emit(PageEvent.FrameNavigated, this); }); }); this.browsingContext.on('load', () => { - this.page().emit(PageEvent.Load, undefined); + this.page().trustedEmitter.emit(PageEvent.Load, undefined); }); this.browsingContext.on('DOMContentLoaded', () => { this._hasStartedLoading = true; - this.page().emit(PageEvent.DOMContentLoaded, undefined); - this.page().emit(PageEvent.FrameNavigated, this); + this.page().trustedEmitter.emit(PageEvent.DOMContentLoaded, undefined); + this.page().trustedEmitter.emit(PageEvent.FrameNavigated, this); }); this.browsingContext.on('userprompt', ({userPrompt}) => { - this.page().emit(PageEvent.Dialog, BidiDialog.from(userPrompt)); + this.page().trustedEmitter.emit( + PageEvent.Dialog, + BidiDialog.from(userPrompt) + ); }); this.browsingContext.on('log', ({entry}) => { @@ -154,7 +156,7 @@ export class BidiFrame extends Frame { }, '') .slice(1); - this.page().emit( + this.page().trustedEmitter.emit( PageEvent.Console, new ConsoleMessage( entry.method as any, @@ -185,7 +187,7 @@ export class BidiFrame extends Frame { } error.stack = [...messageLines, ...stackLines].join('\n'); - this.page().emit(PageEvent.PageError, error); + this.page().trustedEmitter.emit(PageEvent.PageError, error); } else { debugError( `Unhandled LogEntry with type "${entry.type}", text "${entry.text}" and level "${entry.level}"` @@ -197,7 +199,7 @@ export class BidiFrame extends Frame { #createFrameTarget(browsingContext: BrowsingContext) { const frame = BidiFrame.from(this, browsingContext); this.#frames.set(browsingContext, frame); - this.page().emit(PageEvent.FrameAttached, frame); + this.page().trustedEmitter.emit(PageEvent.FrameAttached, frame); browsingContext.on('closed', () => { this.#frames.delete(browsingContext); diff --git a/packages/puppeteer-core/src/bidi/HTTPRequest.ts b/packages/puppeteer-core/src/bidi/HTTPRequest.ts index 5eb41f1325f..e75bb0cf3ce 100644 --- a/packages/puppeteer-core/src/bidi/HTTPRequest.ts +++ b/packages/puppeteer-core/src/bidi/HTTPRequest.ts @@ -60,7 +60,7 @@ export class BidiHTTPRequest extends HTTPRequest { this.#response = BidiHTTPResponse.from(data, this); }); - this.#frame?.page().emit(PageEvent.Request, this); + this.#frame?.page().trustedEmitter.emit(PageEvent.Request, this); } override url(): string { diff --git a/packages/puppeteer-core/src/bidi/HTTPResponse.ts b/packages/puppeteer-core/src/bidi/HTTPResponse.ts index 7ef17435391..bad44ff0895 100644 --- a/packages/puppeteer-core/src/bidi/HTTPResponse.ts +++ b/packages/puppeteer-core/src/bidi/HTTPResponse.ts @@ -40,7 +40,7 @@ export class BidiHTTPResponse extends HTTPResponse { } #initialize() { - this.#request.frame()?.page().emit(PageEvent.Response, this); + this.#request.frame()?.page().trustedEmitter.emit(PageEvent.Response, this); } @invokeAtMostOnceForArguments diff --git a/packages/puppeteer-core/src/bidi/Page.ts b/packages/puppeteer-core/src/bidi/Page.ts index 3234e1aec2f..2035620d515 100644 --- a/packages/puppeteer-core/src/bidi/Page.ts +++ b/packages/puppeteer-core/src/bidi/Page.ts @@ -12,7 +12,11 @@ import type {CDPSession} from '../api/CDPSession.js'; import type {BoundingBox} from '../api/ElementHandle.js'; import type {WaitForOptions} from '../api/Frame.js'; import type {HTTPResponse} from '../api/HTTPResponse.js'; -import type {MediaFeature, GeolocationOptions} from '../api/Page.js'; +import type { + MediaFeature, + GeolocationOptions, + PageEvents, +} from '../api/Page.js'; import { Page, PageEvent, @@ -25,11 +29,13 @@ import {EmulationManager} from '../cdp/EmulationManager.js'; import {Tracing} from '../cdp/Tracing.js'; import type {Cookie, CookieParam, CookieSameSite} from '../common/Cookie.js'; import {UnsupportedOperation} from '../common/Errors.js'; +import {EventEmitter} from '../common/EventEmitter.js'; import type {PDFOptions} from '../common/PDFOptions.js'; import type {Awaitable} from '../common/types.js'; import {evaluationString, parsePDFOptions, timeout} from '../common/util.js'; import type {Viewport} from '../common/Viewport.js'; import {assert} from '../util/assert.js'; +import {bubble} from '../util/decorators.js'; import {isErrorLike} from '../util/ErrorLike.js'; import type {BidiBrowser} from './Browser.js'; @@ -56,6 +62,9 @@ export class BidiPage extends Page { return page; } + @bubble() + accessor trustedEmitter = new EventEmitter(); + readonly #browserContext: BidiBrowserContext; readonly #frame: BidiFrame; #viewport: Viewport | null = null; @@ -91,8 +100,8 @@ export class BidiPage extends Page { #initialize() { this.#frame.browsingContext.on('closed', () => { - this.emit(PageEvent.Close, undefined); - this.removeAllListeners(); + this.trustedEmitter.emit(PageEvent.Close, undefined); + this.trustedEmitter.removeAllListeners(); }); } diff --git a/packages/puppeteer-core/src/bidi/core/Connection.ts b/packages/puppeteer-core/src/bidi/core/Connection.ts index aa4a855f1f9..0c02d468500 100644 --- a/packages/puppeteer-core/src/bidi/core/Connection.ts +++ b/packages/puppeteer-core/src/bidi/core/Connection.ts @@ -157,7 +157,4 @@ export interface Connection method: T, params: Commands[T]['params'] ): Promise<{result: Commands[T]['returnType']}>; - - // This will pipe events into the provided emitter. - pipeTo(emitter: EventEmitter): void; } diff --git a/packages/puppeteer-core/src/bidi/core/Session.ts b/packages/puppeteer-core/src/bidi/core/Session.ts index b6e28061f16..dd90f8190c2 100644 --- a/packages/puppeteer-core/src/bidi/core/Session.ts +++ b/packages/puppeteer-core/src/bidi/core/Session.ts @@ -8,7 +8,11 @@ import type * as Bidi from 'chromium-bidi/lib/cjs/protocol/protocol.js'; import {EventEmitter} from '../../common/EventEmitter.js'; import {debugError} from '../../common/util.js'; -import {inertIfDisposed, throwIfDisposed} from '../../util/decorators.js'; +import { + bubble, + inertIfDisposed, + throwIfDisposed, +} from '../../util/decorators.js'; import {DisposableStack, disposeSymbol} from '../../util/disposable.js'; import {Browser} from './Browser.js'; @@ -81,7 +85,8 @@ export class Session readonly #disposables = new DisposableStack(); readonly #info: Bidi.Session.NewResult; readonly browser!: Browser; - readonly connection: Connection; + @bubble() + accessor connection: Connection; // keep-sorted end private constructor(connection: Connection, info: Bidi.Session.NewResult) { @@ -93,8 +98,6 @@ export class Session } async #initialize(): Promise { - this.connection.pipeTo(this); - // SAFETY: We use `any` to allow assignment of the readonly property. (this as any).browser = await Browser.from(this); @@ -125,10 +128,6 @@ export class Session this[disposeSymbol](); } - pipeTo(emitter: EventEmitter): void { - this.connection.pipeTo(emitter); - } - /** * Currently, there is a 1:1 relationship between the session and the * session. In the future, we might support multiple sessions and in that diff --git a/packages/puppeteer-core/src/util/decorators.test.ts b/packages/puppeteer-core/src/util/decorators.test.ts index 4cdaf15d5b8..bc476b0153e 100644 --- a/packages/puppeteer-core/src/util/decorators.test.ts +++ b/packages/puppeteer-core/src/util/decorators.test.ts @@ -9,7 +9,9 @@ import {describe, it} from 'node:test'; import expect from 'expect'; import sinon from 'sinon'; -import {invokeAtMostOnceForArguments} from './decorators.js'; +import {EventEmitter} from '../common/EventEmitter.js'; + +import {bubble, invokeAtMostOnceForArguments} from './decorators.js'; describe('decorators', function () { describe('invokeAtMostOnceForArguments', () => { @@ -76,4 +78,48 @@ describe('decorators', function () { }).toThrow(); }); }); + + describe('bubble', () => { + it('should work', () => { + class Test extends EventEmitter { + @bubble() + accessor field = new EventEmitter(); + } + + const t = new Test(); + let a = false; + t.on('a', (value: boolean) => { + a = value; + }); + + t.field.emit('a', true); + expect(a).toBeTruthy(); + + // Set a new emitter. + t.field = new EventEmitter(); + a = false; + + t.field.emit('a', true); + expect(a).toBeTruthy(); + }); + + it('should not bubble down', () => { + class Test extends EventEmitter { + @bubble() + accessor field = new EventEmitter(); + } + + const t = new Test(); + let a = false; + t.field.on('a', (value: boolean) => { + a = value; + }); + + t.emit('a', true); + expect(a).toBeFalsy(); + + t.field.emit('a', true); + expect(a).toBeTruthy(); + }); + }); }); diff --git a/packages/puppeteer-core/src/util/decorators.ts b/packages/puppeteer-core/src/util/decorators.ts index af21c5fe296..c4dc3b6c0df 100644 --- a/packages/puppeteer-core/src/util/decorators.ts +++ b/packages/puppeteer-core/src/util/decorators.ts @@ -4,6 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type {EventType} from '../common/EventEmitter.js'; +import type {EventEmitter} from '../common/EventEmitter.js'; import type {Disposed, Moveable} from '../common/types.js'; import {asyncDisposeSymbol, disposeSymbol} from './disposable.js'; @@ -138,3 +140,67 @@ export function guarded( }; }; } + +const bubbleHandlers = new WeakMap>(); + +/** + * Event emitter fields marked with `bubble` will have their events bubble up + * the field owner. + */ +// The type is too complicated to type. +// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types +export function bubble(events?: T) { + return , Value extends EventEmitter>( + {set, get}: ClassAccessorDecoratorTarget, + context: ClassAccessorDecoratorContext + ): ClassAccessorDecoratorResult => { + context.addInitializer(function () { + const handlers = bubbleHandlers.get(this) ?? new Map(); + if (handlers.has(events)) { + return; + } + + const handler = + events !== undefined + ? (type: EventType, event: unknown) => { + if (events.includes(type)) { + this.emit(type, event); + } + } + : (type: EventType, event: unknown) => { + this.emit(type, event); + }; + + handlers.set(events, handler); + bubbleHandlers.set(this, handlers); + }); + return { + set(emitter) { + const handler = bubbleHandlers.get(this)!.get(events)!; + + // In case we are re-setting. + const oldEmitter = get.call(this); + if (oldEmitter !== undefined) { + oldEmitter.off('*', handler); + } + + if (emitter === undefined) { + return; + } + emitter.on('*', handler); + set.call(this, emitter); + }, + // @ts-expect-error -- TypeScript incorrectly types init to require a + // return. + init(emitter) { + if (emitter === undefined) { + return; + } + const handler = bubbleHandlers.get(this)!.get(events)!; + + emitter.on('*', handler); + return emitter; + }, + }; + }; +}