Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix exceptions where HTMLMediaElement loads and plays race #185

Merged
merged 5 commits into from Aug 31, 2016
Merged

Fix exceptions where HTMLMediaElement loads and plays race #185

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b078288
kill unhandled exceptions where loads and plays race by queuing them …
ara4n Aug 31, 2016
c791881
fix lint
ara4n Aug 31, 2016
627f662
PR review
ara4n Aug 31, 2016
1cc74ec
don't break the promise chain if a play() fails
ara4n Aug 31, 2016
7f2070f
fix lint
ara4n Aug 31, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/models/room.js
View file
Open in desktop
Expand Up @@ -1509,7 +1509,8 @@ module.exports = Room;
* added to the end of the live timeline
*
* @example
* matrixClient.on("Room.timeline", function(event, room, toStartOfTimeline, removed, data){
* matrixClient.on("Room.timeline",
* function(event, room, toStartOfTimeline, removed, data) {
* if (!toStartOfTimeline && data.liveEvent) {
* var messageToAppend = room.timeline.[room.timeline.length - 1];
* }
Expand Down
113 changes: 95 additions & 18 deletions lib/webrtc/call.js
View file
Open in desktop
Expand Up @@ -74,6 +74,12 @@ MatrixCall.ERR_LOCAL_OFFER_FAILED = "local_offer_failed";
*/
MatrixCall.ERR_NO_USER_MEDIA = "no_user_media";

/**
* Lookup from opaque queue ID to a promise for media element operations that
* need to be serialised into a given queue
*/
MatrixCall.mediaPromises = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you attaching this to the prototype rather than being an instance member of the MatrixCall like the other fields are?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we're using this as a dictionary where an external entity (the caller) can set arbitrary keys. We should make this a proper map then using Object.create(null) (an object without a prototype) else you can get fun bugs if people decide to use "hasOwnProperty" as a queue ID. It's unlikely to happen in this particular case, but given it's a direct replacement and everything else remains the same, it's trivial to do.


utils.inherits(MatrixCall, EventEmitter);

/**
Expand Down Expand Up @@ -144,6 +150,64 @@ MatrixCall.prototype.placeScreenSharingCall =
_tryPlayRemoteStream(this);
};

/**
* Play the given HTMLMediaElement, serialising the operation into a chain
* of promises to avoid racing access to the element
* @param {Element} HTMLMediaElement to play
* @param {string} Arbitrary ID to track the chain of promises to be used
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format for JSDoc is:

@param {Type} argName Some description

The argName is missing here. This is important because npm run gendoc makes HTML output based on the JSDoc here which is shipped to gh-pages on releases.

*/
MatrixCall.prototype.playElement = function(element, queueId) {
if (MatrixCall.mediaPromises[queueId]) {
MatrixCall.mediaPromises[queueId] =
MatrixCall.mediaPromises[queueId].then(function() {
return element.play();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried this is creating a memory leak.

We never nuke the promises in the map, and I don't know enough about how Q (in this case) stores chained promises to know whether or not they get cleaned up after use (I suspect not). This means we may be holding onto references to element in the middle of a promise chain.

I don't expect you to do anything right now for this (you can heap dump a few times if you really want to find out if it is leaking, the deltas on the HTMLMediaElement types will clearly show it), but heads up either way.

});
}
else {
MatrixCall.mediaPromises[queueId] = element.play();
}
};

/**
* Pause the given HTMLMediaElement, serialising the operation into a chain
* of promises to avoid racing access to the element
* @param {Element} HTMLMediaElement to pause
* @param {string} Arbitrary ID to track the chain of promises to be used
*/
MatrixCall.prototype.pauseElement = function(element, queueId) {
if (MatrixCall.mediaPromises[queueId]) {
MatrixCall.mediaPromises[queueId] =
MatrixCall.mediaPromises[queueId].then(function() {
return element.pause();
});
}
else {
// pause doesn't actually return a promise, but do this for symmetry
// and just in case it does in future.
MatrixCall.mediaPromises[queueId] = element.pause();
}
};

/**
* Assign the given HTMLMediaElement by setting the .src attribute on it,
* serialising the operation into a chain of promises to avoid racing access
* to the element
* @param {Element} HTMLMediaElement to pause
* @param {string} the src attribute value to assign to the element
* @param {string} Arbitrary ID to track the chain of promises to be used
*/
MatrixCall.prototype.assignElement = function(element, src, queueId) {
if (MatrixCall.mediaPromises[queueId]) {
MatrixCall.mediaPromises[queueId] =
MatrixCall.mediaPromises[queueId].then(function() {
element.src = src;
});
}
else {
element.src = src;
}
};

/**
* Retrieve the local <code>&lt;video&gt;</code> DOM element.
* @return {Element} The dom element
Expand Down Expand Up @@ -180,13 +244,15 @@ MatrixCall.prototype.setLocalVideoElement = function(element) {

if (element && this.localAVStream && this.type === 'video') {
element.autoplay = true;
element.src = this.URL.createObjectURL(this.localAVStream);
this.assignElement(element,
this.URL.createObjectURL(this.localAVStream),
"localVideo");
element.muted = true;
var self = this;
setTimeout(function() {
var vel = self.getLocalVideoElement();
if (vel.play) {
vel.play();
self.playElement(vel, "localVideo");
}
}, 0);
}
Expand Down Expand Up @@ -414,16 +480,20 @@ MatrixCall.prototype._gotUserMediaForInvite = function(stream) {
videoEl.autoplay = true;
if (this.screenSharingStream) {
debuglog("Setting screen sharing stream to the local video element");
videoEl.src = this.URL.createObjectURL(this.screenSharingStream);
this.assignElement(videoEl,
this.URL.createObjectURL(this.screenSharingStream),
"localVideo");
}
else {
videoEl.src = this.URL.createObjectURL(stream);
this.assignElement(videoEl,
this.URL.createObjectURL(stream),
"localVideo");
}
videoEl.muted = true;
setTimeout(function() {
var vel = self.getLocalVideoElement();
if (vel.play) {
vel.play();
self.playElement(vel, "localVideo");
}
}, 0);
}
Expand Down Expand Up @@ -460,12 +530,14 @@ MatrixCall.prototype._gotUserMediaForAnswer = function(stream) {

if (localVidEl && self.type == 'video') {
localVidEl.autoplay = true;
localVidEl.src = self.URL.createObjectURL(stream);
this.assignElement(localVidEl,
this.URL.createObjectURL(stream),
"localVideo");
localVidEl.muted = true;
setTimeout(function() {
var vel = self.getLocalVideoElement();
if (vel.play) {
vel.play();
self.playElement(vel, "localVideo");
}
}, 0);
}
Expand Down Expand Up @@ -712,7 +784,7 @@ MatrixCall.prototype._onAddStream = function(event) {
t.onstarted = hookCallback(self, self._onRemoteStreamTrackStarted);
});

event.stream.onended = hookCallback(self, self._onRemoteStreamEnded);
event.stream.oninactive = hookCallback(self, self._onRemoteStreamEnded);
Copy link
Member

@kegsay kegsay Aug 31, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't mention about changing this in the PR description, what's the deal? I'm guessing they changed onended to oninactive, and have deprecated onended? Should we be checking for the presence of oninactive to accomodate older browsers?

// not currently implemented in chrome
event.stream.onstarted = hookCallback(self, self._onRemoteStreamStarted);

Expand Down Expand Up @@ -824,21 +896,21 @@ var sendCandidate = function(self, content) {
var terminate = function(self, hangupParty, hangupReason, shouldEmit) {
if (self.getRemoteVideoElement()) {
if (self.getRemoteVideoElement().pause) {
self.getRemoteVideoElement().pause();
self.pauseElement(self.getRemoteVideoElement(), "remoteVideo");
}
self.getRemoteVideoElement().src = "";
self.assignElement(self.getRemoteVideoElement(), "", "remoteVideo");
}
if (self.getRemoteAudioElement()) {
if (self.getRemoteAudioElement().pause) {
self.getRemoteAudioElement().pause();
self.pauseElement(self.getRemoteAudioElement(), "remoteAudio");
}
self.getRemoteAudioElement().src = "";
self.assignElement(self.getRemoteAudioElement(), "", "remoteAudio");
}
if (self.getLocalVideoElement()) {
if (self.getLocalVideoElement().pause) {
self.getLocalVideoElement().pause();
self.pauseElement(self.getLocalVideoElement(), "localVideo");
}
self.getLocalVideoElement().src = "";
self.assignElement(self.getLocalVideoElement(), "", "localVideo");
}
self.hangupParty = hangupParty;
self.hangupReason = hangupReason;
Expand Down Expand Up @@ -896,11 +968,13 @@ var _tryPlayRemoteStream = function(self) {
if (self.getRemoteVideoElement() && self.remoteAVStream) {
var player = self.getRemoteVideoElement();
player.autoplay = true;
player.src = self.URL.createObjectURL(self.remoteAVStream);
self.assignElement(player,
self.URL.createObjectURL(self.remoteAVStream),
"remoteVideo");
setTimeout(function() {
var vel = self.getRemoteVideoElement();
if (vel.play) {
vel.play();
self.playElement(vel, "remoteVideo");
}
// OpenWebRTC does not support oniceconnectionstatechange yet
if (self.webRtc.isOpenWebRTC()) {
Expand All @@ -914,11 +988,13 @@ var _tryPlayRemoteAudioStream = function(self) {
if (self.getRemoteAudioElement() && self.remoteAStream) {
var player = self.getRemoteAudioElement();
player.autoplay = true;
player.src = self.URL.createObjectURL(self.remoteAStream);
self.assignElement(player,
self.URL.createObjectURL(self.remoteAStream),
"remoteAudio");
setTimeout(function() {
var ael = self.getRemoteAudioElement();
if (ael.play) {
ael.play();
self.playElement(ael, "remoteAudio");
}
// OpenWebRTC does not support oniceconnectionstatechange yet
if (self.webRtc.isOpenWebRTC()) {
Expand Down Expand Up @@ -1102,6 +1178,7 @@ var forAllTracksOnStream = function(s, f) {
/** The MatrixCall class. */
module.exports.MatrixCall = MatrixCall;


/**
* Create a new Matrix call for the browser.
* @param {MatrixClient} client The client instance to use.
Expand Down