811183 - Recursive GC in PeerConnection shutdown

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[Eric Rescorla (:ekr)]

+++ This bug was initially created as a clone of Bug #800969 +++

The testcase must not be opened over the command-line via GDB else it will produce an assertion failure: (mod != NULL, at pk11slot.c:1766). Instead drag & drop the testcase into Firefox.


Reason: KERN_PROTECTION_FAILURE at address: 0x00000001304c3eb8
[Switching to process 63747 thread 0x3907]
PL_ArenaAllocate (pool=<value temporarily unavailable, due to optimizations>, nb=<value temporarily unavailable, due to optimizations>) at /Users/cdiehl/Code/Mozilla/mc-inbound-asan/nsprpub/lib/ds/plarena.c:170
170	        if ( PR_FAILURE == LockArena())


mozilla-inbound
changeset:   110109:78f0949318a5
tag:         tip

Comment 1

[Eric Rescorla (:ekr)]

Attachment: crash dump

The last test-case in 800969 now generates the following crash.

Comment 2

[Eric Rescorla (:ekr)]

Attachment: Refactor PCImpl

Comment 3

[Eric Rescorla (:ekr)]

Attachment: Refactor PCImpl

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 680924 [details] [diff] [review]
Refactor PCImpl

Review of attachment 680924 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't 100% done, but it's done enough to make sure you're happy with the general strategy.

Comment 5

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

Comment on attachment 680924 [details] [diff] [review]
Refactor PCImpl

