Closed
Bug 827878
Opened 12 years ago
Closed 12 years ago
crash in mozilla::TransportFlow::SendPacket
Categories
(Core :: WebRTC, defect, P1)
Core
WebRTC
Tracking
()
VERIFIED
FIXED
mozilla21
People
(Reporter: florian, Assigned: jesup)
References
Details
(Keywords: crash, Whiteboard: [webrtc][blocking-webrtc+])
Crash Data
Attachments
(2 files)
(deleted),
patch
|
ekr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ekr
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is
report bp-02b12d82-5ad1-4791-98ae-495e52130108 .
=============================================================
We started seeing this crash in our WebRTC in SocialAPI demo with today's nightly (21.0a1 (2013-01-08)).
Steps to reproduce:
1. A calls B (audio+video call).
2. B accepts the call.
3. B closes the call. B usually crashes at this point.
4. The video of B is now frozen in A's floating chat window. If A closes that window, A often (but not always) crashes too.
Note: at step 3, if A closes the call, there seems to be no crash.
Assignee | ||
Comment 1•12 years ago
|
||
Likely a null mTransport.... Probably a race between async stuff shutting down the connection and outgoing packets. We probably need to make sure any shutdown packets have flushed, and/or hold a strong ref. See also ::Destroy()
Assignee: nobody → rjesup
Priority: -- → P1
Whiteboard: [webrtc][blocking-webrtc+]
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Comment on attachment 700200 [details] [diff] [review]
don't release TransportFlow while DataChannel runnables may still be in use
the SendPacket runnable holds DataChannelConnection, but doesn't stop it from being Destroyed and the transport ref dropped. This will hold the transport (but not stop DataChannelConnection from disconnecting).
Attachment #700200 -
Flags: review?(ekr)
Assignee | ||
Updated•12 years ago
|
OS: Mac OS X → All
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][webrtc-uplift]
Comment 4•12 years ago
|
||
Comment on attachment 700200 [details] [diff] [review]
don't release TransportFlow while DataChannel runnables may still be in use
Review of attachment 700200 [details] [diff] [review]:
-----------------------------------------------------------------
lgtm
Attachment #700200 -
Flags: review?(ekr) → review+
Assignee | ||
Comment 5•12 years ago
|
||
Target Milestone: --- → mozilla21
Assignee | ||
Comment 6•12 years ago
|
||
I managed to reproduce a variant of this due to the fix just landed being incomplete - releasing the TransportFlow (on MainThread) can also cause NSS to send packets from PR_Close(), which is a no-no off of the STS thread. The comment in my patch landed for bug 805521 stated that releasing on MainThread was ok per ekr; it turns out that's wrong. We should check other TransportFlow releases to verify no others do this.
Patch to be attached; leave-open
Depends on: 805251
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift] → [webrtc][blocking-webrtc+][webrtc-uplift][leave-open]
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #701443 -
Flags: review?(ekr)
Comment 8•12 years ago
|
||
Comment 9•12 years ago
|
||
Comment on attachment 701443 [details] [diff] [review]
TransportFlows must be released on STS thread
Review of attachment 701443 [details] [diff] [review]:
-----------------------------------------------------------------
I suggested a simpler approach below. The r+ holds whether you do it my way or with the class
as you do now.
Please annotate this bug so we can back it out when bug 830100 is resolved.
::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +165,5 @@
> + if (mTransportFlow && !IsSTSThread()) {
> + MOZ_ASSERT(mSTS);
> + nsCOMPtr<nsIRunnable> release_transport = new TransportRelease(mTransportFlow.forget());
> + mSTS->Dispatch(release_transport, NS_DISPATCH_NORMAL);
> + }
You can certainly do it this way with a class, but it would be simpler to do:
static void ReleaseTransportFlow(nsRefPtr<TransportFlow> tflow) {}
and then...
RUN_ON_THREAD(mSTS, WrapRunnableNM(ReleaseTransportFlow, mTransportFlow), NS_DISPATCH_NORMAL);
::: netwerk/sctp/datachannel/DataChannel.h
@@ +185,5 @@
> + {
> + MOZ_ASSERT(!NS_IsMainThread());
> + return NS_OK;
> + }
> + nsRefPtr<TransportFlow> mFlow;
Is there any reason for mFlow to be public?
Attachment #701443 -
Flags: review?(ekr) → review+
Assignee | ||
Comment 10•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dccf31ab0c4
Using RUN_ON_THREAD
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift][leave-open] → [webrtc][blocking-webrtc+][webrtc-uplift]
Comment 11•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•12 years ago
|
||
Comment on attachment 701443 [details] [diff] [review]
TransportFlows must be released on STS thread
[Approval Request Comment]
Bug caused by (feature/regressing bug #): When DataChannels landed, though a fix for cleaning up DataChannels that landed shortly before uplift a week ago made it much easier to actually hit it (or more likely). The first fix moved it back the original "hard to hit" category; the second patch (this one) closed the hole.
This request is to uplift both fixes in this bug.
User impact if declined: Crashes when trying DataChannels (SociaAPI demo) with FF20.
Testing completed (on m-c, etc.): On m-c for a few days (longer for first fix); local testing (hundred of runs and closes or reloads of DataChannel tests)
Risk to taking this patch (and alternatives if risky): Very low (very simple fixes).
String or UUID changes made by this patch: none
Attachment #701443 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Attachment #701443 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 13•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c5fa1c30a15a
https://hg.mozilla.org/releases/mozilla-aurora/rev/d39c44ce7f8b
status-firefox20:
--- → fixed
status-firefox21:
--- → fixed
Updated•12 years ago
|
Flags: in-testsuite?
Comment 15•12 years ago
|
||
Verified by testing apprtc.appspot.com and closing calls during the call's execution to ensure no crash is happening.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Comment 16•12 years ago
|
||
Fixed in Fx20+, clearing webrtc-uplift flag.
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift] → [webrtc][blocking-webrtc+]
You need to log in
before you can comment on or make changes to this bug.
Description
•