#723 ✓ staged
David Seifried

Implement static and proto .destroy() methods for complete event and instance cleanup

Reported by David Seifried | September 16th, 2011 @ 11:54 AM | in 0.9 (closed)

As per #710, the timeUpdate listener is never removed when an instance is removed and we should look into this.

Comments and changes to this ticket

  • David Humphrey

    David Humphrey September 16th, 2011 @ 12:06 PM

    As far as I can tell, the only thing we leak when a popcorn instance is destroyed is one listener for timeUpdate, since it gets added internally, and therefore can't be removed from outside.

    Earlier discussions happened wrt doing this in removeInstance(), which was wrong because that simply removes the instance from a list vs. destroying it.

    One option is to add a .destroy() method to the instance, which removes this listener. Maybe there are others.

    We might also decide that this isn't an issue, and that popcorn instances are meant to be long-lived (e.g., duration of document).

    I'm not sure what to think.

  • brianchirls

    brianchirls September 16th, 2011 @ 12:12 PM

    I believe there are also event listeners added by trigger that need to be removed as well.

  • David Seifried

    David Seifried September 16th, 2011 @ 12:26 PM

    Cool I will look into this today an update everyone

  • Rick

    Rick September 16th, 2011 @ 01:32 PM

    I'm definitely down for something like...

    
    // Static method allows calls passing any instance
    Popcorn.destroy = function( instance ) {
     // complete annihilation!
    }; 
    
    // Accessible as a proto method
    Popcorn.prototype.destroy = function() {
      
      Popcorn.destroy( this );
      // don't return, because references are dead
    };
    

    Note: trigger() definitely doesn't add any event listeners

  • brianchirls

    brianchirls September 16th, 2011 @ 01:36 PM

    My bad. I meant listen, not trigger.
    https://github.com/jbuck/popcorn-js/blob/develop/popcorn.js#L647

    +1 Rick's destroy syntax. In particular, the "complete annihilation!" comment.

    Agreed that Popcorn.prototype.destroy should not return itself. It might be useful to return the instanceId, but I'm not married to it.

  • Rick

    Rick September 16th, 2011 @ 01:44 PM

    As for listen, yep that's its job. We'll have to iterate all of the events that the instance has stored in instance.data.events and remove them one at a time with instance.media.removeEventListener( type, handler, capture ) (this wil be faster then removing them with instance.unlisten( type )... which would do the same thing anyway... )

    No reason to return anything at all, since it will be gone from memory and there will be nothing to find with the instanceId anyway.

  • David Humphrey

    David Humphrey September 16th, 2011 @ 01:55 PM

    Rick, this is sounding like a good approach to me. +1

  • David Seifried

    David Seifried September 16th, 2011 @ 02:25 PM

    Yea I essentially found out what Rick outlined above. The timeUpdate event is on the media objects so we are going to have to remove them the way Rick said. I am going to start working on this today

  • Rick

    Rick September 16th, 2011 @ 02:52 PM

    David (Seifried)... One good test for this is to attach a listener with an assertion in it, then make a sep. reference to the actual dom element and try firing an event on it after destroy() has been called on the popcorn instance. If that assertion runs, it should fail the unit by exceeding the expect() count. I'm typing into a tiny textarea on my phone,so if this is hard to follow, I apologize :) another way to think of the test: we want try catching the element doing something it shouldn't be able to do. Hope this is helpful :)

  • Rick

    Rick September 16th, 2011 @ 03:41 PM

    • Title changed from “Explore way of removing timeUpdate listener so it doesn't remain when instance is destroyed.” to “Implement static and proto .destroy() methods for complete event and instance cleanup ”
  • David Seifried

    David Seifried September 19th, 2011 @ 01:18 PM

    Ive pretty much got this done, but I ran into one issue. There is a possibility that the user will attempt to call destroy before the video has finished loading, or passing a sufficient readyState. THe problem with this is that the timeUpdate event listener isnt added until after readyState has passed 2.

    Im wondering if something like a setTimeout would be good enough ( tho it is pretty dirty, as if the video takes a long time to load on a slow network, its gonna to be spinning in destroy for a while ).

    Thoughts?

  • Rick

    Rick September 19th, 2011 @ 01:48 PM

    Interesting... is this present in your test cases for the implementation? If you push up what you have, I'll def take a look and see if I can help at all.

  • David Humphrey

    David Humphrey September 19th, 2011 @ 02:30 PM

    I don't think relying on setTimeout is good when we can avoid it. You could just as easily provide a flag via an internal variable (isDestroyed maybe), and then make sure your invariants match when doing things later (e.g., bail or throw in cases where isDestroyed === true).

  • Scott Downe

    Scott Downe September 20th, 2011 @ 08:37 AM

    Also, consider when you enter destroy, and isDestroyed is false, set it to true, and also do an isDestroyed check in the event listener assignment to not add it, so destroy can safely be called before the event is ready.

  • David Seifried

    David Seifried September 20th, 2011 @ 10:19 AM

    Here is what I have so far. I did something similar to the flag, but just straight up accessed the readyState instead and threw an error if the problem occured.

    https://github.com/dseif/popcorn-js/commits/t710

    I added a file ( which will be removed before I actually put up for review ) testDestroy.html that displays what im talking about. I added a console.log in to display what I mean. Initially click play, let play for a second or so, pause, click destroy, click play again, and no more console.logs are being fired ( listener was removed ). If you uncomment the popcorn.destroy() line in testDestroy.html and run again, the error will be thrown as it is not ready yet.

    Also, in order to remove the event listener for timeUpdate that was added by core upon instantiation of a new popcorn instance, I needed to store a reference to that function somewhere in order to remove it later. I attached this to instance.data. If someone else has a better idea for where it can go let me know.

  • David Seifried

    David Seifried September 20th, 2011 @ 11:36 AM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “Rick”

    Alright after making a few changes Rick suggested im gonna put this into review.

    https://github.com/dseif/popcorn-js/commits/t710

    Assigning to Rick for PR.

  • David Seifried

    David Seifried September 21st, 2011 @ 11:14 AM

    Sorry for putting this in a wrongly named branch. Its all in a correctly named one now.

    https://github.com/dseif/popcorn-js/tree/t723

  • Rick

    Rick September 21st, 2011 @ 04:22 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “Rick” to “David Seifried”

    The tests you wrote are really smart - great work :)

    
    Checking Popcorn against JSLint...
    Linting popcorn.js
    
                }
    
    Problem at line 266 character 14: Missing semicolon.
    
  • David Seifried

    David Seifried September 22nd, 2011 @ 10:47 AM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “Rick”

    Fixed linting error and removed the Popcorn.removeInstance method that I was calling before, as that will be gone soon if it isn't already.

    https://github.com/dseif/popcorn-js/tree/t723

  • Rick

    Rick September 22nd, 2011 @ 11:37 AM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “Rick” to “David Seifried”

    Remove these lines...

    
    Popcorn.instances.splice( Popcorn.instanceIds[ instance.id ], 1 );
    
    delete Popcorn.instanceIds[ instance.id ];
    
  • David Seifried

    David Seifried September 22nd, 2011 @ 11:42 AM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “Rick”

    Alright removed, thanks Rick.

    https://github.com/dseif/popcorn-js/tree/t723

  • Rick

    Rick September 22nd, 2011 @ 11:54 AM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “Rick” to “cadecairos”

    Awesome!

    PR+

    Tested: core test suite;

    Passing in:

    • FF 3.6 (stable), 6.x (stable), 7.x (Aurora), 8.x (Nightly)

    • Chrome 14 (Stable), 15 (Beta), 16 (Canary)

    Lint Passes

  • cadecairos

    cadecairos September 22nd, 2011 @ 03:38 PM

    code styling is good.

    Tested:
    Core unit test suite

    On:
    Firefox 6, 7, Aurora
    Chrome 13

    All passing

  • cadecairos

    cadecairos September 22nd, 2011 @ 03:39 PM

    • State changed from “super-review-requested” to “review-looks-good”
  • cadecairos

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Referenced by

Pages