Skip to content

Commit 4e551ad

Browse files
obecnyobecny
authored
Merge pull request open-telemetry#158 from johnbley/master
fix: patch removeEventListener to properly remove patched callbacks
2 parents 559fd20 + 4e6d498 commit 4e551ad

File tree

2 files changed

+178
-1
lines changed

2 files changed

+178
-1
lines changed

‎plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts‎

Lines changed: 106 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,11 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
4848
moduleName = this.component;
4949
private _spansData = new WeakMap<api.Span, SpanData>();
5050
private _zonePatched = false;
51+
// for addEventListener/removeEventListener state
52+
private _wrappedListeners = new WeakMap<
53+
Function,
54+
Map<string, Map<HTMLElement, Function>>
55+
>();
5156

5257
constructor() {
5358
super('@opentelemetry/plugin-user-interaction', VERSION);
@@ -165,6 +170,61 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
165170
}
166171
}
167172

173+
/**
174+
* Returns true iff we should use the patched callback; false if it's already been patched
175+
*/
176+
private addPatchedListener(
177+
on: HTMLElement,
178+
type: string,
179+
listener: Function,
180+
wrappedListener: Function
181+
): boolean {
182+
let listener2Type = this._wrappedListeners.get(listener);
183+
if (!listener2Type) {
184+
listener2Type = new Map();
185+
this._wrappedListeners.set(listener, listener2Type);
186+
}
187+
let element2patched = listener2Type.get(type);
188+
if (!element2patched) {
189+
element2patched = new Map();
190+
listener2Type.set(type, element2patched);
191+
}
192+
if (element2patched.has(on)) {
193+
return false;
194+
}
195+
element2patched.set(on, wrappedListener);
196+
return true;
197+
}
198+
199+
/**
200+
* Returns the patched version of the callback (or undefined)
201+
*/
202+
private removePatchedListener(
203+
on: HTMLElement,
204+
type: string,
205+
listener: Function
206+
): Function | undefined {
207+
const listener2Type = this._wrappedListeners.get(listener);
208+
if (!listener2Type) {
209+
return undefined;
210+
}
211+
const element2patched = listener2Type.get(type);
212+
if (!element2patched) {
213+
return undefined;
214+
}
215+
const patched = element2patched.get(on);
216+
if (patched) {
217+
element2patched.delete(on);
218+
if (element2patched.size === 0) {
219+
listener2Type.delete(type);
220+
if (listener2Type.size === 0) {
221+
this._wrappedListeners.delete(listener);
222+
}
223+
}
224+
}
225+
return patched;
226+
}
227+
168228
/**
169229
* This patches the addEventListener of HTMLElement to be able to
170230
* auto instrument the click events
@@ -179,8 +239,12 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
179239
listener: any,
180240
useCapture: any
181241
) {
242+
const once = useCapture && useCapture.once;
182243
const patchedListener = (...args: any[]) => {
183244
const target = this;
245+
if (once) {
246+
plugin.removePatchedListener(this, type, listener);
247+
}
184248
const span = plugin._createSpan(target, 'click');
185249
if (span) {
186250
return plugin._tracer.withSpan(span, () => {
@@ -193,7 +257,37 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
193257
return listener.apply(target, args);
194258
}
195259
};
196-
return original.call(this, type, patchedListener, useCapture);
260+
if (plugin.addPatchedListener(this, type, listener, patchedListener)) {
261+
return original.call(this, type, patchedListener, useCapture);
262+
}
263+
};
264+
};
265+
}
266+
267+
/**
268+
* This patches the removeEventListener of HTMLElement to handle the fact that
269+
* we patched the original callbacks
270+
* This is done when zone is not available
271+
*/
272+
private _patchRemoveEventListener() {
273+
const plugin = this;
274+
return (original: Function) => {
275+
return function removeEventListenerPatched(
276+
this: HTMLElement,
277+
type: any,
278+
listener: any,
279+
useCapture: any
280+
) {
281+
const wrappedListener = plugin.removePatchedListener(
282+
this,
283+
type,
284+
listener
285+
);
286+
if (wrappedListener) {
287+
return original.call(this, type, wrappedListener, useCapture);
288+
} else {
289+
return original.call(this, type, listener, useCapture);
290+
}
197291
};
198292
};
199293
}
@@ -434,11 +528,22 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
434528
'removing previous patch from method addEventListener'
435529
);
436530
}
531+
if (isWrapped(HTMLElement.prototype.removeEventListener)) {
532+
shimmer.unwrap(HTMLElement.prototype, 'removeEventListener');
533+
this._logger.debug(
534+
'removing previous patch from method removeEventListener'
535+
);
536+
}
437537
shimmer.wrap(
438538
HTMLElement.prototype,
439539
'addEventListener',
440540
this._patchElement()
441541
);
542+
shimmer.wrap(
543+
HTMLElement.prototype,
544+
'removeEventListener',
545+
this._patchRemoveEventListener()
546+
);
442547
}
443548

444549
this._patchHistoryApi();

‎plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,78 @@ describe('UserInteractionPlugin', () => {
8989
context.disable();
9090
});
9191

92+
it('should not break removeEventListener', () => {
93+
let called = false;
94+
const listener = function () {
95+
called = true;
96+
};
97+
// add same listener three different ways
98+
document.body.addEventListener('bodyEvent', listener);
99+
document.body.addEventListener('bodyEvent2', listener);
100+
document.addEventListener('docEvent', listener);
101+
document.body.dispatchEvent(new Event('bodyEvent'));
102+
assert.strictEqual(called, true);
103+
called = false;
104+
// Remove first callback, second type should still fire
105+
document.body.removeEventListener('bodyEvent', listener);
106+
document.body.dispatchEvent(new Event('bodyEvent'));
107+
assert.strictEqual(called, false);
108+
document.body.dispatchEvent(new Event('bodyEvent2'));
109+
assert.strictEqual(called, true);
110+
called = false;
111+
// Remove doc callback, body 2 should still fire
112+
document.removeEventListener('docEvent', listener);
113+
document.dispatchEvent(new Event('docEvent'));
114+
assert.strictEqual(called, false);
115+
document.body.dispatchEvent(new Event('bodyEvent2'));
116+
assert.strictEqual(called, true);
117+
called = false;
118+
// Finally, remove the last one and nothing should fire
119+
document.body.removeEventListener('bodyEvent2', listener);
120+
document.body.dispatchEvent(new Event('bodyEvent'));
121+
document.body.dispatchEvent(new Event('bodyEvent2'));
122+
document.dispatchEvent(new Event('docEvent'));
123+
assert.strictEqual(called, false);
124+
});
125+
126+
it('should not double-register a listener', () => {
127+
let callCount = 0;
128+
const listener = function () {
129+
callCount++;
130+
};
131+
// addEventListener semantics treat the second call as a no-op
132+
document.body.addEventListener('bodyEvent', listener);
133+
document.body.addEventListener('bodyEvent', listener);
134+
document.body.dispatchEvent(new Event('bodyEvent'));
135+
assert.strictEqual(callCount, 1);
136+
// now ensure remove still works
137+
callCount = 0;
138+
document.body.removeEventListener('bodyEvent', listener);
139+
document.body.dispatchEvent(new Event('bodyEvent'));
140+
assert.strictEqual(callCount, 0);
141+
});
142+
143+
it('should handle once-only callbacks', () => {
144+
let callCount = 0;
145+
const listener = function () {
146+
callCount++;
147+
};
148+
// addEventListener semantics treat the second call as a no-op
149+
document.body.addEventListener('bodyEvent', listener, { once: true });
150+
document.body.addEventListener('bodyEvent', listener); // considered a double-register
151+
document.body.dispatchEvent(new Event('bodyEvent'));
152+
assert.strictEqual(callCount, 1);
153+
// now that it's been dispatched once, it's been removed
154+
document.body.dispatchEvent(new Event('bodyEvent'));
155+
assert.strictEqual(callCount, 1);
156+
// should be able to re-add
157+
document.body.addEventListener('bodyEvent', listener);
158+
document.body.dispatchEvent(new Event('bodyEvent'));
159+
assert.strictEqual(callCount, 2);
160+
document.body.dispatchEvent(new Event('bodyEvent'));
161+
assert.strictEqual(callCount, 3);
162+
});
163+
92164
it('should handle task without async operation', () => {
93165
fakeInteraction();
94166
assert.equal(exportSpy.args.length, 1, 'should export one span');

0 commit comments

Comments
 (0)