fix: patch removeEventListener to properly remove patched callbacks #158

johnbley · 2020-07-23T12:39:10Z

We need to patch removeEventListener in order to compensate for wrapping callbacks passed to addEventListener.

codecov · 2020-07-23T12:46:30Z

Codecov Report

Merging #158 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
+ Coverage   94.06%   94.10%   +0.04%     
==========================================
  Files          74       74              
  Lines        3536     3582      +46     
  Branches      374      387      +13     
==========================================
+ Hits         3326     3371      +45     
- Misses        210      211       +1

Impacted Files	Coverage Δ
...try-plugin-user-interaction/src/userInteraction.ts	`93.80% <0.00%> (+0.94%)`	⬆️

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

dyladan · 2020-07-23T14:12:18Z

@rezakrimi the lint action is failing completely. Can you please look into this?

rezakrimi · 2020-07-23T14:27:47Z

@rezakrimi the lint action is failing completely. Can you please look into this?

Sure.

obecny · 2020-07-23T16:01:17Z

Can you please provide more details what is the exact problem here ?, thx

johnbley · 2020-07-23T16:05:39Z

Can you please provide more details what is the exact problem here ?, thx

Sure thing! The added test shows that the addEventListener instrumentation actually breaks apps because it doesn't have the compensating logic for removeEventListener. A patched callback is actually added as the event listener, and when the app tries to remove the original one, it's not found/ignored, leaving our patched callback present (along with unexpected calls to the underlying original one). For some apps, this doesn't matter, but for others this can be arbitrarily bad (breaking functionality, thrown errors, infinite loops, etc. etc.).

obecny · 2020-07-23T16:27:34Z

it looks fine, the failing build might be connected with yesterday eslint merge, we have to check it @dyladan https://circleci.com/gh/open-telemetry/opentelemetry-js-contrib/1546
EDIT:
I might be wrong with the link, but when checking the lint in this PR something else is failing not related to this PR

dyladan · 2020-07-23T16:34:54Z

it looks fine, the failing build might be connected with yesterday eslint merge, we have to check it @dyladan circleci.com/gh/open-telemetry/opentelemetry-js-contrib/1546
EDIT:
I might be wrong with the link, but when checking the lint in this PR something else is failing not related to this PR

It is definitely related. eslint isn't running at all because of a gts dependency error.

rezakrimi · 2020-07-23T16:42:12Z

it looks fine, the failing build might be connected with yesterday eslint merge, we have to check it @dyladan circleci.com/gh/open-telemetry/opentelemetry-js-contrib/1546
EDIT:
I might be wrong with the link, but when checking the lint in this PR something else is failing not related to this PR

It is definitely related. eslint isn't running at all because of a gts dependency error.

yes the caching was wrong, I'll make a PR today to fix it

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

rezakrimi · 2020-07-23T17:04:26Z

#159 should fix the lint check.

fix: fixing caching issue for lint action

dyladan · 2020-07-23T17:10:42Z

@johnbley if you update from master now the lint should run properly

johnbley · 2020-07-23T17:50:08Z

@johnbley if you update from master now the lint should run properly

Will do, but please hold on merging until I find the time to properly handle some edge cases better.

johnbley · 2020-07-23T18:47:31Z

Looks like the lint step in the build is failing again, but this time with out-of-date code... I ran lint:fix and pushed those changes.

rezakrimi · 2020-07-23T19:05:54Z

Looks like the lint step in the build is failing again, but this time with out-of-date code... I ran lint:fix and pushed those changes.

That's strange. Same thing happened to my PR on the main opentelemetry-js repo. For me it just got resolved after pushing another commit.

obecny · 2020-07-23T19:29:51Z

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

+      const listener = function () {
+        called = true;
+      };
+      // add same listener three different ways


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);

fixed my example

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

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.

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 ?

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

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!

thx for taking this into deep details :)

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

obecny

great job, thx for investigating this further :)

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

dyladan

Apart from minor suggestions by @obecny this LGTM

lykkin · 2020-07-23T21:21:59Z

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

