From aa0bf66ce0c2b1ac4e3d4de3319005b5addddebe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Wed, 7 Dec 2022 16:16:49 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20[RUMF-1449]=20implement=20a=20wo?= =?UTF-8?q?rkaround=20for=20Firefox=20memory=20leak?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../core/src/browser/addEventListener.spec.ts | 38 +++++++++++++++++++ packages/core/src/browser/addEventListener.ts | 36 +++++++++++++++++- packages/core/src/index.ts | 1 + .../src/tools/getZoneJsOriginalValue.spec.ts | 32 ++++++++++++++++ .../core/src/tools/getZoneJsOriginalValue.ts | 28 ++++++++++++++ packages/core/test/specHelper.ts | 29 ++++++++++++++ packages/core/test/stubReportApis.ts | 2 +- .../src/browser/domMutationObservable.spec.ts | 27 +++++-------- .../src/browser/domMutationObservable.ts | 21 ++++------ 9 files changed, 181 insertions(+), 33 deletions(-) create mode 100644 packages/core/src/browser/addEventListener.spec.ts create mode 100644 packages/core/src/tools/getZoneJsOriginalValue.spec.ts create mode 100644 packages/core/src/tools/getZoneJsOriginalValue.ts diff --git a/packages/core/src/browser/addEventListener.spec.ts b/packages/core/src/browser/addEventListener.spec.ts new file mode 100644 index 0000000000..342ea23657 --- /dev/null +++ b/packages/core/src/browser/addEventListener.spec.ts @@ -0,0 +1,38 @@ +import { stubZoneJs } from '../../test/specHelper' +import { noop } from '../tools/utils' + +import { addEventListener, DOM_EVENT, clearGetEventTargetOriginMethodsCache } from './addEventListener' + +describe('addEventListener', () => { + describe('Zone.js support', () => { + let zoneJsStub: ReturnType + + beforeEach(() => { + zoneJsStub = stubZoneJs() + }) + + afterEach(() => { + zoneJsStub.restore() + clearGetEventTargetOriginMethodsCache() + }) + + it('uses the original addEventListener method instead of the method patched by Zone.js', () => { + const zoneJsPatchedAddEventListener = jasmine.createSpy() + zoneJsStub.replaceProperty(EventTarget.prototype, 'addEventListener', zoneJsPatchedAddEventListener) + + const eventTarget = document.createElement('div') + addEventListener(eventTarget, DOM_EVENT.CLICK, noop) + expect(zoneJsPatchedAddEventListener).not.toHaveBeenCalled() + }) + + it('uses the original removeEventListener method instead of the method patched by Zone.js', () => { + const zoneJsPatchedRemoveEventListener = jasmine.createSpy() + zoneJsStub.replaceProperty(EventTarget.prototype, 'removeEventListener', zoneJsPatchedRemoveEventListener) + + const eventTarget = document.createElement('div') + const { stop } = addEventListener(eventTarget, DOM_EVENT.CLICK, noop) + stop() + expect(zoneJsPatchedRemoveEventListener).not.toHaveBeenCalled() + }) + }) +}) diff --git a/packages/core/src/browser/addEventListener.ts b/packages/core/src/browser/addEventListener.ts index 46336b03ef..3ab4d3e124 100644 --- a/packages/core/src/browser/addEventListener.ts +++ b/packages/core/src/browser/addEventListener.ts @@ -1,4 +1,5 @@ import { monitor } from '@datadog/browser-core' +import { getZoneJsOriginalValue } from '../tools/getZoneJsOriginalValue' export const enum DOM_EVENT { BEFORE_UNLOAD = 'beforeunload', @@ -86,10 +87,41 @@ export function addEventListeners( ) const options = passive ? { capture, passive } : capture - events.forEach((event) => eventTarget.addEventListener(event, wrappedListener, options)) - const stop = () => events.forEach((event) => eventTarget.removeEventListener(event, wrappedListener, options)) + const { addEventListener, removeEventListener } = getEventTargetOriginalMethods() + events.forEach((event) => addEventListener.call(eventTarget, event, wrappedListener, options)) + const stop = () => events.forEach((event) => removeEventListener.call(eventTarget, event, wrappedListener, options)) return { stop, } } + +let getEventTargetOriginalMethodsCache: Pick | undefined + +/** + * Get the original 'addEventListener' and 'removeEventListener' methods in case Zone.js did patch + * them. + * + * This is a workaround for a memory leak in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1804409 + */ +export function getEventTargetOriginalMethods(): { + addEventListener: EventTarget['addEventListener'] + removeEventListener: EventTarget['removeEventListener'] +} { + if (!getEventTargetOriginalMethodsCache) { + getEventTargetOriginalMethodsCache = { + addEventListener: + // eslint-disable-next-line @typescript-eslint/unbound-method + getZoneJsOriginalValue(EventTarget.prototype, 'addEventListener') || EventTarget.prototype.addEventListener, + removeEventListener: + getZoneJsOriginalValue(EventTarget.prototype, 'removeEventListener') || + // eslint-disable-next-line @typescript-eslint/unbound-method + EventTarget.prototype.removeEventListener, + } + } + return getEventTargetOriginalMethodsCache +} + +export function clearGetEventTargetOriginMethodsCache() { + getEventTargetOriginalMethodsCache = undefined +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index ca81888514..8dfa89631a 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -57,6 +57,7 @@ export * from './tools/timeUtils' export * from './tools/utils' export * from './tools/createEventRateLimiter' export * from './tools/browserDetection' +export { getZoneJsOriginalValue } from './tools/getZoneJsOriginalValue' export { instrumentMethod, instrumentMethodAndCallOriginal, instrumentSetter } from './tools/instrumentMethod' export { ErrorSource, diff --git a/packages/core/src/tools/getZoneJsOriginalValue.spec.ts b/packages/core/src/tools/getZoneJsOriginalValue.spec.ts new file mode 100644 index 0000000000..bfc4cdce58 --- /dev/null +++ b/packages/core/src/tools/getZoneJsOriginalValue.spec.ts @@ -0,0 +1,32 @@ +import { stubZoneJs } from '../../test/specHelper' + +import { getZoneJsOriginalValue } from './getZoneJsOriginalValue' +import { noop } from './utils' + +describe('getZoneJsOriginalValue', () => { + let zoneJsStub: ReturnType | undefined + + afterEach(() => { + zoneJsStub?.restore() + }) + + it('returns undefined if Zone is not not defined', () => { + expect(getZoneJsOriginalValue({ toto: noop }, 'toto')).toBeUndefined() + }) + + it("returns undefined if Zone is defined but didn't patch that method", () => { + zoneJsStub = stubZoneJs() + expect(getZoneJsOriginalValue({ toto: noop }, 'toto')).toBeUndefined() + }) + + it('returns the original value if Zone did patch the method', () => { + zoneJsStub = stubZoneJs() + + const originalMethod = () => { + // noop + } + expect(getZoneJsOriginalValue({ toto: noop, [zoneJsStub.getSymbol('toto')]: originalMethod }, 'toto')).toBe( + originalMethod + ) + }) +}) diff --git a/packages/core/src/tools/getZoneJsOriginalValue.ts b/packages/core/src/tools/getZoneJsOriginalValue.ts new file mode 100644 index 0000000000..5f39326f42 --- /dev/null +++ b/packages/core/src/tools/getZoneJsOriginalValue.ts @@ -0,0 +1,28 @@ +export interface BrowserWindowWithZoneJs extends Window { + Zone?: { + __symbol__: (name: string) => string + } +} + +/** + * Gets the original value for a DOM API that was potentially patched by Zone.js. + * + * Zone.js[1] is a library that patches a bunch of JS and DOM APIs. It usually stores the original + * value of the patched functions/constructors/methods in a hidden property prefixed by + * __zone_symbol__. + * + * In multiple occasions, we observed that Zone.js is the culprit of important issues leading to + * browser resource exhaustion (memory leak, high CPU usage). This method is used as a workaround to + * use the original DOM API instead of the one patched by Zone.js. + * + * [1]: https://github.com/angular/angular/tree/main/packages/zone.js + */ +export function getZoneJsOriginalValue( + target: Target, + name: Name +): Target[Name] | undefined { + const browserWindow = window as BrowserWindowWithZoneJs + if (browserWindow.Zone) { + return (target as any)[browserWindow.Zone.__symbol__(name)] as Target[Name] + } +} diff --git a/packages/core/test/specHelper.ts b/packages/core/test/specHelper.ts index 142a3cd841..49a8f638fc 100644 --- a/packages/core/test/specHelper.ts +++ b/packages/core/test/specHelper.ts @@ -4,6 +4,7 @@ import { resetNavigationStart } from '../src/tools/timeUtils' import { buildUrl } from '../src/tools/urlPolyfill' import { noop, objectEntries, assign } from '../src/tools/utils' import type { BrowserWindowWithEventBridge } from '../src/transport' +import type { BrowserWindowWithZoneJs } from '../src/tools/getZoneJsOriginalValue' // to simulate different build env behavior export interface BuildEnvWindow { @@ -485,3 +486,31 @@ export function interceptRequests() { }, } } + +export function stubZoneJs() { + const browserWindow = window as BrowserWindowWithZoneJs + const restorers: Array<() => void> = [] + + function getSymbol(name: string) { + return `__zone_symbol__${name}` + } + + browserWindow.Zone = { __symbol__: getSymbol } + + return { + restore: () => { + delete browserWindow.Zone + restorers.forEach((restorer) => restorer()) + }, + getSymbol, + replaceProperty(target: Target, name: Name, replacement: Target[Name]) { + const original = target[name] + target[name] = replacement + ;(target as any)[getSymbol(name)] = original + restorers.push(() => { + delete (target as any)[getSymbol(name)] + target[name] = original + }) + }, + } +} diff --git a/packages/core/test/stubReportApis.ts b/packages/core/test/stubReportApis.ts index 56709a30eb..d18cdd962a 100644 --- a/packages/core/test/stubReportApis.ts +++ b/packages/core/test/stubReportApis.ts @@ -46,7 +46,7 @@ export function stubReportingObserver() { } export function stubCspEventListener() { - spyOn(document, 'addEventListener').and.callFake((_type: string, listener: EventListener) => { + spyOn(EventTarget.prototype, 'addEventListener').and.callFake((_type: string, listener: EventListener) => { listeners.push(listener) }) diff --git a/packages/rum-core/src/browser/domMutationObservable.spec.ts b/packages/rum-core/src/browser/domMutationObservable.spec.ts index 5c833468ee..5ad5bf61c4 100644 --- a/packages/rum-core/src/browser/domMutationObservable.spec.ts +++ b/packages/rum-core/src/browser/domMutationObservable.spec.ts @@ -1,5 +1,5 @@ import { isIE } from '@datadog/browser-core' -import type { BrowserWindow } from './domMutationObservable' +import { stubZoneJs } from '../../../core/test/specHelper' import { createDOMMutationObservable, getMutationObserverConstructor } from './domMutationObservable' // The MutationObserver invokes its callback in an event loop microtask, making this asynchronous. @@ -108,38 +108,31 @@ describe('domMutationObservable', () => { ) describe('Zone.js support', () => { - const browserWindow = window as BrowserWindow - const OriginalMutationObserverConstructor = browserWindow.MutationObserver! + let zoneJsStub: ReturnType + const OriginalMutationObserverConstructor = window.MutationObserver beforeEach(() => { - browserWindow.Zone = { __symbol__: zoneSymbol } + zoneJsStub = stubZoneJs() }) afterEach(() => { - delete browserWindow.Zone - delete browserWindow[zoneSymbol('MutationObserver') as any] - browserWindow.MutationObserver = OriginalMutationObserverConstructor + zoneJsStub.restore() + window.MutationObserver = OriginalMutationObserverConstructor }) it('gets the original MutationObserver constructor from the "window" object (Zone.js >= 0.8.6)', () => { - browserWindow.MutationObserver = function () { + zoneJsStub.replaceProperty(window, 'MutationObserver', function () { // This won't be instantiated. - } as any - browserWindow[zoneSymbol('MutationObserver') as any] = OriginalMutationObserverConstructor as any - + } as any) expect(getMutationObserverConstructor()).toBe(OriginalMutationObserverConstructor) }) it('gets the original MutationObserver constructor from a patched instance (Zone.js < 0.8.6)', () => { - browserWindow.MutationObserver = function (this: any, callback: () => void) { - this[zoneSymbol('originalInstance')] = new OriginalMutationObserverConstructor(callback) + window.MutationObserver = function (this: any, callback: () => void) { + this[zoneJsStub.getSymbol('originalInstance')] = new OriginalMutationObserverConstructor(callback) } as any expect(getMutationObserverConstructor()).toBe(OriginalMutationObserverConstructor) }) - - function zoneSymbol(name: string) { - return `__zone_symbol__${name}` - } }) }) diff --git a/packages/rum-core/src/browser/domMutationObservable.ts b/packages/rum-core/src/browser/domMutationObservable.ts index 777d40dc7f..217dfdfa54 100644 --- a/packages/rum-core/src/browser/domMutationObservable.ts +++ b/packages/rum-core/src/browser/domMutationObservable.ts @@ -1,4 +1,4 @@ -import { monitor, noop, Observable } from '@datadog/browser-core' +import { monitor, noop, Observable, getZoneJsOriginalValue } from '@datadog/browser-core' export function createDOMMutationObservable() { const MutationObserver = getMutationObserverConstructor() @@ -21,11 +21,10 @@ export function createDOMMutationObservable() { } type MutationObserverConstructor = new (callback: MutationCallback) => MutationObserver + export interface BrowserWindow extends Window { MutationObserver?: MutationObserverConstructor - Zone?: { - __symbol__: (name: string) => string - } + Zone?: unknown } export function getMutationObserverConstructor(): MutationObserverConstructor | undefined { @@ -44,14 +43,10 @@ export function getMutationObserverConstructor(): MutationObserverConstructor | // [1] https://github.com/angular/angular/issues/26948 // [2] https://github.com/angular/angular/issues/31712 if (browserWindow.Zone) { - const zoneSymbol = browserWindow.Zone.__symbol__ - // Zone.js 0.8.6+ is storing original class constructors into the browser 'window' object[3]. // // [3] https://github.com/angular/angular/blob/6375fa79875c0fe7b815efc45940a6e6f5c9c9eb/packages/zone.js/lib/common/utils.ts#L288 - constructor = browserWindow[zoneSymbol('MutationObserver') as any] as unknown as - | MutationObserverConstructor - | undefined + constructor = getZoneJsOriginalValue(browserWindow, 'MutationObserver') if (!constructor && browserWindow.MutationObserver) { // Anterior Zone.js versions (used in Angular 2) does not expose the original MutationObserver @@ -61,11 +56,11 @@ export function getMutationObserverConstructor(): MutationObserverConstructor | // // [4] https://github.com/angular/zone.js/blob/v0.8.5/lib/common/utils.ts#L412 - const patchedInstance = new browserWindow.MutationObserver(noop) as any - const originalInstance = patchedInstance[zoneSymbol('originalInstance')] as - | { constructor: MutationObserverConstructor } - | undefined + const patchedInstance = new browserWindow.MutationObserver(noop) as { + originalInstance?: { constructor: MutationObserverConstructor } + } + const originalInstance = getZoneJsOriginalValue(patchedInstance, 'originalInstance') constructor = originalInstance && originalInstance.constructor } }