Flash tech breaks event handlers and leaks memory

[vasklund]

After debugging a Cannot read property 'vdata1424889252489' of null error I found what seems to be a problem with how the Flash tech is initialized. Essentially, because the Flash tech overwrites this.el_ in two places (init and embed), not copying over the expando ID to the new element, the data associated with the old element is lost.

This seems to have some pretty nasty implications, because event handling functions (vjs.on, vjs.off, vjs.cleanUpEvents, vjs.trigger) rely on data stored in the expando.

A broken example scenario already in the Video.js core:

1. a new player with Flash tech is initialized
2. MediaTechController.initControlsListeners sets up two listeners on the player (addControlsListeners and removeControlsListeners)
3. source changes and tech changes from Flash to Html5
4. Flash tech is disposed
5. Component.dispose (parent of Flash tech) tries to deregister all event handlers, but misses the ones set up between Component.init and Flash.init - this includes cleanup handlers set up in Component.on --> memory leak
6. new tech and source are loaded
7. player.controls(false) generates an error, because the old MediaTechController.removeControlsListeners is still hooked up, but the original element doesn't exist anymore
8. the correct MediaTechController.removeControlsListeners (the one for the new tech) is never fired, because of the error

Super simple test case (have to run the output in it's own window):
http://jsbin.com/cigihufico/2/edit?html,js

The best solution I can think of would be to reuse the existing element throughout Flash.init and Flash.embed (don't use placeHolder), but I guess there's a reason that's not possible? Copying over the expando ID to the new element also seems to work, although that might not completely remove all references to the element?

This is probably the real solution to #1793 (comment), and might also help with #1505 (which I suspect is still not fixed). #1859 is also related.

[mmcc]

Thanks for the in depth writeup on this one. @videojs/contributors thoughts on the proposed solutions?

[dmlap]

It seems like placeHolder is a remnant of the old iframe embed mode that didn't get removed completely. I'd like to update the code to construct the <object> on init and do away with placeholder stuff entirely, which I think is what @vasklund is proposing.

This is a great writeup of a complex issue. Thanks for the analysis from me too, @vasklund!

[heff]

Yeah, the placeholder piece is definitely a remnant, possibly even from when SWFObject (external lib) was being used to create the object. I believe that project required you to pass it a placeholder.

I can't see any issues with updating the Flash tech to have a createEl function that just builds the object from the start.

[vasklund]

I have a potential fix ready for this issue, but am having problems writing good, specific tests. The problem, as discussed earlier, is that the Flash tech replacing this.el_ in init and embed leaves parents up the hierarchy (MediaTechController, Component) with old references to this.el_, so a good test would need to initialize not only the Flash tech (e.g. new vjs.Flash(playerMock, {})), but invoke the whole class inheritance (e.g. through creating a new Player).

I found the PlayerTest.makePlayer() helper but I couldn't get it to work for this scenario, probably because PhantomJS doesn't support Flash?

If anyone has any ideas, I'm listening - I should be able to provide a pull request as long as I have some method of testing through the inheritance chain.

[saxena-gaurav]

@vasklund : I can help you in writing unit tests for your potential fix related to this problem. Let me know if you want to pair up.

[vasklund]

@saxena-gaurav: Sounds great! I've got a fork up at https://github.com/vasklund/video.js/commits/flash-dispose-fix if you want to take a look. What timezone are you in?

[saxena-gaurav]

@vasklund : Thanks so much for the bug fix.

I added the unit test. #2125