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

mmodrow · 2017-10-18T08:03:27Z

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

… functions.

…elines.

gkatsev · 2017-10-18T15:51:03Z

src/js/utils/events.js

@@ -351,8 +351,12 @@ export function off(elem, type, fn) {

  // Are we removing all bound events?
  if (!type) {
-    for (const t in data.handlers) {
-      removeType(t);
+    if (!!data && !!data.handlers) {


Why do this check here? If data.handlers doesn't exist, we should already have exited on line 338.

You are right. It appears as if I was overly cautious when looking for the reason of data.handlers being undefined.

gkatsev · 2017-10-18T15:53:15Z

src/js/utils/events.js

-      removeType(t);
+    if (!!data && !!data.handlers) {
+      for (const t in data.handlers) {
+        if (!!data.handlers && !!data.handlers.hasOwnProperty && data.handlers.hasOwnProperty(t)) {


does this mean that if hasOwnProperty is missing for whatever reason, we'll end up not actually removing the handler?

I just went in and reconstructed the situation.
I called player.off("") to get into the if (!type) statement.
It had data.handlers filled up and in the loop removed one child after another. It seems like after the last own property is removed data.handler gets removed, as when it starts processing the prototype properties data actually is {}.

So in the end

if (!type) { for (const t in data.handlers) { if (data.handlers) { removeType(t); } } return; }

would totally do the trick.

Is player.off('') what causing issues here? Are there other examples?

I hit this problem at work in a more complex code base. I cannot exactly find the instance/code point that caused it.

player.off('') was the most condensed trigger I could find to hit that code. It appears any falsy input would trigger the loop and thus the error.

I am constantly removing and adding players though... Maybe it's caused by one of the 3 xxx.off() calls in evented.js 369, track-list.js 115 and plugin.js 291? target.on('dispose', () => target.off()); at evented.js 369 seems especially likely.

gkatsev · 2017-10-18T15:53:51Z

src/js/utils/url.js

@@ -85,6 +85,10 @@ export const parseUrl = function(url) {
    document.body.removeChild(div);
  }

+  if (details.protocol === '') {


seems like this belongs in separate PR.

You are right. This is a separate topic.
It's my first time committing to a bigger project with such a refined build tool chain and I did not want to (and I believe it didn't let me) push failing code. Deemed that more important than singling out a secondary fix which is closed due to inactivity anyway and wait for it to be accepted before committing this PR.

If you prefer I can revert this here and add it to a separate PR.

Thanks for contributing!
Making multiple PRs is generally better because then a single topic isn't blocking another topic that has been approved and the discussion can focus on the thing issue at hand.

Ok. I will move it out from here.

gkatsev · 2017-10-18T16:08:05Z

Thanks for the PR! I made some comments.

gkatsev · 2017-10-18T19:27:06Z

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 · 2017-10-18T19:42:58Z

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 · 2017-10-18T19:48:33Z

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 · 2017-10-18T20:43:24Z

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 · 2017-10-18T22:04:08Z

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 · 2017-10-18T22:17:17Z

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 ownProperty-check for monkey-patched Object.prototype in off()

gkatsev · 2017-10-23T15:52:15Z

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!

mmodrow · 2017-10-23T19:32:20Z

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.

mmodrow · 2017-10-29T16:59:06Z

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 · 2017-10-29T17:13:30Z

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

Marc A. Modrow and others added 3 commits October 17, 2017 22:21

Fixed a problem with the event removal if Object.prototype has custom…

1db4831

… functions.

Slight reformatting in events.js to be compatible to the prepush guid…

311c3f5

…elines.

Fixed issue videojs#3100 to make the tests pass on Win10 for IE.

6774799

gkatsev reviewed Oct 18, 2017

View reviewed changes

mmodrow added 2 commits October 18, 2017 20:47

Implemented feedback from @gkatsev .

8567e03

Revert: Fix for issue videojs#3100 - will be handed in as separate PR

6ab5b1d

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

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

improved event.js off()-check for type-parameter being omitted;

14708fe

improved ownProperty-check for monkey-patched Object.prototype in off()

gkatsev approved these changes Oct 23, 2017

View reviewed changes

misteroneill approved these changes Oct 23, 2017

View reviewed changes

gkatsev added the patch This PR can be added to a patch release. label Oct 23, 2017

gkatsev added the confirmed label Oct 24, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mmodrow commented Oct 18, 2017 •

edited

Loading

gkatsev Oct 18, 2017

mmodrow Oct 18, 2017

gkatsev Oct 18, 2017

mmodrow Oct 18, 2017

gkatsev Oct 18, 2017

mmodrow Oct 18, 2017 •

edited

Loading

gkatsev Oct 18, 2017

mmodrow Oct 18, 2017

gkatsev Oct 18, 2017

mmodrow Oct 18, 2017

gkatsev commented Oct 18, 2017

gkatsev commented Oct 18, 2017

mmodrow commented Oct 18, 2017

gkatsev commented Oct 18, 2017

mmodrow commented Oct 18, 2017 •

edited

Loading

gkatsev commented Oct 18, 2017

mmodrow commented Oct 18, 2017 •

edited

Loading

gkatsev commented Oct 23, 2017

misteroneill left a comment

mmodrow commented Oct 23, 2017

mmodrow commented Oct 29, 2017

gkatsev commented Oct 29, 2017

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

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

Conversation

mmodrow commented Oct 18, 2017 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

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

mmodrow Oct 18, 2017 • 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

gkatsev commented Oct 18, 2017

gkatsev commented Oct 18, 2017

mmodrow commented Oct 18, 2017

gkatsev commented Oct 18, 2017

mmodrow commented Oct 18, 2017 • edited Loading

gkatsev commented Oct 18, 2017

mmodrow commented Oct 18, 2017 • edited Loading

gkatsev commented Oct 23, 2017

misteroneill left a comment

Choose a reason for hiding this comment

mmodrow commented Oct 23, 2017

mmodrow commented Oct 29, 2017

gkatsev commented Oct 29, 2017

mmodrow commented Oct 18, 2017 •

edited

Loading

mmodrow Oct 18, 2017 •

edited

Loading

mmodrow commented Oct 18, 2017 •

edited

Loading

mmodrow commented Oct 18, 2017 •

edited

Loading