Review of attachment 680924 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, and partitions the complexity some which is probably good.  Trailing spaces in two files

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +120,5 @@
> +    mIceStreams.push_back(videoStream);
> +  }
> +
> +  if (!dcStream) {
> +    CSFLogErrorS(logTag, __FUNCTION__ << ": datachannel stream is NULL");              

trailing spaces

Comment 6

[Eric Rescorla (:ekr)]

Attachment: Refactor PCImpl

Comment 7

[Eric Rescorla (:ekr)]

Attachment: Refactor PCImpl

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 681314 [details] [diff] [review]
Refactor PCImpl

Review of attachment 681314 [details] [diff] [review]:
-----------------------------------------------------------------

Note: this also includes cleanup of a leak of the nsDOMMediaStreams for the remote side
by removing an addRef.

I made a stab at cleaning that up more (i.e., not moving a raw pointer around) but ran
into some annoy C++ template/lock issues and so decided to leave it for now.

Comment 9

[Ethan Hugg [:ehugg]]

Comment on attachment 681314 [details] [diff] [review]
Refactor PCImpl

Review of attachment 681314 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine - just nits.

::: media/webrtc/signaling/signaling.gyp
@@ +134,5 @@
>          './src/peerconnection/PeerConnectionImpl.cpp',
>          './src/peerconnection/PeerConnectionImpl.h',
> +        './src/peerconnection/PeerConnectionMedia.cpp',
> +        './src/peerconnection/PeerConnectionMedia.h',
> +

This means you'll need to do a clobber when you checkin.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +391,5 @@
> +
> +  // Initialize the media object.
> +  nsresult res = mMedia->Init();
> +  if (NS_FAILED(res)) {
> +    CSFLogErrorS(logTag, __FUNCTION__ << ": Couldn't initialize media object");

We were told that using streams was bad, so these are under if debug.  I think we should be using the printf-style ones instead so they'll be more predictable in case someone wants a opt build with NSPR.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +180,5 @@
>    ReadyState mReadyState;
>  
> +  // ICE State
> +  IceState mIceState;
> +  

Perhaps we shouldn't introduce any new trailing whitespace since we just got rid of it all.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +345,5 @@
> +
> +  // Transport flows: even is RTP, odd is RTCP
> +  std::map<int, mozilla::RefPtr<mozilla::TransportFlow> > mTransportFlows;
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaObjects);

/PeerConnectionMedia.h:349:880: warning: extra ‘;’ [-pedantic]

Comment 10

[Eric Rescorla (:ekr)]

(In reply to Ethan Hugg [:ehugg] from comment #9)
> Comment on attachment 681314 [details] [diff] [review]
> Refactor PCImpl
> 
> Review of attachment 681314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine - just nits.
> 
> ::: media/webrtc/signaling/signaling.gyp
> @@ +134,5 @@
> >          './src/peerconnection/PeerConnectionImpl.cpp',
> >          './src/peerconnection/PeerConnectionImpl.h',
> > +        './src/peerconnection/PeerConnectionMedia.cpp',
> > +        './src/peerconnection/PeerConnectionMedia.h',
> > +
> 
> This means you'll need to do a clobber when you checkin.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +391,5 @@
> > +
> > +  // Initialize the media object.
> > +  nsresult res = mMedia->Init();
> > +  if (NS_FAILED(res)) {
> > +    CSFLogErrorS(logTag, __FUNCTION__ << ": Couldn't initialize media object");
> 
> We were told that using streams was bad, so these are under if debug.  I
> think we should be using the printf-style ones instead so they'll be more
> predictable in case someone wants a opt build with NSPR.

This is consistency with the exisitng code.
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
> @@ +180,5 @@
> >    ReadyState mReadyState;
> >  
> > +  // ICE State
> > +  IceState mIceState;
> > +  
> 
> Perhaps we shouldn't introduce any new trailing whitespace since we just got
> rid of it all.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
> @@ +345,5 @@
> > +
> > +  // Transport flows: even is RTP, odd is RTCP
> > +  std::map<int, mozilla::RefPtr<mozilla::TransportFlow> > mTransportFlows;
> > +
> > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaObjects);
> 
> /PeerConnectionMedia.h:349:880: warning: extra ‘;’ [-pedantic]

Comment 11

[Ethan Hugg [:ehugg]]

(In reply to Eric Rescorla from comment #10)
> (In reply to Ethan Hugg [:ehugg] from comment #9)
> > 
> > We were told that using streams was bad, so these are under if debug.  I
> > think we should be using the printf-style ones instead so they'll be more
> > predictable in case someone wants a opt build with NSPR.
> 
> This is consistency with the exisitng code.

Yes I moved most of them to stream style to be safer, but now it's on my list to move them all back since I learned that we shouldn't be using streams in FF.  No need to change it now.

Comment 12

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

Comment on attachment 681314 [details] [diff] [review]
Refactor PCImpl

Review of attachment 681314 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/signaling.gyp
@@ +134,5 @@
>          './src/peerconnection/PeerConnectionImpl.cpp',
>          './src/peerconnection/PeerConnectionImpl.h',
> +        './src/peerconnection/PeerConnectionMedia.cpp',
> +        './src/peerconnection/PeerConnectionMedia.h',
> +

Not anymore!  I fixed that - let me know if there are any problems on checkin.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +71,5 @@
> +
> +    mozilla::AudioSegment segment;
> +    segment.Init(1);
> +    segment.AppendFrames(samples.forget(), 1600,
> +                         0, 1600, mozilla::AUDIO_FORMAT_S16);

very minor point: I suggest making all the 1600's into a single #define

I realize this was just copied from elsewhere

@@ +73,5 @@
> +    segment.Init(1);
> +    segment.AppendFrames(samples.forget(), 1600,
> +                         0, 1600, mozilla::AUDIO_FORMAT_S16);
> +
> +    gen->mStream->GetStream()->AsSourceStream()->AppendToTrack(1, &segment);

This doesn't match kAudioTrack from MediaManager.h, though it does match the AddTrack above... I'm concerned this may cause problems if anything assumes the trackIds are meaningful in a within-webrtc sense, which I think they shouldn't - so maybe this isn't an issue.

Again, it was this way before, so this is probably fine.

Comment 13

[Eric Rescorla (:ekr)]

Comment on attachment 681314 [details] [diff] [review]
Refactor PCImpl

Review of attachment 681314 [details] [diff] [review]:
-----------------------------------------------------------------

*  How easily can the security issue be deduced from the patch?

It's not clear there actually is a security issue. This originally
came out of bug 800969 which seemed to be a memory error, but
we no longer can make that one cause the memory failure. The
failure here is from an assert. I marked this security-critical
because it came from a security bug.


*  Do comments in the patch, the check-in comment, or tests included
  in the patch paint a bulls-eye on the security problem?

I don't believe so.


*  Which older supported branches are affected by this flaw?

FF 18 Aurora


*  If not all supported branches, which bug introduced the flaw?

FF 18 Aurora


*  Do you have backports for the affected branches? If not, how different,
   hard to create, and risky will they be?

This fix should be upliftable to Aurora.


*  How likely is this patch to cause regressions; how much testing does
   it need?

This is a big patch, but we have tested it some. It probably needs more
testing before we consider uplift. However, because it is the likely
source of nontrivial problems we should consider it nonetheless.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +345,5 @@
> +
> +  // Transport flows: even is RTP, odd is RTCP
> +  std::map<int, mozilla::RefPtr<mozilla::TransportFlow> > mTransportFlows;
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaObjects);

Also, it's the wrong name. Surprised this worked.

Comment 14

[Eric Rescorla (:ekr)]

Comment on attachment 681314 [details] [diff] [review]
Refactor PCImpl

Review of attachment 681314 [details] [diff] [review]:
-----------------------------------------------------------------

*  How easily can the security issue be deduced from the patch?

It's not clear there actually is a security issue. This originally
came out of bug 800969 which seemed to be a memory error, but
we no longer can make that one cause the memory failure. The
failure here is from an assert. I marked this security-critical
because it came from a security bug.


*  Do comments in the patch, the check-in comment, or tests included
  in the patch paint a bulls-eye on the security problem?

I don't believe so.


*  Which older supported branches are affected by this flaw?

FF 18 Aurora


*  If not all supported branches, which bug introduced the flaw?

FF 18 Aurora


*  Do you have backports for the affected branches? If not, how different,
   hard to create, and risky will they be?

This fix should be upliftable to Aurora.


*  How likely is this patch to cause regressions; how much testing does
   it need?

This is a big patch, but we have tested it some. It probably needs more
testing before we consider uplift. However, because it is the likely
source of nontrivial problems we should consider it nonetheless.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +345,5 @@
> +
> +  // Transport flows: even is RTP, odd is RTCP
> +  std::map<int, mozilla::RefPtr<mozilla::TransportFlow> > mTransportFlows;
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaObjects);

Also, it's the wrong name. Surprised this worked.

Comment 15

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

We definitely need to land this to help with plugging leaks and getting safe with GC.

Comment 16

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

We'd like to land this before 19 uplifts.
I'm not sure this particular bug can happen in 18, as this appeared after other fixes went in that fixed the original test.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 681314 [details] [diff] [review]
Refactor PCImpl

Sec-approval+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35ea96602c26

Comment 20

[Jason Smith [:jsmith]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=800969#c20

Comment 21

[Robert Kaiser]

This is tracking+ for 18 and apparently landed for 19. Did we get enough testing on 19/20 to get this uplifted to 18 (Beta)? If so, please request approval for beta.

Comment 22

[Robert Kaiser]

(In reply to Robert Kaiser (:[e-mail]) from comment #21)
> This is tracking+ for 18 and apparently landed for 19. Did we get enough
> testing on 19/20 to get this uplifted to 18 (Beta)? If so, please request
> approval for beta.

Eric? Randell?

Comment 23

[Eric Rescorla (:ekr)]

Yes, I think we should uplift this.

Comment 24

[Alex Keybl [:akeybl]]

(In reply to Eric Rescorla (:ekr) from comment #23)
> Yes, I think we should uplift this.

Please nominate for approval ASAP. This bug has been sitting on central for long enough. If you need any help with the approval nomination process, please contact us or sync up with Randell.

We should get this landed on mozilla-beta no later than Tuesday EOD PT to make it into our fourth beta.

Comment 25

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

I've been working on applying this patch to Beta.  It doesn't apply cleanly since there were some other intervening changes that the patch we landed (one change in particular).  I hope to have it done later today or tomorrow

Comment 26

[Alex Keybl [:akeybl]]

(In reply to Randell Jesup [:jesup] from comment #25)
> I've been working on applying this patch to Beta.  It doesn't apply cleanly
> since there were some other intervening changes that the patch we landed
> (one change in particular).  I hope to have it done later today or tomorrow

Thanks, as long as we have a patch nominated for approval tomorrow the risk will be manageable.

Comment 27

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

Attachment: Recursive GC In PeerConnection shutdown (uplift to Beta);

merged to Beta (mostly spurious conflicts, and removal of the MediaStreamList changes since that patch isn't on Beta, plus a few warnings-as-errors fixes)

Comment 28

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

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

Comment 29

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

Diffs from the original patch are minimal (I diffed the patches virtually nothing but cruft from other things changing around it or changes to routines that are fully removed in the patch, plus a symbol namechange and some (void) foo's to quiet warnings-as-errors.)

Comment 30

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

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

Respin with unused member removed (mac respin only)

Comment 31

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

Attachment: Recursive GC In PeerConnection shutdown (uplift to Beta);

patch with unused member removed, for uplift

Comment 32

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

Comment on attachment 690716 [details] [diff] [review]
Recursive GC In PeerConnection shutdown (uplift to Beta);

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Initial impl
User impact if declined: Security risk if user enabled WebRTC
Testing completed (on m-c, etc.): Running for a month on FF19/FF20
Risk to taking this patch (and alternatives if risky): low risk though not minimal as it's a largish patch; however mitigated by the fact that the feature is off by default.  Biggest risk would be a null-ptr deref of media().
Patch virtually identical in practice to m-c version (compared patch diffs with ediff).
String or UUID changes made by this patch: None

Comment 33

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

https://hg.mozilla.org/releases/mozilla-beta/rev/2323a19a28cf

Comment 34

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

Can we open this bug?