933360 - GlobalPCList._list is sparse, and not handled correctly in some cases.

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Byron Campen [:bwc]]

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).

Comment 1

[Byron Campen [:bwc]]

Attachment: Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work.

Initial patch

Comment 2

[Byron Campen [:bwc]]

Attachment: Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work.

Initial patch

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 4

[Byron Campen [:bwc]]

(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.

Comment 5

[Byron Campen [:bwc]]

Try results:

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

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 7

[Adam Roach [:abr]]

Attachment: 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".

Comment 8

[Byron Campen [:bwc]]

(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

Comment 9

[Byron Campen [:bwc]]

Attachment: Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work.

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

Comment 10

[Byron Campen [:bwc]]

Attachment: Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work.

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.

Comment 11

[Byron Campen [:bwc]]

Attachment: Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work.

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

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 13

[Byron Campen [:bwc]]

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 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 15

[Byron Campen [:bwc]]

Attachment: Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work.

Remove try/catch for jib.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 17

[Byron Campen [:bwc]]

(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.

Comment 18

[Byron Campen [:bwc]]

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

Comment 19

[Byron Campen [:bwc]]

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

Comment 20

[Byron Campen [:bwc]]

Attachment: Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work.

Fix nit.

Comment 21

[Byron Campen [:bwc]]

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.

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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().

Comment 23

[Byron Campen [:bwc]]

Attachment: Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work.

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

Comment 24

[Byron Campen [:bwc]]

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

Comment 25

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=19215e4517d2

Comment 26

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=44bb06ce606e

Comment 27

[Byron Campen [:bwc]]

Attachment: Change GlobalPCList._list from an array to an object, and rewritesome teardown code so it will actually work.

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

Comment 28

[Byron Campen [:bwc]]

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 29

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b17c85dfc6e0

Comment 30

[Adam Roach [:abr]]

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...

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b17c85dfc6e0

Comment 32

[Ioana (away)]

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.

Comment 33

[Byron Campen [:bwc]]

(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).

Comment 34

[u279076]

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

Comment 35

[Byron Campen [:bwc]]

(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.