websites/videos.bentasker.co.uk#5: Div based embedding doesn't handle multi-video pages well

I'm guessing, somewhere in the flow, we end up using a global for state, so you end up with a race where one video prevents the other.

That wouldn't be an issue with the legacy embed because things are run at the point of embed rather than all at once at the end.

I've set up a repro at https://www.bentasker.co.uk/posts/blog/general/stop-the-invasion-tmp.html (I needed to switch the original back to the legacy embed until this can be resolved).

In prepareVars() we do this

    var vidid = 0;
    while (true){
        if (window.BensPlayerInstances.includes(vidid)){
            vidid++;
        }else{
            window.BensPlayerInstances.push(vidid);
            break;
        }
    }    

So I wondered if that was perhaps racey and we were reallocating Video IDs.

That ID ultimately gets written into the div ID.

Looking at the repro though, the first video (working) has

<div class="BensVideoCont" data-src="2022/shelling_in_ukraine_l/shelling_l.mp4_master.m3u8" id="BensplayerWrapper1">

Whilst the next (not working) still has the original embed markup

<div class="embedBensPlayer" data-src="2022/sakhaschyk_speech/sakhaschyk.mp4.m3u8"></div>

Suggesting it's been skipped entirely.

That's a little odd, because it's the entrypoint (embedBensPlayerDivs()) which updates the attributes

function embedBensPlayerDivs(){
    var ret, vidurl, vidtype, vidid;

    var eles = document.getElementsByClassName('embedBensPlayer');

    for (var i=0; i<eles.length; i++){
        vidurl = eles[i].getAttribute('data-src');

        if (!vidurl){
            continue;
        }

        ret = prepareVars(vidurl);
        vidtype = ret[0]
        vidid = ret[1]
        vidurl = ret[2]

        // Update the container to use our desired ID and class
        eles[i].setAttribute("id", "BensplayerWrapper" + vidid);
        eles[i].setAttribute("class", "BensVideoCont");

        // Setup the player
        setupPlayer(vidtype, vidurl, vidid);
    }
}

Actually, just to make things slightly odder, looking at the ID's that have been assigned, the ordering is a little unexpected

• Video 1 (working) : ID 1
• Video 2 (not working) : No ID
• Video 3 (working) : ID 2
• Video 4 (working) : ID 0

The script embed.min.js is referenced multiple times, so I wonder if we're getting a race based on that?

I'm going to update the repro page to use embed.vnext.js instead so we can drop some logging in

OK, this time we're using vnext.

Video load state is:

• Video 1 (working): ID 1
• Video 2 (not working) : No ID
• Video 3 (not working) : ID 2
• Video 4 (working) : ID 0

odd, we've got 3 working videos again... It's possible relative latency factors into this, I was testing delivery over I2P when I first noticed this issue, and there were definitely only 2 loading then.

I put a bunch of console.log() statements in, so in the JS console we have

Queing video detection
embedBensPlayerDivs fired

We're not somehow accidentally firing multiple times then - it gets queued once and fired once.

There's a console.log() dumping out the result of the getElementsByClassName call before anything else happens

HTMLCollection { 0: div.embedBensPlayer, 1: div.embedBensPlayer, 2: div.embedBensPlayer, length: 3 }

There's only 3 there...

Running the function manually picks up on it

embedBensPlayerDivs fired embed.vnext.js:187:13
HTMLCollection { 0: div.embedBensPlayer, length: 1 }

So, our getElementsByClassName() call isn't returning the missing video. Weird...

The embedder is supposed to fire on DOM Ready, there's no evidence that isn't happening. But, even if it were - it doesn't really make sense that a middle video would get missed but later ones would not.

What the fuck?

I wonder if we get the same effect using document.querySelectorAll()?

My repro now isn't reproing though....

Ah, I missed one of the embeds, so that was still using the original code - probably explains why I was getting 3 working videos instead of 2. Also explains why there were only 3 in the dumped collection.

Ok, lets run through this again.

Page refreshed, state is

• Video 1 (working): ID 0
• Video 2 (not working) : No ID
• Video 3 (working): ID 1
• Video 4 (not working) : No ID

In the JS console, we have 1 set of

Queing video detection
embedBensPlayerDivs fired

However, our dumped HTML Collection includes all elements

