Fix off by 1 issue in when relying on keySystems[].maxSessionCacheSize
#1565
Conversation
peaBerberian
added
bug
`keySystems[].maxSessionCacheSize', which allows to limit the number of created `MediaKeySession`, was relied on before creating a new `MediaKeySession` (this is good I guess). However it considered if we were over the limit at that instant, were in reality the logic should probably also account for the `MediaKeySession` that will be created just after. The current behavior leads to having a supplementary `MediaKeySession` than what `keySystems[].maxSessionCacheSize` authorizes. To fix it, I simply remove `1` from the limit we want to enforce in that call, unless removing `1` would lead to a negative number as this has a special semantic (which is to have no limit). This still works for `0` like it worked before (which is strangely equivalent to setting it to `1` in our logic, but setting it to `0` is just asking for bugs anyway I guess :p). NOTE: We could have a plausible justification for our precedent behavior that it was only setting the "cache size" and thus excluding the last "actually relied on" one, but this is not how it is documented in the API documentation, so to me this is a bug.
peaBerberian
force-pushed
the
fix/maxSessionCacheSize-off-by-one
branch
from
9953ddb
to
608ecbb
Compare
| @@ -30,7 +30,7 @@ export default async function cleanOldLoadedSessions( | |||
| loadedSessionsStore: LoadedSessionsStore, | |||
| limit: number, | |||
| ): Promise<void> { | |||
| if (limit < 0 || limit >= loadedSessionsStore.getLength()) { | |||
| if (limit < 0 || limit > loadedSessionsStore.getLength()) { | |||
There was a problem hiding this comment.
I'm not sure this is right:
If maxSessionCacheSize was set to 2 in the API, then limit here is equal to maxSessionCacheSize - 1 so limit is 1.
So before creating second session loadedSessionsStore.getLength() is 1.
Then 1 > 1 returns false and we clean the old sessions,
thus we should be able to have 2 sessions!
There was a problem hiding this comment.
It seems to me that you're right though we wouldn't be closing anything thanks to:
const toDelete = entries.length - limit;
I'm not sure of what I was thinking here.
Can you fix it on your side? It's hard to develop in my current setup.
There was a problem hiding this comment.
Ok, I have changed it, I removed the change on this file.
Basically this file was already fine, and behaves like cleanOldStoredPersistentInfo.
The only change needed is create_or_load_session.ts, that have the logic of setting the limit minus 1 because it's about to create a session.
The equivalent is already done for persistent sessions in the content decryptor:
cleanOldStoredPersistentInfo(
persistentSessionsStore,
EME_MAX_STORED_PERSISTENT_SESSION_INFORMATION - 1,
);
keySystems[].maxSessionCacheSize, which allows to limit the number of createdMediaKeySession, was relied on before creating a newMediaKeySession(this is good I guess).However it considered if we were over the limit at that instant, were in reality the logic should probably also account for the
MediaKeySessionthat will be created just after.The current behavior leads to having a supplementary
MediaKeySessionthan whatkeySystems[].maxSessionCacheSizeauthorizes.To fix it, I simply remove
1from the limit we want to enforce in that call, unless removing1would lead to a negative number as this has a special semantic (which is to have no limit). This still works for0like it worked before (which is strangely equivalent to setting it to1in our logic, but setting it to0is just asking for bugs anyway I guess :p).NOTE: We could have a plausible justification for our precedent behavior that it was only setting the "cache size" and thus excluding the last "actually relied on" one, but this is not how it is documented in the API documentation, so to me this is a bug.