Bug 210113

Summary: [EME][GStreamer] onencrypted should be dispatched iff readyState >= HAVE_METADATA
Product: WebKit Reporter: Charlie Turner <cturner>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: bugs-noreply
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 210644    
Bug Blocks:    
Attachments:
Description Flags
Event diagram showing race condition. none

Description Charlie Turner 2020-04-07 08:10:13 PDT 
Currently it's dispatched as soon as GStreamer notifies us about discovering the relevant CENC boxes in the demuxer, which happens very early. In MSE, this event races with onupdateend and breaks tests like this,
https://github.com/web-platform-tests/wpt/blob/master/encrypted-media/util/testmediasource.js#L37
which is used like so,
https://github.com/web-platform-tests/wpt/blob/master/encrypted-media/scripts/reset-src-after-setmediakeys.js#L28

The high-level goal of this test is to set video.src to a new blob, receive the two expected encrypted events, reset the video src using the same blob (like a heavy restart) and check the two events are received again.

The problem is, the test assumes that updateend's must complete before the onencrypted events. It's not that easy finding out if this is correct using paragraph and verse from the specs... But, I noticed a bug in EME. The spec does say onencrypted should only be dispatched if readyState >= HAVE_METADATA, which would be a completed preroll for GStreamer and hence the appends would have completed, so fixing this seems like it will fix the entire issue.

However... The state machine for MSE as been copied from the base player, and then grown in different ways over the years. This has resulted in a complicated web of side-effects updating the readyState. So, this task is now a bit larger: to refactor the state machines in the base player, and the MSE player (ideally, if possible, unify them) and have a single place where the readyState is updated so that we can fire various cached data at the appropriate positions of readyState.
Comment 1 Charlie Turner 2020-04-07 08:14:07 PDT 
Created attachment 395683 [details]
Event diagram showing race condition.

The append splits into two varied operations, one to append the audio, one the video. If onencrypted happens as (1), then the readyState can be closed before both updates have ended, breaking the test. If it happens as (2), the test passes. This is a classic race conditioning, most likely resulting from the onencrypted event being dispatched too soon.
Comment 2 Diego Pino 2021-06-09 00:20:32 PDT 
The only test filed under this bug was:

  - imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-reset-src-after-setmediakeys.https.html

GTK post-commit test bot reports the test has been failing consistently for the last 4000 revisions.

Diff:

--- /home/buildbot/worker/gtk-linux-64-release-tests/build/layout-test-results/imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-reset-src-after-setmediakeys.https-expected.txt
+++ /home/buildbot/worker/gtk-linux-64-release-tests/build/layout-test-results/imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-reset-src-after-setmediakeys.https-actual.txt
@@ -1,3 +1,5 @@
 
-PASS Reset src after setMediaKeys().
+Harness Error (FAIL), message = Test named "Reset src after setMediaKeys()." passed a function to `async_test` that returned a value. Consider using `promise_test` instead when using Promises or async/await.
 
+TIMEOUT Reset src after setMediaKeys(). Test timed out
+