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(eventPool): check existance of handler in EventTarget._unlisten #2246

Closed
wants to merge 2 commits into from

Conversation

mhagmajer
Copy link

Fixes #2120

@mhagmajer
Copy link
Author

Failing check seems like false negative

@layershifter
Copy link
Member

@levithomason I've marked this as ready for review, however I'm not sure that this is fixes something. We don't have reproduction and I don't see how this can be covered by unit tests.

@layershifter layershifter changed the title Fix EventTarget._unlisten fix(eventPool): check existance of handler in EventTarget._unlisten Oct 25, 2017
@codecov-io
Copy link

codecov-io commented Oct 25, 2017

Codecov Report

Merging #2246 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2246   +/-   ##
=======================================
  Coverage   99.73%   99.73%           
=======================================
  Files         151      151           
  Lines        2624     2624           
=======================================
  Hits         2617     2617           
  Misses          7        7

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5841dd7...d65569d. Read the comment docs.

@levithomason
Copy link
Member

As I've shown here #2120 (comment), both our code and the browser's native code already gracefully handle removing undefined handlers. I want to get a broken test case that this fixes before merging.

@levithomason
Copy link
Member

I should note, I spent 20-30 minutes researching and also trying to reproduce this. I could not do so in Chrome nor PhantomJS no matter what abuse I threw at our EventTarget and eventStack. I was also unabled to reproduce this in Chrome with removeEventListener.

The closest I could find was this angular/zone.js#21 issue. They had this error, but they also seemed unable to reproduce it. It seemed to resolve itself, unfortunately.

Lastly, neither _bound nor bound are properties that occur in our codebase. This leads me to believe the error is something native to the browser, or some other conflicting lib.

@levithomason
Copy link
Member

Any more progress on this one?

@mhagmajer
Copy link
Author

mhagmajer commented Nov 4, 2017 via email

@layershifter
Copy link
Member

Closing for housekeeping. Will be reopened if we will have a repro case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants