#911 ✓ staged
Bobby Richter

Track events still appear after end time.

Reported by Bobby Richter | February 9th, 2012 @ 08:41 PM | in 1.2 (closed)

I've been fighting with Butter all night, and I finally managed to back the problem into a corner. Looks like Popcorn is the culprit:

      var p = Popcorn( "#main" );
      p.text({ start: 0, end: 3, text: "test", target: "Area1" });
      var id1 = p.getLastTrackEventId();
      p.footnote({ start: 1, end: 2, text: "boof", target: "Area2" });
      p.removeTrackEvent( id1 );
      p.play();

      var byStart = p.data.trackEvents.byStart[ 1 ],
          byEnd = p.data.trackEvents.byEnd[ 1 ];

      console.log( byStart._id, byStart.start, byStart.end, p.data.trackEvents.byStart.length );
      console.log( byEnd._id, byEnd.start, byEnd.end, p.data.trackEvents.byEnd.length );

This code will create two events, and delete the first one. Presumably, there is no trace of the first event. However, the console will read [something similar to] this:

footnote1328837449838 1 2 2
test.html:57text1328837449832 0 3 2

Deciphering my horrible printout reveals the following:
1. The last number is 2 in both cases, so the lengths of the byStart and byEnd array are equal and correct.
2. byStart looks like it's right, retaining the correct event data.
3. byEnd looks like it held onto the event that I deleted.

Why...?! Is my syntax wrong somewhere?! Is this a bug!? Someone... help my sanity. Too many hours staring at code today.

