Shaka-player: Calling Player#unload followed by Player#load allows loading to proceed even though Player#mediaSource_ and Player#mediaSourceOpen_ are null

[chrisfillmore]

Hello!

As part of our application's preparation process before playback, we call Player.prototype.unload, then perform some other work before calling Player.prototype.load. We have observed this produces a race condition when you attempt to load content _while content is currently being played_. Since load _also_ calls unload, it is possible for the player to get into a state where both this.mediaSource_ and this.mediaSourceOpen_ are null, but the loading process is allowed to proceed. This is because when load calls unload, which in turn calls resetStreaming_ it observes that nothing is being played and simply returns Promise.resolve().

I _believe_ we exclusively see this problem with protected content. I was able to produce it in the Shaka demo app with this content:

• Sintel 4k (multicode, Widevine)
• Tears of Steel (Widevine)

Simply add a call to unload before load in shakaDemo.load:

player.unload();
// Load the manifest.
player.load(asset.manifestUri).then(function() {
...

Steps to reproduce in the demo app (with the above change):

• Select content for playback
• Press Load
• Observe video is playing
• Press Load again

Observed behaviour: Assertion failed, and Cannot read property 'readyState' of null. (The MediaSource object being passed into the MediaSourceEngine constructor is null.)

Expected behaviour: Player should load video normally.

The proposed fix is to store the unloading process on the player as a Promise. When unload is called, check if the player is already unloading. If so, do not initiate a new unloading process, but instead continue with the existing one.

I have submitted PR #613

I wrote a test in the player_integration suite which correctly fails, though it is not included in the PR. Truthfully, I am not aware of the prevailing wisdom around testing for race conditions, and at any rate the test would need some more work before being included.

The PR is accepted by the compiler, however it nukes a few player_unit tests. I am unsure about how to proceed with regard to these now-failing tests and am open to feedback/suggestions.

[joeyparrish]

Testing race conditions is tricky. I recommend a unit test rather than an integration test, because it will be easier to control the environment. Everything other than the object under test (Player) should be mocked in a unit test.

[chrisfillmore]

Hi Joey, I need some advice on how to proceed.

I think it's important that the test load real content. If I just write this:

it('handles load interrupting unload', function(done) {
  player.load('', 0, factory1).then(function() {
    player.unload();
    return player.load('', 0, factory1);
  }).then(done).catch(fail);
});

...then the test does not fail when it should. The original test I wrote connected to a development server in our office to load content, and that correctly failed. I've been trying to understand the bootstrapping process that the Shaka integration suite uses to create manifests for the local test content (I've been looking at player_integration.js and test/util/test_scheme.js). This seems to me like the way to go with this test, but I also get the feeling I'm just missing something. Is it possible to mock content?

Thanks for your help.

[joeyparrish]

If you can only reproduce the bug with real content, we have several simulated streams in test/test/util/test_scheme.js that can be used in an integration test (player_integration.js).

Alternately, if you can show me how to reproduce your error in our demo app, I would be happy to investigate and try to fix it myself.

[chrisfillmore]

Hi Joey, sorry to be out of touch. Yes it looks like circumstances only align correctly to produce the bug when the player goes through a real unloading process. You can reproduce it in the demo app with the steps I listed in the issue above (you just have to add the call to player.unload() before calling load).

I'm happy to write the test myself. We rely on Shaka extensively so my familiarity with the code base and test suite is of value to me and my employer. We have an unresolved ticket to return to the main Shaka branch once various PR's are merged, so, eventually I'll write this test, but I have a few other pressing things right now.

[joeyparrish]

@chrisfillmore, I added player.unload(); in demo/assets.js right before player.load(...) and I am unable to reproduce any error.

[joeyparrish]

I take it back. I kept toying with it, and after several tries it finally blew up. For me, it fails if I select "Dig the Uke", click "Load", wait a second or two, then click "Load" again.

[joeyparrish]

The fix for this has just been released in v2.0.3.