There is a cleanup loop inside GlobalPCList that does not function correctly, because _list is a sparse array: // Delete all peerconnections on shutdown - mostly synchronously (we ... let array; while ((array = this._list.pop()) != undefined) { When calling pop() on a sparse array, it will return undefined before the array is empty. We either need to be checking length here, or we need to stop using an array and just use an object as our associative container. My preference is to stop using an array for this; what we really want here is an associative container with integral keys, which is modeled better by an object than an array, IMHO (ie; length on an associative container is never the max index + 1, it is the number of elements).

Initial patch

Initial patch

Is there proof of the assertion in comment 0 that pop() will return undefined before the array is empty? My reading of some doc pages doesn't mention this - and if so, perhaps it's a different bug that leaves a landmine in the array?

(In reply to Randell Jesup [:jesup] from comment #3) > Is there proof of the assertion in comment 0 that pop() will return > undefined before the array is empty? My reading of some doc pages doesn't > mention this - and if so, perhaps it's a different bug that leaves a > landmine in the array? Did that jsfiddle sandbox jib spun up convince you? Just writing it down so it doesn't look like nobody has answered here.

Try results: https://tbpl.mozilla.org/?tree=Try&rev=fd3995a34f66

Yes. From IRC: http://andrewdupont.net/2006/05/18/javascript-associative-arrays-considered-harmful/

Here's a demonstration of a sparse array in a loop like the one under discussion. Note that it never gets to "two".

(In reply to Adam Roach [:abr] from comment #7) > Created attachment 825510 [details] > How sparse arrays behave... > > Here's a demonstration of a sparse array in a loop like the one under > discussion. Note that it never gets to "two". Here's the tool I wanted! (Dirt simple JS sandbox that you can share code snippets with) http://repl.it/MDm/1

Forgot that you cannot access this from within an inner function.

removeNullRefs was checking the wrong thing for null. However, this still only works for cases where we've set _pc to null because a window was closed; this does _not_ work when a peer connection has gone away for other reasons (eg; the other end of the connection has gone away). This needs more work.

This was how we were supposed to check for null, it just never happens right now due to another bug.

As I said on IRC, this bug makes me question whether the window-list-walking teardown code is needed at shutdown. I ran the same webrtc test in three tabs and closed the middle one, then quit Firefox, and it worked without any throw-up.

Comment on attachment 825985 [details] [diff] [review] Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work. Review of attachment 825985 [details] [diff] [review]: ----------------------------------------------------------------- This still does the teardown loop, which may end up going away. It is at least less troublesome to maintain now.

Comment on attachment 825985 [details] [diff] [review] Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work. Review of attachment 825985 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/PeerConnection.js @@ +90,5 @@ > + if (list.hasOwnProperty(winID)) { > + try { > + list[winID].forEach(cleanupPcRef); > + } catch(e) { > + } Please re-write to avoid try/catch. Shouldn't be needed here since there's no outside or unknown data.

Remove try/catch for jib.

Comment on attachment 826891 [details] [diff] [review] Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work. Review of attachment 826891 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit. Nice use of hasOwnProperty() btw. ::: dom/media/PeerConnection.js @@ +121,5 @@ > getStatsForEachPC: function(callback, errorCallback) { > for (let winId in this._list) { > + if (this._list.hasOwnProperty(winId)) { > + this.removeNullRefs(winId); > + if (this._list[winId]) { We can remove this redundant if-statement, as well as the following defensive code in removeNullRefs(): > if (this._list === undefined || this._list[winID] === undefined) { > return; > } because: 1. this._list is never undefined, 2. removeNullRefs() never deletes _list properties, (filter() returns an empty array on empty, according to its polyfill), and 3. removeNullRefs() will never encounter an undefined entry the way it's called. 4. We're not using Array anymore.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #16) > @@ +121,5 @@ > > getStatsForEachPC: function(callback, errorCallback) { > > for (let winId in this._list) { > > + if (this._list.hasOwnProperty(winId)) { > > + this.removeNullRefs(winId); > > + if (this._list[winId]) { > > We can remove this redundant if-statement, as well as the following > defensive code in removeNullRefs(): > > > if (this._list === undefined || this._list[winID] === undefined) { > > return; > > } > > because: > 1. this._list is never undefined, > 2. removeNullRefs() never deletes _list properties, > (filter() returns an empty array on empty, according to its polyfill), > and Is this about to change? Tweaking removeNullRefs() to remove the list if empty was something we talked about doing. Otherwise we have a bunch of cruft from old windows.

Actually, I'm seeing cases where this._list === undefined in our mochitests... weird.

Wait, no... somehow I ended up with an old patch here. False alarm.

Fix nit.

Comment on attachment 827093 [details] [diff] [review] Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work. Review of attachment 827093 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r+ from jib.

(In reply to Byron Campen [:bwc] from comment #17) > > 2. removeNullRefs() never deletes _list properties, > > (filter() returns an empty array on empty, according to its polyfill), > > Is this about to change? Tweaking removeNullRefs() to remove the list if > empty was something we talked about doing. Otherwise we have a bunch of > cruft from old windows. I'm all for adding that, which would make the if-statement no longer redundant, obviously. We should still remove the defensive code in removeNullRefs().

Nit fixed, should be ready to go once 906990 lands.

Comment on attachment 827116 [details] [diff] [review] Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work. Review of attachment 827116 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r+ from jib

Untangle from 906990 (removed hunk will go to part 10 of 906990)

Comment on attachment 8333499 [details] [diff] [review] Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work. Review of attachment 8333499 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r+, request checkin.

Comment on attachment 8333499 [details] [diff] [review] Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work. Looks like jesup checked this in; clearing flag...

I tested this with the testcase attached by Adam on Firefox 28.0a1 and Firefox 28.0b4 (Win 7, Mac OSX 10.8.5, Ubuntu 13.04). I get the same results for both builds (also same for Chrome and Safari) - two is never popped.

(In reply to Ioana Budnar, QA [:ioana] from comment #32) > I tested this with the testcase attached by Adam on Firefox 28.0a1 and > Firefox 28.0b4 (Win 7, Mac OSX 10.8.5, Ubuntu 13.04). I get the same results > for both builds (also same for Chrome and Safari) - two is never popped. That test case was just to demonstrate the (non-intuitive) behavior of javascript arrays, to show that we were using one incorrectly in PeerConnection.js. The patch was intended to fix PeerConnection.js, not JS array's behavior (since this behavior is correct according to the specification).

Byron, is there a reliable way we can verify this is fixed? If not, let's leave it [qa-].

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #34) > Byron, is there a reliable way we can verify this is fixed? If not, let's > leave it [qa-]. There might be some way, but this is buried down in internals, so I wouldn't burn resources on it.