Comments and changes to this ticket

  • Bobby Richter

    Bobby Richter February 9th, 2012 @ 08:47 PM

    Also, if JSFiddle ever stops exploding tonight, I made this: http://jsfiddle.net/FDSPh/2/

  • Jon Buckley

    Jon Buckley February 9th, 2012 @ 08:56 PM

    And if you examine all of the array data in your web console, you come up with this: http://dl.dropbox.com/u/4403845/Screenshots/2t.png

  • Scott Downe

    Scott Downe February 9th, 2012 @ 09:05 PM

    check out #907, very very similar.

  • Scott Downe

    Scott Downe February 9th, 2012 @ 09:06 PM

    To answer all your questions. It is a bug, syntax is correct, and this makes perfect sense to me.

  • Bobby Richter

    Bobby Richter February 10th, 2012 @ 09:49 AM

    Scott, is this a duplicate then? Regardless of the attack vector, I think it might be the same bug.

  • Bobby Richter

    Bobby Richter February 10th, 2012 @ 09:50 AM

    • State changed from “new” to “bugs”
  • Scott Downe

    Scott Downe February 10th, 2012 @ 01:37 PM

    • State changed from “bugs” to “assigned”
    • Assigned user set to “Scott Downe”

    It's not a dupe. One bug was in removePlugin, the other in removeTrackEvent.

    One bug is also already in review.

    If they were found at the same time, I would of put them together though.

    I am amazed both of these bugs lasted this long, tbh.

    I am going to own this, but I'll do it after IE8 is done, so if someone else wants to do it sooner be my guest.

  • Rick

    Rick February 10th, 2012 @ 02:50 PM

    • Assigned user changed from “Scott Downe” to “Rick”

    Addressing this is probably more important then IE8 "almost" support. I can take it over.

  • Rick

    Rick February 10th, 2012 @ 03:06 PM

    Update, the bug appears to be caused by assumptions made about "indexWasAt"

    EDIT: Just Kidding, it was caused by assumptions made about the current index in the trackEvent iteration

  • Rick

    Rick February 10th, 2012 @ 03:54 PM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user changed from “Rick” to “Scott Downe”

    Character removal patch FTW.

    https://github.com/rwldrn/popcorn-js/commits/t911

    git pull git://github.com/rwldrn/popcorn-js.git t911

  • Rick

    Rick February 10th, 2012 @ 03:55 PM

    I used the following code:

    var testEnd;
    for ( var k = 0; k < obj.data.trackEvents.byEnd.length; k++ ) {
      testEnd = obj.data.trackEvents.byEnd[k];
    
      if ( testEnd._id ) {
        console.log( deepEqual( testEnd, o, "testEnd, o" ) );
      }
    }
    

    Placed at line 954 to test and determine that the track event object in the byStart and byEnd arrays are in fact the same object reference. This allows us to simple push the current object in the iteration into the "saved" track events, instead of incorrectly using the the index of the iteration

  • Scott Downe

    Scott Downe February 10th, 2012 @ 07:00 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “Scott Downe” to “Jon Buckley”

    Awesome!

    "Character removal patch FTW"... FTW!

    Jon, you look like you could use some more work to do :P

  • Scott Downe

    Scott Downe February 10th, 2012 @ 07:17 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “Jon Buckley” to “Rick”

    Actually, I think I found a bug with this.

    It does manage to remove the correct track event, but in the process it re orders the end array.

      Popcorn.plugin( "a", {
        _setup: function( options ) {},
        start: function( event, options ) {},
        end: function( event, options ) {},
        _teardown: function( options ) {}
      });
    
      Popcorn.plugin( "b", {
        _setup: function( options ) {},
        start: function( event, options ) {},
        end: function( event, options ) {},
        _teardown: function( options ) {}
      });
    
      Popcorn.plugin( "c", {
        _setup: function( options ) {},
        start: function( event, options ) {},
        end: function( event, options ) {},
        _teardown: function( options ) {}
      });
    
      Popcorn.plugin( "d", {
        _setup: function( options ) {},
        start: function( event, options ) {},
        end: function( event, options ) {},
        _teardown: function( options ) {}
      });
    
      pop.a({
        start: 1,
        end: 5
      });
    
      // Store track event id for "plugin a"
      aId = pop.getLastTrackEventId();
    
      pop.b({
        start: 3,
        end: 4
      });
    
      // Store track event id for "plugin b"
      bId = pop.getLastTrackEventId();
    
      pop.c({
        start: 0,
        end: 3
      });
    
      pop.d({
        start: 1,
        end: 2
      });
    
    
      // Shorthand references
      byStart = pop.data.trackEvents.byStart;
      byEnd = pop.data.trackEvents.byEnd;
    
    console.log( byStart ); // before the removal, byStart's first element is c
    console.log( byEnd ); // before the removal, byEnd's first element is d
    
      // Remove the first track event for "plugin a"
      pop.removeTrackEvent( aId );
    
      // Shorthand references
      byStart = pop.data.trackEvents.byStart;
      byEnd = pop.data.trackEvents.byEnd;
    
    console.log( byStart ); // after the removal, byStart's first element is c
    console.log( byEnd ); // after the removal, byEnd's first element is c, when it should be d
    

    Another way to say it is the end array has been made to look just like the start array.

  • Rick

    Rick February 10th, 2012 @ 08:10 PM

    Ok, I have a bit more of a brute force approach I can try. Thanks for spotting that

    Rick

  • Rick

    Rick February 11th, 2012 @ 06:01 PM

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

    https://github.com/rwldrn/popcorn-js/commits/t911

    git pull git://github.com/rwldrn/popcorn-js.git t911

  • Scott Downe

    Scott Downe February 14th, 2012 @ 10:36 AM

    Can someone else grab this for the time being? I feel a bit swamped until the end of the week. By that time, I can do a SR on this.

  • Scott Downe
  • Scott Downe

    Scott Downe February 14th, 2012 @ 02:39 PM

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

    Sorry for the delay with this.

    I think this looks great.

    Everything works, fixed, and with tests.

  • Rick

    Rick February 14th, 2012 @ 02:53 PM

    No problem, thanks for the review

  • cadecairos

    cadecairos February 23rd, 2012 @ 03:12 PM

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

    This looks solid, code style us good, passes lint and all core unit tests passing on Firefox 10, Chrome 17, Safari 5.2, Opera 11.61

    SR+

  • cadecairos

    cadecairos February 23rd, 2012 @ 03:14 PM

    • State changed from “review-looks-good” to “staged”
    • Assigned user changed from “cadecairos” to “Rick”

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

Pages