+  ): boolean {
+    let listener2Type = this._wrappedListeners.get(listener);
+    if (!listener2Type) {
+      listener2Type = new Map();


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

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.

Awesome, thanks for the clarification!

johnbley · 2020-07-23T23:50:06Z

great job, thx for investigating this further :)

Thanks to all for spotting further issues and edge cases!

fix: patch removeEventListener to properly remove patched functions

0a2a397

johnbley requested a review from a team July 23, 2020 12:39

dyladan reviewed Jul 23, 2020

View reviewed changes

plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts Outdated Show resolved Hide resolved

chore: delete patched listener from weak map

5117e4b

fix: fixing caching issue for lint action

6b35ea8

lykkin reviewed Jul 23, 2020

View reviewed changes

plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts Outdated Show resolved Hide resolved

Merge pull request #159 from rezakrimi/lint-action

559fd20

fix: fixing caching issue for lint action

johnbley added 3 commits July 23, 2020 13:53

fix: patch removeEventListener to properly remove patched functions

e238549

chore: delete patched listener from weak map

b56aa2b

Merge branch 'master' of github.com:johnbley/opentelemetry-js-contrib

a50f6cf

johnbley marked this pull request as draft July 23, 2020 17:56

johnbley added 2 commits July 23, 2020 14:38

chore: also handle type+element in removeEventListener

e6a0b97

chore: lint

f790b62

johnbley marked this pull request as ready for review July 23, 2020 18:58

obecny reviewed Jul 23, 2020

View reviewed changes

johnbley added 2 commits July 23, 2020 15:56

chore: handle double-registered listeners

f529213

chore: also handle callbacks with once:true

bfb3d67

obecny approved these changes Jul 23, 2020

View reviewed changes

plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts Outdated Show resolved Hide resolved

plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts Show resolved Hide resolved

dyladan approved these changes Jul 23, 2020

View reviewed changes

lykkin reviewed Jul 23, 2020

View reviewed changes

chore: deeper testing for once:true

4e6d498

lykkin approved these changes Jul 23, 2020

View reviewed changes

obecny merged commit 4e551ad into open-telemetry:master Jul 24, 2020

dyladan added the bug Something isn't working label Aug 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: patch removeEventListener to properly remove patched callbacks #158

fix: patch removeEventListener to properly remove patched callbacks #158

johnbley commented Jul 23, 2020 •

edited

Loading

codecov bot commented Jul 23, 2020 •

edited

Loading

dyladan commented Jul 23, 2020

rezakrimi commented Jul 23, 2020

obecny commented Jul 23, 2020

johnbley commented Jul 23, 2020

obecny commented Jul 23, 2020 •

edited

Loading

dyladan commented Jul 23, 2020

rezakrimi commented Jul 23, 2020

rezakrimi commented Jul 23, 2020

dyladan commented Jul 23, 2020

johnbley commented Jul 23, 2020

johnbley commented Jul 23, 2020

rezakrimi commented Jul 23, 2020

obecny Jul 23, 2020 •

edited

Loading

obecny Jul 23, 2020

johnbley Jul 23, 2020

obecny Jul 23, 2020

obecny Jul 23, 2020 •

edited

Loading

obecny Jul 23, 2020

johnbley Jul 23, 2020

obecny Jul 23, 2020

johnbley Jul 23, 2020

obecny left a comment

dyladan left a comment

lykkin Jul 23, 2020

johnbley Jul 23, 2020

lykkin Jul 23, 2020

johnbley commented Jul 23, 2020

fix: patch removeEventListener to properly remove patched callbacks #158

fix: patch removeEventListener to properly remove patched callbacks #158

Conversation

johnbley commented Jul 23, 2020 • edited Loading

codecov bot commented Jul 23, 2020 • edited Loading

Codecov Report

dyladan commented Jul 23, 2020

rezakrimi commented Jul 23, 2020

obecny commented Jul 23, 2020

johnbley commented Jul 23, 2020

obecny commented Jul 23, 2020 • edited Loading

dyladan commented Jul 23, 2020

rezakrimi commented Jul 23, 2020

rezakrimi commented Jul 23, 2020

dyladan commented Jul 23, 2020

johnbley commented Jul 23, 2020

johnbley commented Jul 23, 2020

rezakrimi commented Jul 23, 2020

obecny Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbley commented Jul 23, 2020

johnbley commented Jul 23, 2020 •

edited

Loading

codecov bot commented Jul 23, 2020 •

edited

Loading

obecny commented Jul 23, 2020 •

edited

Loading

obecny Jul 23, 2020 •

edited

Loading

obecny Jul 23, 2020 •

edited

Loading