HTMLCollection { 0: div.embedBensPlayer, 1: div.embedBensPlayer, 2: div.embedBensPlayer, 3: div.embedBensPlayer, length: 4 }

We only get 2 instances of

Player Ready

So, our selector is getting all the elements, but for some reason we're not operating on 2 of them.

If I run embedBensPlayerDivs() then we get

HTMLCollection { 0: div.embedBensPlayer, 1: div.embedBensPlayer, length: 2 }

And a single Player comes up (the first div).

So, it looks like we're only operating on odd divs - any even one gets skipped

    for (var i=0; i<eles.length; i++){
        vidurl = eles[i].getAttribute('data-src');

        if (!vidurl){
            console.log("No url, skipping");
            console.log(eles[i]);
            continue;
        }

        ret = prepareVars(vidurl);
        vidtype = ret[0]
        vidid = ret[1]
        vidurl = ret[2]

        // Update the container to use our desired ID and class
        eles[i].setAttribute("id", "BensplayerWrapper" + vidid);
        eles[i].setAttribute("class", "BensVideoCont");

        // Setup the player
        setupPlayer(vidtype, vidurl, vidid);
    }

I wonder if there's an i somewhere that's leaking scope?

Before though, lets get that loop to log the element it's working on so we can verify it never actually operates on this missing ones (though I'm fairly sure that's the case).

On refresh,

Queing video detection embed.vnext.js:527:13
embedBensPlayerDivs fired embed.vnext.js:187:13
HTMLCollection { 0: div.embedBensPlayer, 1: div.embedBensPlayer, 2: div.embedBensPlayer, 3: div.embedBensPlayer
, length: 4 }
<div class="embedBensPlayer" data-src="2022/shelling_in_ukraine…lling_l.mp4_master.m3u8">
<div class="embedBensPlayer" data-src="2022/russian_pow_describ…ces/pow.mp4_master.m3u8">
Player Ready
Player Ready

So the loop is indeed skipping those elements

It's not i - that's explicitly declared as local scope in the function, and in any case there's no other i in embed.js.

This is going to be something really dumb. I just don't see it yet.

Now, this is interesting (and probably the answer), I've dumped some more console.log() into the loop

    for (var i=0; i<eles.length; i++){
        vidurl = eles[i].getAttribute('data-src');
        console.log(eles[i]);


        console.log("1: " + i + " " + eles.length);
        if (!vidurl){
            console.log("No url, skipping");
            console.log(eles[i]);
            continue;
        }

        ret = prepareVars(vidurl);
        console.log("2: " + i+ " " + eles.length);
        vidtype = ret[0]
        vidid = ret[1]
        vidurl = ret[2]

        // Update the container to use our desired ID and class
        eles[i].setAttribute("id", "BensplayerWrapper" + vidid);
        eles[i].setAttribute("class", "BensVideoCont");
        console.log("3: " + i+ " " + eles.length);

        // Setup the player
        setupPlayer(vidtype, vidurl, vidid);
        console.log("4: " + i+ " " + eles.length);
    }

So it should show the current value of i (in case it increments) and the length of eles

If we run it, we get HTMLCollection { 0: div.embedBensPlayer, 1: div.embedBensPlayer, 2: div.embedBensPlayer, 3: div.embedBensPlayer , length: 4 } <div class="embedBensPlayer" data-src="2022/shelling_in_ukraine…lling_l.mp4_master.m3u8"> 1: 0 4 embed.vnext.js:195:17 2: 0 4 embed.vnext.js:203:17 3: 0 3 embed.vnext.js:211:17 4: 0 3 embed.vnext.js:215:17 <div class="embedBensPlayer" data-src="2022/russian_pow_describ…ces/pow.mp4_master.m3u8"> 1: 1 3 embed.vnext.js:195:17 2: 1 3 embed.vnext.js:203:17 3: 1 2 embed.vnext.js:211:17 4: 1 2

I know what the answer is now.

The result of getElementsByClassName is live, so when we overwrite the class of the div, it drops out of eles. The result is that eles gets shorter - but, of course, i still increments.

As a result, the element that was in position 1 is now in position 0, so doesn't get processed.

We need to append the new class rather than make it the only one (also need to make sure nothing later on overwrites it, though I don't think it does).

Switched to using

eles[i].setAttribute("class", "BensVideoCont " + eles[i].className);

and that's done the job.

I'll just test this over I2P (higher latency) and then I'll look at getting the change committed