Unsafe key session removal. Regression since 4.7.2

[alexhmit]

Environment

• The MPD passes the DASH-IF Conformance Tool on https://conformance.dashif.org/
• The stream has correct Access-Control-Allow-Origin headers (CORS)
• There are no network errors such as 404s in the browser console when trying to play the stream
• The issue observed is not mentioned on https://github.com/Dash-Industry-Forum/dash.js/wiki/FAQ
• The issue occurs in the latest reference client on http://reference.dashif.org/dash.js/ and not just on my page

• Link to playable MPD file: Any MPD containing DRM
• Dash.js version: 4.7.2 or later
• Browser name/version: WebKit
• OS name/version: OEM

Steps to reproduce

This behavior is not stable. May or may not occur on a particular system. Please review the code change at https://github.com/Dash-Industry-Forum/dash.js/pull/4239/files#diff-8260cad47ef106d2b2ff4036c1c72af9333181ac995f57bdb92af5c8c91b86b7. This was introduced by PR #4239.

Priori to 4.7.2 the key session were removed in the following way:

for (let i = 0; i < sessions.length; i++) {
            session = sessions[i];
            if (!session.getUsable()) {
                _closeKeySessionInternal(session).catch(function () {
                    removeSession(session);
                });
            }
        }

removeSession is called in catch() only. This was changed to

                _closeKeySessionInternal(session)
                removeSession(session);

This will call removeSession unconditionally and NOT waiting for the resolution of _closeKeySessionInternal(). This can cause incoherence in underlying session object removal and session close() calls.

Maybe a better way is something like

_closeKeySessionInternal(session).finally(function () {
                    removeSession(session);
                });

This removes session when the promise resolves without error as well.

Observed behavior

On some systems this problematic code can lead to memory corruption and/or leak.

Console output

No useful logging for this since it is causing native area memory issues. I believe code review should be sufficient.

Expected behavior

Media key sessions removed correctly and resources reclaimed.

[alexhmit]

I just noticed some of the previous comments disappeared include the one you requested a suggested change and my response to it.

Maybe the info was transferred to elsewhere. I will repeat them here.

The bug is actually more serious then I first thought. Because of the sessions array is modified (spliced by removeSession), the loop iteration will fail except the case where the removal is the last element.

In our patch, we basically reverse the iteration to avoid this problem, (There might be a better way.)

For example, instead of

for (let i = 0; i < sessions.length; i++) {
one can do:

for (let i = sessions.length -1; i >= 0; i--) {

This problem occurs in function reset() and stop() in file src/streaming/protection/models/ProtectionModel_21Jan2015.js

Thanks

Alex

[dsilhavy]

I checked this in more detail, and it looks like this is mainly a problem caused by a Chrome bug: https://issues.chromium.org/issues/40707308?pli=1

The problem is that the promise returned by session.close() is never rejected or resolved for key sessions that are not usable or do not contain any key status. This is why we can't use .finally here as well. It is simply never called in the tests I did.

The only workaround I see right now is to use Promise.race() together with a timeout in _closeKeySessionInternal. Example:

    function _closeKeySessionInternal(sessionToken) {
        if (!sessionToken || !sessionToken.session) {
            return Promise.resolve;
        }
        const session = sessionToken.session;

        // Remove event listeners
        session.removeEventListener('keystatuseschange', sessionToken);
        session.removeEventListener('message', sessionToken);

        // Send our request to the key session
        return Promise.race([
            session.close(),
            new Promise((resolve, reject) => {
                setTimeout(reject, 500)
            })]);
    }

However, this has implications when calling attachSource() with a new MPD. We would need to make MediaPlayer._resetPlaybackControllers async as well in order to wait for all key sessions to be closed before attaching a new MPD. Otherwise, we run into the issue described here: #4238.

Another thing is that this has implications on the startup time, as we have to wait for our artificial timeout to complete before starting playback of the new content. Otherwise, we use an invalid key session if keepProtectionMediaKeys is set to true. On the other hand it is cleaner to wait for everything to be reset before starting playback of a new content.

I have created a first version of a potential fix in #4478. It is based on v4. It would be great if people have feedback on this. @alexhmit , @bbert

If changing the for loop like this is sufficient, we might also use this as a workaround for now until Chrome fixes the issue, and we can work with promises again:

for (let i = sessions.length -1; i >= 0; i--) {
...