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

Fixing an issue where patching functions into Object.prototype makes the event removal crash. #4669

Merged
merged 6 commits into from Oct 30, 2017

Conversation

Projects
None yet
3 participants
@mmodrow
Contributor

mmodrow commented Oct 18, 2017

Description

I found a problem, where adding custom functions to Object.prototype made the event-off function crash upon iterating over all properties of an object.
I fixed it by checking for hasOwnProperty. As the tests failed for me on Win10 IE11 I also fixed issue #3100 that addressed this.

Specific Changes proposed

I added an if-clause to url.js‘s parseUrl to add the current window.location.protocol if the protocol of the given url is empty (or //).

I also added 2 if statements to events.js‘s off in the "type is falsy" if to check that data and data.handler is present (occasionally was undefined with me) and within the loop check if data.handler has t as it‘s own property.
I added an if statement to events.js‘s off in the if (!type) to check that data.handler is still present and has t as its own property while running the for loop. This way it won‘t try to remove monkey-patched properties.

To make the loop only run in the case of an omitted type parameter the surrounding if was corrected from (!type) to (type === undefined)

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors
Show outdated Hide outdated src/js/utils/events.js
Show outdated Hide outdated src/js/utils/events.js
Show outdated Hide outdated src/js/utils/url.js
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 18, 2017

Member

Thanks for the PR! I made some comments.

Member

gkatsev commented Oct 18, 2017

Thanks for the PR! I made some comments.

@mmodrow mmodrow changed the title from Fixing issue #3100 & fixing an issue where patching functions into Object.prototype makes the event removal crash. to ~~Fixing issue #3100 &~~ fixing an issue where patching functions into Object.prototype makes the event removal crash. Oct 18, 2017

@mmodrow mmodrow changed the title from ~~Fixing issue #3100 &~~ fixing an issue where patching functions into Object.prototype makes the event removal crash. to Fixing an issue where patching functions into Object.prototype makes the event removal crash. Oct 18, 2017

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 18, 2017

Member

So, I think I figured out the root cause of the issue. Video.js has a feature where on a component or the player if you call off() (like player.off()) without arguments, it will remove all bound events. However, an empty string is an argument but it's falsey, so, line 353 thinks that we didn't pass anything in and goes on to remove all the handlers. Seems like a proper solution would be to specifically check that type is set to undefined. So, something like if (type === undefined) instead of if (!type). Thoughts?

Member

gkatsev commented Oct 18, 2017

So, I think I figured out the root cause of the issue. Video.js has a feature where on a component or the player if you call off() (like player.off()) without arguments, it will remove all bound events. However, an empty string is an argument but it's falsey, so, line 353 thinks that we didn't pass anything in and goes on to remove all the handlers. Seems like a proper solution would be to specifically check that type is set to undefined. So, something like if (type === undefined) instead of if (!type). Thoughts?

@mmodrow

This comment has been minimized.

Show comment
Hide comment
@mmodrow

mmodrow Oct 18, 2017

Contributor

You are right in that if (undefined === type) (or other way around - your call) would be more precise than if (!type).

But that does not affect the problem I was aiming to fix. It still fails on off() as soon as there are custom properties in Object.prototype.
So we still need

for (const t in data.handlers) {
  if (data.handlers) {
    removeType(t);
  }
}
Contributor

mmodrow commented Oct 18, 2017

You are right in that if (undefined === type) (or other way around - your call) would be more precise than if (!type).

But that does not affect the problem I was aiming to fix. It still fails on off() as soon as there are custom properties in Object.prototype.
So we still need

for (const t in data.handlers) {
  if (data.handlers) {
    removeType(t);
  }
}
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 18, 2017

Member

Oh, I see.
Not exactly sure how the extra data.handlers check helps, though.
However, just doing an if (Object.prototype.hasOwnProperty.call(data.handlers, t) { removeType(t); } is probably best.
And if you can do the type === undefined check that would be great too.

Member

gkatsev commented Oct 18, 2017

Oh, I see.
Not exactly sure how the extra data.handlers check helps, though.
However, just doing an if (Object.prototype.hasOwnProperty.call(data.handlers, t) { removeType(t); } is probably best.
And if you can do the type === undefined check that would be great too.

@mmodrow

This comment has been minimized.

Show comment
Hide comment
@mmodrow

mmodrow Oct 18, 2017

Contributor

I did the change locally and if (Object.prototype.hasOwnProperty.call(data.handlers, t)) still fails if data.handlers is null or undefined.

The two obvious solutions would be

if (data.handlers && Object.prototype.hasOwnProperty.call(data.handlers, t))

and

if (Object.prototype.hasOwnProperty.call(data.handlers || {}, t))

Which one do you prefer?

Contributor

mmodrow commented Oct 18, 2017

I did the change locally and if (Object.prototype.hasOwnProperty.call(data.handlers, t)) still fails if data.handlers is null or undefined.

The two obvious solutions would be

if (data.handlers && Object.prototype.hasOwnProperty.call(data.handlers, t))

and

if (Object.prototype.hasOwnProperty.call(data.handlers || {}, t))

Which one do you prefer?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 18, 2017

Member

Probably the second one but I don't understand why data.handlers would ever be null at that point. Can you try and make a test case for this?

Member

gkatsev commented Oct 18, 2017

Probably the second one but I don't understand why data.handlers would ever be null at that point. Can you try and make a test case for this?

@mmodrow

This comment has been minimized.

Show comment
Hide comment
@mmodrow

mmodrow Oct 18, 2017

Contributor

That would be, because line 30 of events.js deletes the current t from data.handlers and then the lines 43-47 delete data.handlers, data.dispatcher and data.disabled if data.handlers has no own properties left.

So once the last own property is removed _cleanUpEvents kills data.handlers.

With "make a test case for this" you mean like unit test? Currently there are no unit tests for the events.js and I have never done unit testing on JS before - I don't feel up to the task of starting a new test suite.

Contributor

mmodrow commented Oct 18, 2017

That would be, because line 30 of events.js deletes the current t from data.handlers and then the lines 43-47 delete data.handlers, data.dispatcher and data.disabled if data.handlers has no own properties left.

So once the last own property is removed _cleanUpEvents kills data.handlers.

With "make a test case for this" you mean like unit test? Currently there are no unit tests for the events.js and I have never done unit testing on JS before - I don't feel up to the task of starting a new test suite.

improved event.js off()-check for type-parameter being omitted;
improved ownProperty-check for monkey-patched Object.prototype in off()
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 23, 2017

Member

Oh, I see. I was able to reproduce it locally. So, the issue here is really that for..in doesn't return only own enumerable properties. So, adding the if (Object.prototype.hasOwnProperty(...)) works, another solution is to iterate over Object.keys(data.handlers) instead.
But this should be good to go then. Thanks for bearing with me while I try to understand the issue :)

Member

gkatsev commented Oct 23, 2017

Oh, I see. I was able to reproduce it locally. So, the issue here is really that for..in doesn't return only own enumerable properties. So, adding the if (Object.prototype.hasOwnProperty(...)) works, another solution is to iterate over Object.keys(data.handlers) instead.
But this should be good to go then. Thanks for bearing with me while I try to understand the issue :)

@misteroneill

Good catch. Thanks!

@gkatsev gkatsev added the patch label Oct 23, 2017

@mmodrow

This comment has been minimized.

Show comment
Hide comment
@mmodrow

mmodrow Oct 23, 2017

Contributor

No problem.
I ran into that problem a couple of times in different places in my own code and didn't think of explaining that part of the problem to you.
Sorry for that on my part.
Thanks for taking your time with me.

Contributor

mmodrow commented Oct 23, 2017

No problem.
I ran into that problem a couple of times in different places in my own code and didn't think of explaining that part of the problem to you.
Sorry for that on my part.
Thanks for taking your time with me.

@gkatsev gkatsev added the confirmed label Oct 24, 2017

@mmodrow

This comment has been minimized.

Show comment
Hide comment
@mmodrow

mmodrow Oct 29, 2017

Contributor

Are we still missing some step that I should perform or is anything else still off here?
Or is this simply sticking around as nobody took the time merging this, yet?

Contributor

mmodrow commented Oct 29, 2017

Are we still missing some step that I should perform or is anything else still off here?
Or is this simply sticking around as nobody took the time merging this, yet?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 29, 2017

Member

Yup, we hadn't merged it yet. We'll be merging things in this week :)

Member

gkatsev commented Oct 29, 2017

Yup, we hadn't merged it yet. We'll be merging things in this week :)

@gkatsev gkatsev merged commit 7963913 into videojs:master Oct 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment