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

fix: patch removeEventListener to properly remove patched callbacks #158

Merged
merged 12 commits into from
Jul 24, 2020
Merged
4 changes: 3 additions & 1 deletion .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ jobs:
path: |
node_modules
packages/*/node_modules
metapackages/*/node_modules
plugins/node/*/node_modules
plugins/web/*/node_modules
propagators/*/node_modules
key: ${{ runner.os }}-${{ hashFiles('**/package.json') }}

- name: Bootstrap
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
moduleName = this.component;
private _spansData = new WeakMap<api.Span, SpanData>();
private _zonePatched = false;
// for addEventListener/removeEventListener state
private _wrappedListeners = new WeakMap<
Function,
Map<string, Map<HTMLElement, Function>>
>();

constructor() {
super('@opentelemetry/plugin-user-interaction', VERSION);
Expand Down Expand Up @@ -165,6 +170,61 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
}
}

/**
* Returns true iff we should use the patched callback; false if it's already been patched
*/
private addPatchedListener(
on: HTMLElement,
type: string,
listener: Function,
wrappedListener: Function
): boolean {
let listener2Type = this._wrappedListeners.get(listener);
if (!listener2Type) {
listener2Type = new Map();
Copy link

Choose a reason for hiding this comment

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

Is this going to be coerced into a WeakMap when you set it on _wrappedListeners?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, though making the HTMLElement reference weak is a good idea. Unfortunately I don't see a way to do it since there are no iterators/size/isEmpty methods available on WeakMap and without it we'd definitely leak. I'll ponder this but I'd like to get this fix in as it is since it's clearly better than the current breaking of functional behavior.

Copy link

Choose a reason for hiding this comment

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

Awesome, thanks for the clarification!

this._wrappedListeners.set(listener, listener2Type);
}
let element2patched = listener2Type.get(type);
if (!element2patched) {
element2patched = new Map();
listener2Type.set(type, element2patched);
}
if (element2patched.has(on)) {
return false;
}
element2patched.set(on, wrappedListener);
return true;
}

/**
* Returns the patched version of the callback (or undefined)
*/
private removePatchedListener(
on: HTMLElement,
type: string,
listener: Function
): Function | undefined {
const listener2Type = this._wrappedListeners.get(listener);
if (!listener2Type) {
return undefined;
}
const element2patched = listener2Type.get(type);
if (!element2patched) {
return undefined;
}
const patched = element2patched.get(on);
if (patched) {
element2patched.delete(on);
if (element2patched.size === 0) {
listener2Type.delete(type);
if (listener2Type.size === 0) {
this._wrappedListeners.delete(listener);
}
}
}
return patched;
}

/**
* This patches the addEventListener of HTMLElement to be able to
* auto instrument the click events
Expand All @@ -179,8 +239,12 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
listener: any,
useCapture: any
) {
const once = useCapture && useCapture.once;
const patchedListener = (...args: any[]) => {
const target = this;
if (once) {
plugin.removePatchedListener(this, type, listener);
}
const span = plugin._createSpan(target, 'click');
if (span) {
return plugin._tracer.withSpan(span, () => {
Expand All @@ -193,7 +257,37 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
return listener.apply(target, args);
}
};
return original.call(this, type, patchedListener, useCapture);
if (plugin.addPatchedListener(this, type, listener, patchedListener)) {
return original.call(this, type, patchedListener, useCapture);
}
};
};
}

/**
* This patches the removeEventListener of HTMLElement to handle the fact that
* we patched the original callbacks
* This is done when zone is not available
*/
private _patchRemoveEventListener() {
const plugin = this;
return (original: Function) => {
return function removeEventListenerPatched(
this: HTMLElement,
type: any,
listener: any,
useCapture: any
) {
const wrappedListener = plugin.removePatchedListener(
this,
type,
listener
);
if (wrappedListener) {
return original.call(this, type, wrappedListener, useCapture);
} else {
return original.call(this, type, listener, useCapture);
}
};
};
}
Expand Down Expand Up @@ -434,11 +528,22 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
'removing previous patch from method addEventListener'
);
}
if (isWrapped(HTMLElement.prototype.removeEventListener)) {
shimmer.unwrap(HTMLElement.prototype, 'removeEventListener');
this._logger.debug(
'removing previous patch from method removeEventListener'
);
}
shimmer.wrap(
HTMLElement.prototype,
'addEventListener',
this._patchElement()
);
shimmer.wrap(
HTMLElement.prototype,
'removeEventListener',
this._patchRemoveEventListener()
);
}

