Closed
Bug 807929
Opened 12 years ago
Closed 12 years ago
DataChannel object needs to be refcounted - hard to manage the lifetime during deferring close
Categories
(Core :: WebRTC: Networking, defect)
Core
WebRTC: Networking
Tracking
()
VERIFIED
FIXED
mozilla19
People
(Reporter: jesup, Assigned: jesup)
References
Details
(Keywords: crash)
Attachments
(2 files, 1 obsolete file)
2.28 KB,
patch
|
Details | Diff | Splinter Review | |
32.03 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
The raw pointers to DataChannels in the mStreamsOut and mStreamsIn arrays, and ::Close queuing the actual close to be handled by resetting the stream caused a lifetime problem for DataChannel. Also a known problem was managing the lifetime during a runnable. Switching to a refcounted (threadsafe!) impl fixes most of these things. Some fancy footwork is required because nsDeque isn't a template library, so it's hard to store already_AddRefed<> objects in it directly. Note that this means a number of things like NS_NewDOMDataChannel take already_AddRefed<DataChannel>'s, etc.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #677700 -
Flags: review?(mcmanus)
Assignee | ||
Comment 2•12 years ago
|
||
This was crashing in shutdown in Ted's testcase from Bug 807647 once the patch there is applied
Keywords: crash
Target Milestone: --- → mozilla19
Comment 3•12 years ago
|
||
Comment on attachment 677700 [details] [diff] [review] Make DataChannel refcounted Review of attachment 677700 [details] [diff] [review]: ----------------------------------------------------------------- I like the ref counts and anything that gets rid of nsAutoPtr I generally approve of. (I dislike the syntax where a = b ends up modifying b.. ) but you've got more uses of already_addrefed in here than I've ever seen.. maybe its just a style thing but I think this would be a lot more reliable if you could get rid of them. I understand you need it for deque, but that can be much more limited, yes? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +660,5 @@ > NS_ENSURE_TRUE(dataChannel,NS_ERROR_FAILURE); > > CSFLogDebugS(logTag, __FUNCTION__ << ": making DOMDataChannel"); > > + return NS_NewDOMDataChannel(dataChannel.forget(), mWindow, aRetval); here is a good example.. you are forgetting a reference just to get an already addref'd pointer that gets assigned to a refptr inside DOMDataChannel. why not just pass in dataChannel in this function as a plain DataChannel * in the dom datachannel prototypes which then gets assigned (this time adding a reference) to a refptr. The dataChannel on this stack will soon go out of scope and drop the corresponding reference so they'll still balance. that's the normal pattern afaik. ::: netwerk/sctp/datachannel/DataChannel.cpp @@ +972,3 @@ > channel->mFlags |= DATA_CHANNEL_FLAGS_FINISH_RSP; > + channel->AddRef(); > + mPending.Push(channel); // Can't cast away already_AddRefed<> from channel.forget() DataChannel *tmp = channel.get(); channel.forget(); mPending.Push(tmp); ??
Attachment #677700 -
Flags: review?(mcmanus)
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Patrick McManus [:mcmanus] from comment #3) > Comment on attachment 677700 [details] [diff] [review] > Make DataChannel refcounted > > Review of attachment 677700 [details] [diff] [review]: > ----------------------------------------------------------------- > > I like the ref counts and anything that gets rid of nsAutoPtr I generally > approve of. (I dislike the syntax where a = b ends up modifying b.. ) > > but you've got more uses of already_addrefed in here than I've ever seen.. > maybe its just a style thing but I think this would be a lot more reliable > if you could get rid of them. I understand you need it for deque, but that > can be much more limited, yes? I also need (ok, it saves add/release cycles) to use them in ::Open() and in HandleOpenRequest(). These need to be internally asynchronous (::Open and ::OpenFinish), and so need to keep references. Since they end up holding a reference, and are "giving it up" in returning it, .forget() makes sense, which means already_AddRefed<> (or equivalent; there are several pseudo-synonyms). I think the API chosen for a lot of things in the tree does something similar, but ends up using getter_AddRefs(). I'll comb through an eliminate ones I can. > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > @@ +660,5 @@ > > NS_ENSURE_TRUE(dataChannel,NS_ERROR_FAILURE); > > > > CSFLogDebugS(logTag, __FUNCTION__ << ": making DOMDataChannel"); > > > > + return NS_NewDOMDataChannel(dataChannel.forget(), mWindow, aRetval); > > here is a good example.. you are forgetting a reference just to get an > already addref'd pointer that gets assigned to a refptr inside > DOMDataChannel. That one is ::Open - it has to have a reference, and then return it and give it up without having it destroyed. I could return it as a raw pointer from a .forget(), but that's just losing the compiler checking things for me. > ::: netwerk/sctp/datachannel/DataChannel.cpp > @@ +972,3 @@ > > channel->mFlags |= DATA_CHANNEL_FLAGS_FINISH_RSP; > > + channel->AddRef(); > > + mPending.Push(channel); // Can't cast away already_AddRefed<> from channel.forget() > > DataChannel *tmp = channel.get(); > channel.forget(); > mPending.Push(tmp); > ?? Ah, yes, that avoids the add/release pair, thanks. Fought with the compiler for a while there with casts and basically gave up.
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #677700 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 677784 [details] [diff] [review] Make DataChannel refcounted https://bug807929.bugzilla.mozilla.org/attachment.cgi?id=677783 has the delta between this patch and the last. Reviewing it, all the already_AddRefed<> uses were either ::Open/OpenFinish or ::HandleOpenRequest/Finish, which per before need a ref internally, and the nsDeque stuff. One of the nsDeque AddRef()s is still needed (we need to add it to a deque and return an already_AddRefed<> pointer) I'm open to a different pattern, but this was what worked with the code paths without a redesign (and I'm not sure that would help anyways).
Attachment #677784 -
Flags: review?(mcmanus)
Updated•12 years ago
|
Attachment #677784 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd2ad699353 May make sense to upload this after a short bake/verify
status-firefox18:
--- → affected
status-firefox19:
--- → affected
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bfd2ad699353
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Updated•12 years ago
|
Comment 11•11 years ago
|
||
Verified through testing the other bug.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•