video-6336: media element leak causes error on chrome 92

[makarandp0]

chrome 92 started limiting the WebMediaPlayers. This breaks lot of webrtc applications. This fix ensures that 1) we do not leak media elements created internally and 2) we clear the srcObject before removing media elements.

The application that use RemoteAudioTrack.detach() method need to ensure that they clear srcObject on the returned elements as well.

related github issues: #1367 & #1528

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

• I acknowledge that all my contributions will be made under the project's license.

[charliesantos]

Do we still need to keep this try catch block?

[makarandp0]

I think so. This fix ensures that we clean up the dummy element. but customers would still see the problem if they manage to create 75+ elements (possible on large rooms). In such cases its good to log the error for further confirmation and of the issue

[charliesantos]

Should we throw a TwilioError instead? That way, it follows our convention when an error happens.

[charliesantos]

Nice one @makarandp0 . So we are only detaching elements, not properly disposing them. I've learned that we also need to pause the element first, then, call load after setting the srcObject to null. Something like

this._detachElementInternal(this._dummyEl);
this._dummyEl.pause();
this._dummyEl.remove();
this._dummyEl.srcObject = null;
this._dummyEl.oncanplay = null;
this._dummyEl.load();
this._dummyEl = null;

[makarandp0]

okay, I will update and test per your suggestion.

[makarandp0]

After experimenting a bit, I found that just setting srcObject to null is sufficient to avoid this issue. I will keep this simple and just reset srcObject as suggested as a workaround:

[charliesantos]

Is this try catch block for debugging? Should we remove this?

[makarandp0]

I would keep this block, since the issue will still happen when 75+ elements are created.

[charliesantos]

Should we throw a TwilioError instead? That way, it follows our convention when an error happens.

[makarandp0]

TwilioErrors are heavy - require associated documentation and such. I think this error is fine for now. I am also hoping that this issue will get fixed by chrome as lot of apps are affected by it. https://bugs.chromium.org/p/chromium/issues/detail?id=1232649

[makarandp0]

Update: I found that the try block is not very useful as the error happens async and not caught by the block :(. I will remove the try block.

[charliesantos]

Why do we need to create a separate _detachElementInternal method? So on _end, calling _detachElementInternal will not update internal properties such as _attachments and _elShims right?

[makarandp0]

we need seperate function because _detachElement is no-op for _dummyEl as its not added to this._attachments
It was a bug that caused us to never detach _dummyEl

[charliesantos]

Ah good one.

[charliesantos]

Just want to double check. Since we're not calling detachElement anymore, is setting srcObject to null enough, and we don't need to call removeTrack?

    const mediaStream = this._dummyEl.srcObject;
    if (mediaStream instanceof this._MediaStream) {
      mediaStream.removeTrack(this.processedTrack || this.mediaStreamTrack);
    }

[makarandp0]

thats correct, removeTrack applies only for elements attached by application - since they may already have a mediaStream, in that case we add/remove track from it.