this._patchHistoryApi();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,78 @@ describe('UserInteractionPlugin', () => {
context.disable();
});

it('should not break removeEventListener', () => {
let called = false;
const listener = function () {
called = true;
};
// add same listener three different ways
Copy link
Member

@obecny obecny Jul 23, 2020

Choose a reason for hiding this comment

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

I'm curious about one scenario. To check that can you modify this a bit to test the case with overwriting the previous event with the same callback

let called2 = 0;
const listener2 = function () {
  called2++;
};
document.body.addEventListener('bodyEvent', listener2);
document.body.addEventListener('bodyEvent', listener2);
document.body.addEventListener('bodyEvent', listener2);

// now fun
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(called2, 1);

Copy link
Member

Choose a reason for hiding this comment

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

fixed my example

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, good catch and an easy fix for this one based on the tracking state. Pushing fix momentarily.

Copy link
Member

Choose a reason for hiding this comment

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

There is one more thing a bonus :). Currently you can call addEventListener providing a 3rd param options. In options you can set once:true which means the event will be removed automatically. It might be worth to test that too.

Copy link
Member

@obecny obecny Jul 23, 2020

Choose a reason for hiding this comment

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

let called2 = 0;
const listener2 = function () {
  called2++;
};
document.body.addEventListener('bodyEvent', listener2);
document.body.addEventListener('bodyEvent', listener2, { once: true });

// now fun
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(called2, 1);
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(called2, 1); // error ?

Copy link
Member

Choose a reason for hiding this comment

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

and you can revert it add once first and then add without options, heh small things and many edge cases

Copy link
Member Author

Choose a reason for hiding this comment

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

Argh. There's also the case that once:true shouldn't prevent us from allowing it to be adding later (it's not really a double-register). Will dive in and fix this one tomorrow. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

thx for taking this into deep details :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't leave it alone; handled the once:true callbacks properly being removed when an event is dispatched.

document.body.addEventListener('bodyEvent', listener);
document.body.addEventListener('bodyEvent2', listener);
document.addEventListener('docEvent', listener);
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(called, true);
called = false;
// Remove first callback, second type should still fire
document.body.removeEventListener('bodyEvent', listener);
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(called, false);
document.body.dispatchEvent(new Event('bodyEvent2'));
assert.strictEqual(called, true);
called = false;
// Remove doc callback, body 2 should still fire
document.removeEventListener('docEvent', listener);
document.dispatchEvent(new Event('docEvent'));
assert.strictEqual(called, false);
document.body.dispatchEvent(new Event('bodyEvent2'));
assert.strictEqual(called, true);
called = false;
// Finally, remove the last one and nothing should fire
document.body.removeEventListener('bodyEvent2', listener);
document.body.dispatchEvent(new Event('bodyEvent'));
document.body.dispatchEvent(new Event('bodyEvent2'));
document.dispatchEvent(new Event('docEvent'));
assert.strictEqual(called, false);
});

it('should not double-register a listener', () => {
let callCount = 0;
const listener = function () {
callCount++;
};
// addEventListener semantics treat the second call as a no-op
document.body.addEventListener('bodyEvent', listener);
document.body.addEventListener('bodyEvent', listener);
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(callCount, 1);
// now ensure remove still works
callCount = 0;
document.body.removeEventListener('bodyEvent', listener);
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(callCount, 0);
});

it('should handle once-only callbacks', () => {
let callCount = 0;
const listener = function () {
callCount++;
};
// addEventListener semantics treat the second call as a no-op
document.body.addEventListener('bodyEvent', listener, { once: true });
document.body.addEventListener('bodyEvent', listener); // considered a double-register
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(callCount, 1);
// now that it's been dispatched once, it's been removed
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(callCount, 1);
// should be able to re-add
document.body.addEventListener('bodyEvent', listener);
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(callCount, 2);
johnbley marked this conversation as resolved.
Show resolved Hide resolved
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(callCount, 3);
});

it('should handle task without async operation', () => {
fakeInteraction();
assert.equal(exportSpy.args.length, 1, 'should export one span');
Expand Down