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