Closed Bug 842549 Opened 12 years ago Closed 11 years ago

We should emit trickle ICE candidates

Categories

(Core :: WebRTC: Networking, defect)

Product:
Core
Component:
WebRTC: Networking
Version:
21 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26

People

(Reporter: abr, Assigned: abr)

References

Details

(Whiteboard: [WebRTC][blocking-webrtc-])

Attachments

(5 files, 27 obsolete files)

Part 4. Small fix from ekr to disarm the trickle_cb when the ICE ctx is destroyed.
882 bytes, patch
Details | Diff | Splinter Review
Part 1. Generate trickle candidates from nICEr, with testing
63.89 KB, patch
Details | Diff | Splinter Review
Part 2: Plumb candidates up to signaling
162.71 KB, patch
Details | Diff | Splinter Review
Part 3. PC.js changes for trickle
8.32 KB, patch
Details | Diff | Splinter Review
Part 5: Fix slots cleanup when VcmSIPCCBinding is destroyed
2.93 KB, patch
ekr
: review+
Details | Diff | Splinter Review
No description provided.
I'd like to set a target milestone for this of Gecko 29, and I'd like to land it early in the cycle -- deadline of Dec 13. The target milestones doesn't include mozilla 29 yet, but when it does, we should set it.
I'm intending to do this before TURN TCP. Actually hoping to get it into the tree this cycle.
(In reply to Eric Rescorla (:ekr) from comment #2) > I'm intending to do this before TURN TCP. Actually hoping to get it > into the tree this cycle. Music to my ears. When do you expect to have it done? I'd prefer it not be too close to Aurora uplift (Sept 16). If it is going to be landing close to uplift or if it's moderately risky, I'd rather hold off landing it until early in Gecko 27.
Attached patch Generate trickle candidates* * * (obsolete) — — Splinter Review
trickle2
Per comment 2, I'm targeting this for gecko 26.
Target Milestone: --- → mozilla26
Attachment #795240 - Attachment is obsolete: true
Comment on attachment 803440 [details] [diff] [review] Part 3. Plumb candidates up to signaling Review of attachment 803440 [details] [diff] [review]: ----------------------------------------------------------------- Ethan, can you take a look at this code and see if you think it's generally on the right track? The other patches are context.
Attachment #803440 - Flags: feedback?(ethanhugg)
Btw ignore peerconnection.js
Comment on attachment 803440 [details] [diff] [review] Part 3. Plumb candidates up to signaling Review of attachment 803440 [details] [diff] [review]: ----------------------------------------------------------------- I think a sipcc foundcandidate event is the right way to go, so I think this is on the right track. I flagged some nits and warnings I ran into anyway. Also looks like you need to get your editor to eat trailing ws. ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp @@ +141,5 @@ > + > + char *candidate_tmp = (char *)malloc(candidate.size() + 1); > + if (!candidate_tmp) > + return; > + strncpy(candidate_tmp, candidate.c_str(), candidate.size() + 1); In signaling we generally use a safe version of strncpy - bug 776682 ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +297,5 @@ > + } > + > + char *endptr; > + unsigned long level_long = > + strtoul(level.c_str(), &endptr, 10); We have been checking errno as well when calling strtoul - Bug 790194 @@ +1358,5 @@ > nsresult > PeerConnectionImpl::CheckApiState(bool assert_ice_ready) const > { > PC_AUTO_ENTER_API_CALL_NO_CHECK(); > + PR_ASSERT(mTrickle || !assert_ice_ready || (mIceState != kIceGathering)); MOZ_ASSERT ::: media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c @@ +756,5 @@ > + static const char *fname="CCAPI_CallInfo_getCandiate"; > + session_data_t *data = (session_data_t *)handle; > + CCAPP_DEBUG(DEB_F_PREFIX"Entering", DEB_F_PREFIX_ARGS(SIP_CC_PROV, fname)); > + > + if (data != NULL){ if (data) ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c @@ +3648,5 @@ > + /* If we have pending candidates flush them too */ > + while (TRUE) { > + /* unlink head and free the media */ > + candidate = (fsmdef_candidate_t *)sll_lite_unlink_head(&dcb->candidate_list); > + if (candidate != NULL) { if (candidate) @@ +4206,5 @@ > + return SM_RC_CLEANUP; > + } > + > + config_get_value(CFGID_SDPMODE, &sdpmode, sizeof(sdpmode)); > + if (sdpmode == FALSE) { if (!sdpmode) - And I wonder if we should MOZ_CRASH here instead. The idea of running any new new code with sdpmode == FALSE seem very remote. @@ +4215,5 @@ > + "never ever happen. Run for your lives!"); > + return (SM_RC_END); > + } > + > + PR_ASSERT(dcb->sdp && dcb->sdp->src_sdp); MOZ_ASSERT would work here and is generally preferred. @@ +4257,5 @@ > + pass up trickle candidates. In the other we buffer them > + and send them later. > + */ > + /* Smuggle the entire candidate structure in a string */ > + PR_snprintf(candidate_tmp, sizeof(candidate_tmp), "%d\t%s\t%s", /home/ehugg/mozilla/work/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:4261:5: warning: implicit declaration of function ‘PR_snprintf’ [-Wimplicit-function-declaration] @@ +4276,5 @@ > + return SM_RC_END; > + > + buffered_cand->candidate = strlib_malloc(candidate_tmp, -1); > + > + if (sll_lite_link_head(&dcb->candidate_list, buffered_cand) != SLL_LITE_RET_SUCCESS) /home/ehugg/mozilla/work/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:4280:6: warning: passing argument 2 of ‘sll_lite_link_head’ from incompatible pointer type [enabled by default] ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c @@ +7378,5 @@ > } > + > +/** > + * The function performs cleaning candidate list of a given call. It walks > + * through the list and deallocates each media entries. each media entry. @@ +7394,5 @@ > + > + while (TRUE) { > + /* unlink head and free the media */ > + candidate = (fsmdef_candidate_t *)sll_lite_unlink_head(&dcb_p->candidate_list); > + if (candidate != NULL) { if (candidate)
Attachment #803440 - Flags: feedback?(ethanhugg) → feedback+
Attachment #803439 - Flags: feedback?(adam)
Comment on attachment 803436 [details] [diff] [review] Part 1. Generate trickle candidates from nICEr Review of attachment 803436 [details] [diff] [review]: ----------------------------------------------------------------- This isn't totally done, but now is a good time to get the reviewing started.
Attachment #803436 - Flags: feedback?(adam)
Attached patch Part 3: PC.js for trickle (obsolete) — — Splinter Review
Attachment #803440 - Attachment is obsolete: true
More trickle/trickle testing
Attachment #803436 - Attachment is obsolete: true
Attachment #803436 - Flags: feedback?(adam)
Attachment #803439 - Attachment is obsolete: true
Attachment #803439 - Flags: feedback?(adam)
Comment on attachment 804652 [details] [diff] [review] Part 1. Generate trickle candidates from nICEr* * * Review of attachment 804652 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/nricectx.cpp @@ +309,5 @@ > +void NrIceCtx::trickle_cb(void *arg, nr_ice_ctx *ice_ctx, > + nr_ice_media_stream *stream, > + int component_id, > + nr_ice_candidate *candidate) { > + // Get the ICE ctx . after comments. @@ +408,5 @@ > #endif // USE_INTERFACE_PRIORITIZER > > + r = nr_ice_ctx_set_trickle_cb(ctx->ctx_, &NrIceCtx::trickle_cb, ctx); > + if (r) { > + MOZ_MTLOG(ML_ERROR, "Couldn't create ICE ctx for '" << name << "'"); Change message. ::: media/mtransport/nricectx.h @@ +222,5 @@ > // more forking. > nsresult Finalize(); > > + // Are we trickling? > + bool generating_trickle() const { return true; } This seems like it needs a real setting. ::: media/mtransport/test/ice_unittest.cpp @@ +215,1 @@ > bool start = true) { Indent. @@ +804,5 @@ > + &test_addr); > + ASSERT_EQ(0, r); > + > + TestStunServer::GetInstance()->SetResponseAddr(&test_addr); > + TestStunServer::GetInstance()->SetDropInitialPackets(2); Make these tests actually verify that the candidtes appear afterward. ::: media/mtransport/third_party/nICEr/src/ice/ice_component.c @@ +679,5 @@ > + Two modes: > + 1. Pair remote candidates which have not been paired > + (used in initial pairing or in processing the other side's > + trickle candidates). > + 2. Pair any remote candidate (used when processing our own Indicate which mode based on pair_all_remote ::: media/mtransport/third_party/nICEr/src/ice/ice_component.h @@ +82,5 @@ > int nr_ice_component_create(struct nr_ice_media_stream_ *stream, int component_id, nr_ice_component **componentp); > int nr_ice_component_destroy(nr_ice_component **componentp); > int nr_ice_component_initialize(struct nr_ice_ctx_ *ctx,nr_ice_component *component); > +int nr_ice_component_maybe_prune_candidate(nr_ice_ctx *ctx, nr_ice_component *comp, nr_ice_candidate *c1, int *was_pruned); > +int nr_ice_component_pair_candidate(nr_ice_peer_ctx *pctx, nr_ice_component *lcomp,nr_ice_component *pcomp, nr_ice_candidate *lcand, int pair_all_remote); space after lcomp, ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c @@ +456,5 @@ > > + if (cand->state == NR_ICE_CAND_STATE_INITIALIZED) { > + int was_pruned = 0; > + > + nr_ice_component_maybe_prune_candidate(ctx, cand->component, Check return value. @@ +464,5 @@ > + and we have a trickle ICE callback fire the callback */ > + if (ctx->trickle_cb && !was_pruned) { > + ctx->trickle_cb(ctx->trickle_cb_arg, ctx, cand->stream, cand->component_id, cand); > + > + nr_ice_ctx_pair_new_trickle_candidates(ctx, cand); Check return value. ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c @@ +831,5 @@ > + if(j==cand->component_id) > + break; > + > + j++; > + } Isn't there a fxn to do this? ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.h @@ +77,5 @@ > int nr_ice_media_stream_finalize(nr_ice_media_stream *lstr,nr_ice_media_stream *rstr); > int nr_ice_media_stream_initialize(struct nr_ice_ctx_ *ctx, nr_ice_media_stream *stream); > int nr_ice_media_stream_get_attributes(nr_ice_media_stream *stream, char ***attrsp,int *attrctp); > int nr_ice_media_stream_get_default_candidate(nr_ice_media_stream *stream, int component, nr_ice_candidate **candp); > +int nr_ice_media_stream_pair_trickle_candidate(nr_ice_peer_ctx *pctx,nr_ice_media_stream *lstream,nr_ice_media_stream *pstream, nr_ice_candidate *cand); spaces after , Is this used? ::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c @@ +357,5 @@ > + { > + int r, _status; > + nr_ice_media_stream *pstream; > + > + r_log(LOG_ICE,LOG_ERR,"ICE(%s): peer (%s) pairing local trickle ICE candidate %s",pctx->ctx->label,pctx->label,cand->label); spaces after ,s @@ +362,5 @@ > + if ((r = nr_ice_peer_ctx_find_pstream(pctx, cand->stream, &pstream))) > + ABORT(r); > + > + if ((r = nr_ice_media_stream_pair_new_trickle_candidate( > + pctx, cand->stream, pstream, cand))) 2 space indent in this code. @@ +367,5 @@ > + ABORT(r); > + > + _status=0; > + abort: > + return 0; return _status. ::: media/mtransport/third_party/nICEr/src/net/transport_addr.h @@ +79,5 @@ > > int nr_transport_addr_get_addrstring(nr_transport_addr *addr, char *str, int maxlen); > int nr_transport_addr_get_port(nr_transport_addr *addr, int *port); > int nr_transport_addr_get_ip4(nr_transport_addr *addr, UINT4 *ip4p); > + Whitespace only change.
More trickle/trickle testing
Attachment #804652 - Attachment is obsolete: true
Comment on attachment 803440 [details] [diff] [review] Part 3. Plumb candidates up to signaling Review of attachment 803440 [details] [diff] [review]: ----------------------------------------------------------------- First pass review. ::: media/webrtc/signaling/src/sipcc/core/includes/uiapi.h @@ +242,5 @@ > pc_error error, > const char *format, ...); > > +void ui_ice_candidate_found(call_events event, > + fsmdef_states_t new_state, indent. ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +611,5 @@ > EXPECT_TRUE(pc); > } > > + SignalingAgent(const std::string &aName, const std::string stun_addr, > + uint16_t stun_port) : pc(nullptr), name(aName) { Indent @@ +660,3 @@ > EXPECT_TRUE_WAIT(ice_state() == sipcc::PeerConnectionImpl::kIceWaiting || > ice_state() == sipcc::PeerConnectionImpl::kIceFailed, 5000); > +#endif Don't just strip these out. We want to run both trickle and non-trickle tests. @@ +2351,5 @@ > + > + sipcc::MediaConstraints constraints; > + agent(0)->CreateOffer(constraints, OFFER_AUDIO, SHOULD_SENDRECV_AUDIO); > + TestStunServer::GetInstance()->SetActive(true); > + PR_Sleep(2000); Verify that getoffer returns the right thing. @@ +2370,5 @@ > + PR_Sleep(2000); // Give time for candidates to be received > + agent(0)->SetLocal(TestObserver::OFFER, agent(0)->offer()); > + PR_Sleep(2000); > +} > + Add some tests for a full handshake with the following three cases: 1. Without trickle. 2. With naturally occurring trickle. 3. With explicit trickle via the test stun server
More trickle/trickle testing
Attachment #804770 - Attachment is obsolete: true
temp c++ changes
Attachment #804649 - Attachment is obsolete: true
Attached patch Part 3. PC.js changes* * * (obsolete) — — Splinter Review
Part 3: PC.js for trickle
Attachment #804644 - Attachment is obsolete: true
Attachment #804969 - Flags: review?(adam)
Comment on attachment 804970 [details] [diff] [review] Part 2: Plumb candidates up to signaling* * * Review of attachment 804970 [details] [diff] [review]: ----------------------------------------------------------------- I'm still thinking about whether we need some additional signaling unit tests.
Attachment #804970 - Flags: review?(adam)
Attachment #804971 - Flags: review?(adam)
Comment on attachment 804970 [details] [diff] [review] Part 2: Plumb candidates up to signaling* * * Review of attachment 804970 [details] [diff] [review]: ----------------------------------------------------------------- changing reviewer to ehugg
Attachment #804970 - Flags: review?(adam) → review?(ethanhugg)
Comment on attachment 804971 [details] [diff] [review] Part 3. PC.js changes* * * Review of attachment 804971 [details] [diff] [review]: ----------------------------------------------------------------- Okay, with nits. ::: dom/media/PeerConnection.js @@ +968,5 @@ > this._dompc._pendingType = null; > this.callCB(this._dompc._onSetLocalDescriptionSuccess); > > + if (!this._dompc._trickleIce || > + this._iceGatheringState == "complete") { The double-check against _trickleIce seems redundant: when we're not doing trickle ICE, the state will necessarily be complete by the time we get here. @@ +970,5 @@ > > + if (!this._dompc._trickleIce || > + this._iceGatheringState == "complete") { > + // If we are not trickling or we completed gathering prior > + // to setLocal, then trigger a call of onicecandidate here. Please remove the tabs from these two lines. @@ +1014,5 @@ > + onIceCandidate: function(level, mid, candidate) { > + this.foundIceCandidate(new this._dompc._win.mozRTCIceCandidate( > + { > + candidate:candidate, > + sdpMid:mid, Spaces after colons. @@ +1027,5 @@ > case Ci.IPeerConnection.kIceWaiting: > this._dompc.changeIceConnectionState("new"); > this.callCB(this._dompc.ongatheringchange, "complete"); > this.callCB(this._onicechange, "starting"); > + if (!this._dompc._trickleIce) { The logic behind the _executeNext() is a bit obscure unless you deeply understand how the whole event queue works and how it interacts with ICE. Please add a comment explaining what's going on here. @@ +1100,2 @@ > this.dispatchEvent(new this._dompc._win.RTCPeerConnectionIceEvent("icecandidate", > + { candidate: c, sdpMid: m, sdpMLineIndex: l The indenting here seems a bit excessive for a line this long. You probably want to at least wrap at the commas. Also, please don't use lowercase "l" as a variable -- it looks far too much like the numeral "1" in most fixed-width fonts.
Attachment #804971 - Flags: review?(adam) → review+
Comment on attachment 804970 [details] [diff] [review] Part 2: Plumb candidates up to signaling* * * Review of attachment 804970 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, giving an r- only because I think you forgot to include stunserver.h/cpp ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp @@ +556,5 @@ > *default_addrp = (char *) cpr_malloc(default_addr.size() + 1); > if (!*default_addrp) > return VCM_ERROR; > sstrncpy(*default_addrp, default_addr.c_str(), default_addr.size() + 1); > + ws-only change @@ -791,5 @@ > > return 0; > } > > - ws-only change ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.h @@ -34,5 @@ > public: > VcmSIPCCBinding (); > virtual ~VcmSIPCCBinding(); > > - ws-only change ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp @@ -142,5 @@ > res = gMainThread->IsOnCurrentThread(&on); > NS_ENSURE_SUCCESS(res, res); > MOZ_ASSERT(on); > #endif > - ws-only change ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +678,5 @@ > > mMedia = new PeerConnectionMedia(this); > > // Connect ICE slots. > +// mMedia->SignalIceCandidate.connect(this, &PeerConnectionImpl::IceCandidate); This change looks unneeded. ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c @@ -1359,5 @@ > /* > * Cache random numbers for SRTP keys > */ > gsmsdp_cache_crypto_keys(); > - ws-only change ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +33,5 @@ > #include "nricectx.h" > #include "mozilla/SyncRunnable.h" > +#include "logging.h" > +#include "stunserver.h" > +#include "stunserver.cpp" Are these two new files that should be included in this patch? @@ +446,5 @@ > > NS_IMETHODIMP > +TestObserver::OnIceCandidate(uint16_t level, > + const char * mid, > + const char * candidate) I see tabs being added on the two lines above. The only tabs in the whole file.
Attachment #804970 - Flags: review?(ethanhugg) → review-
ehugg: stunserver.{cpp,h} are in: https://bugzilla.mozilla.org/show_bug.cgi?id=916187 Can you revisit your review with those as background?
Comment on attachment 804969 [details] [diff] [review] Part 1. Generate trickle candidates from nICEr* * * Review of attachment 804969 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c @@ +139,5 @@ > abort: > return(_status); > } > > +#define NR_ICE_MAX_ATTRIBUTE_SIZE 256 found this defined twice, the other in ice_ctx.h
Depends on: 916187
Comment on attachment 804970 [details] [diff] [review] Part 2: Plumb candidates up to signaling* * * OK, builds/runs fine with bug 916187 applied.
Attachment #804970 - Flags: review- → review+
Comment on attachment 804969 [details] [diff] [review] Part 1. Generate trickle candidates from nICEr* * * Review of attachment 804969 [details] [diff] [review]: ----------------------------------------------------------------- I like the new pruning model, and think it's much cleaner overall. r+, with some nits and quite a few "stronger than nits but non-blocking" comments. I wouldn't mind seeing this again, but I don't insist on it. ::: media/mtransport/nricectx.h @@ +285,5 @@ > nr_ice_ctx *ctx_; > nr_ice_peer_ctx *peer_; > nr_ice_handler_vtbl* ice_handler_vtbl_; // Must be pointer > nr_ice_handler* ice_handler_; // Must be pointer > + bool trickle_; There doesn't appear to be any way to twist this knob. Given that the visible behavior is controlled up in PeerConnection.js, is it actually needed? ::: media/mtransport/nricemediastream.cpp @@ +247,5 @@ > << name_ << "'"); > + > + if (ctx_->generating_trickle()) { > + // Generate the right trickle candidates per S. 9 of > + // draft-ivov-mmusic-trickle-ice-01.txt. -01 actually says to use IPv6 "::" rather than IPv4 "0.0.0.0", although I know you probably don't want to do that before we add IPv6 support overall. I'm not asking you to change the behavior, but the comment should be accurate. ::: media/mtransport/nricemediastream.h @@ +81,5 @@ > +// Abstract base class for opaque values. > +class NrIceOpaque { > + public: > + virtual ~NrIceOpaque() {} > +}; Given that there can realistically only be one type of these in any given system, wouldn't it be cleaner to templatize NrIceMediaStream on the type that your (opaque) data is actually going to be? Basically, since we don't have RTTI, you're going to have to reinterpret the pointer, which is kinda messy. ::: media/mtransport/test/ice_unittest.cpp @@ +488,5 @@ > + WaitForGather(); > + } > + } > + > + void WaitForGather() { Why is this its own function? ::: media/mtransport/third_party/nICEr/src/ice/ice_component.c @@ +339,5 @@ > > This actually won't prune relayed in the very rare > case that relayed is the same. Not relevant in practice. > + */ > +int nr_ice_component_maybe_prune_candidate(nr_ice_ctx *ctx, nr_ice_component *comp, nr_ice_candidate *c1, int *was_pruned) Passing component and candidate seems unnecessary -- isn't comp going to be always equal to c1->component? If so, I would suggest we simplify the interface by removing comp from the function call. @@ +641,5 @@ > abort: > return(_status); > } > > +int nr_ice_component_pair_candidate(nr_ice_peer_ctx *pctx, nr_ice_component *lcomp,nr_ice_component *pcomp, nr_ice_candidate *lcand, int pair_all_remote) lcomp does not appear to be used in this function; remove it. @@ +649,5 @@ > + nr_ice_cand_pair *pair=0; > + char codeword[5]; > + > + nr_ice_compute_codeword(lcand->label,strlen(lcand->label),codeword); > + r_log(LOG_ICE,LOG_DEBUG,"Examining local candidate %s:%s",codeword,lcand->label); s/Examining/Pairing/ @@ +668,5 @@ > + break; > + } > + > + /* PAIR with each peer*/ > + if(TAILQ_EMPTY(&pcomp->candidates)) { The loop below handles empty lists gracefully, making this check superfluous. The only reason I can think we'd want to have a separate check for empty is to allow us to log the condition, which we evidently do not do. Please either remove the check or make use of it by logging this exceptional condition. @@ +686,5 @@ > + */ > + if (pair_all_remote || (pcand->state == NR_ICE_CAND_PEER_CANDIDATE_UNPAIRED)) { > + /* If we are pairing our own trickle candidates, the remote candidate should > + all be paired */ > + assert (!((pair_all_remote) && pcand->state != (NR_ICE_CAND_PEER_CANDIDATE_PAIRED))); The logic in this assert is really hard to follow. In fact, I can't convince myself that it matches the comment without working decomposing it by hand. Perhaps: > if (pair_all_remote) > assert (pcand->state == NR_ICE_CAND_PEER_CANDIDATE_PAIRED); @@ +689,5 @@ > + all be paired */ > + assert (!((pair_all_remote) && pcand->state != (NR_ICE_CAND_PEER_CANDIDATE_PAIRED))); > + > + nr_ice_compute_codeword(pcand->label,strlen(pcand->label),codeword); > + r_log(LOG_ICE,LOG_DEBUG,"Examining peer candidate %s:%s",codeword,pcand->label); s/Examining/Pairing with/ @@ +722,5 @@ > while(lcand){ > + if (lcand->state == NR_ICE_CAND_STATE_INITIALIZED) { > + r = nr_ice_component_pair_candidate(pctx, lcomp, pcomp, lcand, 0); > + if (r) > + ABORT(r); Are we going to this as a new pattern rather than if ((r=...? ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c @@ +455,3 @@ > ctx->uninitialized_candidates--; > > + if (cand->state == NR_ICE_CAND_STATE_INITIALIZED) { Can this be false? It seems to me that this should be an assert rather than a condition. ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c @@ +819,5 @@ > > return(0); > } > > +int nr_ice_media_stream_pair_new_trickle_candidate(nr_ice_peer_ctx *pctx, nr_ice_media_stream *lstream, nr_ice_media_stream *pstream, nr_ice_candidate *cand) lstream is unused in this function; please remove it. ::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c @@ +363,5 @@ > + ABORT(r); > + > + if ((r = nr_ice_media_stream_pair_new_trickle_candidate( > + pctx, cand->stream, pstream, cand))) > + ABORT(r); The indenting here is a bit baroque. Given the overall nICEr style, I would suggest not wrapping the predicate line.
Attachment #804969 - Flags: review?(adam) → review+
More trickle/trickle testing
Attachment #804969 - Attachment is obsolete: true
(In reply to Adam Roach [:abr] from comment #31) > Comment on attachment 804969 [details] [diff] [review] > Part 1. Generate trickle candidates from nICEr* * * > > Review of attachment 804969 [details] [diff] [review]: > ----------------------------------------------------------------- > > I like the new pruning model, and think it's much cleaner overall. > > r+, with some nits and quite a few "stronger than nits but non-blocking" > comments. I wouldn't mind seeing this again, but I don't insist on it. > > ::: media/mtransport/nricectx.h > @@ +285,5 @@ > > nr_ice_ctx *ctx_; > > nr_ice_peer_ctx *peer_; > > nr_ice_handler_vtbl* ice_handler_vtbl_; // Must be pointer > > nr_ice_handler* ice_handler_; // Must be pointer > > + bool trickle_; > > There doesn't appear to be any way to twist this knob. Given that the > visible behavior is controlled up in PeerConnection.js, is it actually > needed? > > ::: media/mtransport/nricemediastream.cpp > @@ +247,5 @@ > > << name_ << "'"); > > + > > + if (ctx_->generating_trickle()) { > > + // Generate the right trickle candidates per S. 9 of > > + // draft-ivov-mmusic-trickle-ice-01.txt. > > -01 actually says to use IPv6 "::" rather than IPv4 "0.0.0.0", although I > know you probably don't want to do that before we add IPv6 support overall. > I'm not asking you to change the behavior, but the comment should be > accurate. Changed. > ::: media/mtransport/nricemediastream.h > @@ +81,5 @@ > > +// Abstract base class for opaque values. > > +class NrIceOpaque { > > + public: > > + virtual ~NrIceOpaque() {} > > +}; > > Given that there can realistically only be one type of these in any given > system, wouldn't it be cleaner to templatize NrIceMediaStream on the type > that your (opaque) data is actually going to be? Basically, since we don't > have RTTI, you're going to have to reinterpret the pointer, which is kinda > messy. I see what you mean, but I think that would infiltrate the code too much. The reinterpret seems safe enough here. > ::: media/mtransport/test/ice_unittest.cpp > @@ +488,5 @@ > > + WaitForGather(); > > + } > > + } > > + > > + void WaitForGather() { > > Why is this its own function? It's used in one of the unit tests. > ::: media/mtransport/third_party/nICEr/src/ice/ice_component.c > @@ +339,5 @@ > > > > This actually won't prune relayed in the very rare > > case that relayed is the same. Not relevant in practice. > > + */ > > +int nr_ice_component_maybe_prune_candidate(nr_ice_ctx *ctx, nr_ice_component *comp, nr_ice_candidate *c1, int *was_pruned) > > Passing component and candidate seems unnecessary -- isn't comp going to be > always equal to c1->component? If so, I would suggest we simplify the > interface by removing comp from the function call. The convention (arguably screwy) is to include component even when it can be inferred. See, for instance nr_ice_compoent_failed_pair() > @@ +641,5 @@ > > abort: > > return(_status); > > } > > > > +int nr_ice_component_pair_candidate(nr_ice_peer_ctx *pctx, nr_ice_component *lcomp,nr_ice_component *pcomp, nr_ice_candidate *lcand, int pair_all_remote) > > lcomp does not appear to be used in this function; remove it. Done. > @@ +649,5 @@ > > + nr_ice_cand_pair *pair=0; > > + char codeword[5]; > > + > > + nr_ice_compute_codeword(lcand->label,strlen(lcand->label),codeword); > > + r_log(LOG_ICE,LOG_DEBUG,"Examining local candidate %s:%s",codeword,lcand->label); > > s/Examining/Pairing/ Done. > @@ +668,5 @@ > > + break; > > + } > > + > > + /* PAIR with each peer*/ > > + if(TAILQ_EMPTY(&pcomp->candidates)) { > > The loop below handles empty lists gracefully, making this check > superfluous. The only reason I can think we'd want to have a separate check > for empty is to allow us to log the condition, which we evidently do not do. > > Please either remove the check or make use of it by logging this exceptional > condition. Removed. It's normal for trickle. > @@ +686,5 @@ > > + */ > > + if (pair_all_remote || (pcand->state == NR_ICE_CAND_PEER_CANDIDATE_UNPAIRED)) { > > + /* If we are pairing our own trickle candidates, the remote candidate should > > + all be paired */ > > + assert (!((pair_all_remote) && pcand->state != (NR_ICE_CAND_PEER_CANDIDATE_PAIRED))); > > The logic in this assert is really hard to follow. In fact, I can't convince > myself that it matches the comment without working decomposing it by hand. > Perhaps: > > > if (pair_all_remote) > > assert (pcand->state == NR_ICE_CAND_PEER_CANDIDATE_PAIRED); Done. > @@ +689,5 @@ > > + all be paired */ > > + assert (!((pair_all_remote) && pcand->state != (NR_ICE_CAND_PEER_CANDIDATE_PAIRED))); > > + > > + nr_ice_compute_codeword(pcand->label,strlen(pcand->label),codeword); > > + r_log(LOG_ICE,LOG_DEBUG,"Examining peer candidate %s:%s",codeword,pcand->label); > > s/Examining/Pairing with/ Done. > @@ +722,5 @@ > > while(lcand){ > > + if (lcand->state == NR_ICE_CAND_STATE_INITIALIZED) { > > + r = nr_ice_component_pair_candidate(pctx, lcomp, pcomp, lcand, 0); > > + if (r) > > + ABORT(r); > > Are we going to this as a new pattern rather than if ((r=...? That's my C++ convention, but here I fixed it. > ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c > @@ +455,3 @@ > > ctx->uninitialized_candidates--; > > > > + if (cand->state == NR_ICE_CAND_STATE_INITIALIZED) { > > Can this be false? It seems to me that this should be an assert rather than > a condition. It can also be FAILED. > ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c > @@ +819,5 @@ > > > > return(0); > > } > > > > +int nr_ice_media_stream_pair_new_trickle_candidate(nr_ice_peer_ctx *pctx, nr_ice_media_stream *lstream, nr_ice_media_stream *pstream, nr_ice_candidate *cand) > > lstream is unused in this function; please remove it. Done. > ::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c > @@ +363,5 @@ > > + ABORT(r); > > + > > + if ((r = nr_ice_media_stream_pair_new_trickle_candidate( > > + pctx, cand->stream, pstream, cand))) > > + ABORT(r); > > The indenting here is a bit baroque. Given the overall nICEr style, I would > suggest not wrapping the predicate line.
temp c++ changes
Attachment #804970 - Attachment is obsolete: true
Comment on attachment 805710 [details] [diff] [review] Part 2: Plumb candidates up to signaling* * * Review of attachment 805710 [details] [diff] [review]: ----------------------------------------------------------------- carrying forward ehugg r+
Attachment #805710 - Flags: review+
More trickle/trickle testing
Attachment #805685 - Attachment is obsolete: true
Comment on attachment 805713 [details] [diff] [review] Part 1. Generate trickle candidates from nICEr* * * Review of attachment 805713 [details] [diff] [review]: ----------------------------------------------------------------- Carrying forward r+ from abr
Attachment #805713 - Flags: review+
Attached patch Part 3. PC.js changes* * * (obsolete) — — Splinter Review
Part 3: PC.js for trickle
Attachment #804971 - Attachment is obsolete: true
Comment on attachment 805724 [details] [diff] [review] Part 3. PC.js changes* * * Review of attachment 805724 [details] [diff] [review]: ----------------------------------------------------------------- carrying forward r+ from ehugg
Attachment #805724 - Flags: review+
Comment on attachment 805713 [details] [diff] [review] Part 1. Generate trickle candidates from nICEr* * * Review of attachment 805713 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/third_party/nICEr/src/ice/ice_component.c @@ +687,5 @@ > + > + nr_ice_compute_codeword(pcand->label,strlen(pcand->label),codeword); > + r_log(LOG_ICE,LOG_DEBUG,"Pairing with peer candidate %s:%s",codeword,pcand->label); > + > + if(r=nr_ice_candidate_pair_create(pctx,lcand,pcand,&pair)) Missed this the first time around, but aren't we going to get compile warnings here without wrapping the assignment in an extra set of parens? @@ +691,5 @@ > + if(r=nr_ice_candidate_pair_create(pctx,lcand,pcand,&pair)) > + ABORT(r); > + > + if(r=nr_ice_candidate_pair_insert(&pcomp->stream->check_list, > + pair)) (here too)
More trickle/trickle testing
Attachment #805713 - Attachment is obsolete: true
More trickle/trickle testing * * * Fix merge problem
Attachment #805994 - Attachment is obsolete: true
Assignee: ekr → docfaraday
Status: NEW → ASSIGNED
bzexport is confused; this needed to land on 916187, not here.
Assignee: docfaraday → ekr
Attachment #806316 - Attachment is obsolete: true
More trickle/trickle testing * * * Fix merge problem
Attachment #806112 - Attachment is obsolete: true
temp c++ changes
Attachment #805710 - Attachment is obsolete: true
Attached patch Part 3. PC.js changes* * * (obsolete) — — Splinter Review
Part 3: PC.js for trickle
Attachment #805724 - Attachment is obsolete: true
Attachment #807484 - Flags: review+
Attachment #807480 - Attachment is obsolete: true
Attachment #807478 - Attachment is obsolete: true
Attached patch Part 3. PC.js changes* * * (obsolete) — — Splinter Review
Part 3: PC.js for trickle Part 3. PC.js changes for trickle
Attachment #807495 - Attachment is obsolete: true
Attachment #807484 - Attachment is obsolete: true
Attachment #807499 - Attachment is obsolete: true
Attachment #807254 - Attachment is obsolete: true
Attachment #807505 - Attachment is obsolete: true
Attachment #807497 - Attachment is obsolete: true
Attachment #807503 - Attachment is obsolete: true
As a couple of observations here, the crash happens on line 43 of SyncRunnable: > 38 { > 39 nsresult rv; > 40 bool on; > 41 > 42 rv = thread->IsOnCurrentThread(&on); > 43 MOZ_ASSERT(NS_SUCCEEDED(rv)); > 44 if (NS_SUCCEEDED(rv) && on) { > 45 mRunnable->Run(); > 46 return; > 47 } In this case, the rv == NS_ERROR_NOT_INITIALIZED, and the thread is the STS thread (nsSocketTransportService), which returns this error iff its mThread has been nulled out (which only happens at shutdown). Combined with the facts that the crash log shows that (a) this is happening well after our tests are done, and (b) the log shows us attempting to clean up 274 calls all at once at this time, I believe that this patch has done something that prevents the PeerConnections from being cleaned up after the JS is finished with them.
More digging; it appears that the PC is going away just fine, and decrementing the refcount on the cc_call_info_t_ -- but, at that point, it simply goes from 2 to 1. This symptom appears even without this patch (see, e.g., the end of https://tbpl.mozilla.org/php/getParsedLog.php?id=28139618&tree=Mozilla-Central&full=1). I suspect that the patches on *this* bug are actually okay, and that we have a separate defect that is causing these objects to hang around later than they should.
Taking ownership of this so it stays on my radar screen.
Assignee: ekr → adam
Depends on: 919767
Okay, the issue now is this dispatch: > VcmSIPCCBinding::~VcmSIPCCBinding () > { > ... > SyncRunnable::DispatchToThread(gSTSThread, > WrapRunnable(this, > &VcmSIPCCBinding::disconnect_all)); > ... Since the VcmSIPCCBinding destruction is triggered by xpcom-shutdown, the STS thread is no longer running. With our current shutdown sequence, I *think* what we want to do is simply remove the dispatch and perform disconnect_all on the main thread. I've demonstrated locally that doing so prevents the issue that caused problems trying to land this patch last time. The only place this might bite us is if we decide to start shutting down PeerConnectionCtx prior to browser shutdown. In such a case, this disconnect might be racy if ICE activity on the STS thread isn't completely quiescent. The alternative is to have a special variant form of DispatchToThread that will attempt to dispatch to STS which checks IsOnCurrentThread()' return code for NS_ERROR_NOT_INITIALIZED, and runs on the current thread if STS isn't running. While inelegant, this would work correctly in both the xpcom-shutdown and earlier shutdown cases.
EKR: I'd like your thoughts on how to handle slot disconnections before I move forward with changes here.
Flags: needinfo?(ekr)
Adam, Slots are disconnected whenever either side is destroyed. The invariant needs to be that it happen in only one thread at a time. Thus, if the nICEr side has been destroyed on the STS thread, then we shouldn't need to do anything here (though I think we might want to MOZ_ASSERT() that there are no connections. So I think we want: 1. If STS is live, do the dispatch above. 2. If STS is not live, just exist. Of course, that needs to be non-racy.
Flags: needinfo?(ekr)
Attachment #810009 - Flags: review?(ekr)
Comment on attachment 810009 [details] [diff] [review] Part 5: Fix slots cleanup when VcmSIPCCBinding is destroyed Review of attachment 810009 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #810009 - Flags: review?(ekr) → review+
Comment on attachment 807509 [details] [diff] [review] Part 2: Plumb candidates up to signaling Review of attachment 807509 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +2628,5 @@ > + agent(0)->SetLocal(TestObserver::OFFER, agent(0)->offer()); > + PR_Sleep(1000); // Give time for the message queues. > + > + // Verify that we got our candidates. > + ASSERT_EQ(2U, agent(0)->MatchingCandidates(kBogusSrflxAddress)); This line consistently fails on my Ubuntu64 machine. It sees 4 matching candidates instead of 2. Should this be an ASSERT_GE? Looks like this in the output: Value of: agent(0)->MatchingCandidates(kBogusSrflxAddress) Actual: 4 Expected: 2U Which is: 2
Filed bug 921604 for the previous comment.
Filed Bug 921656 for some issues with the PeerConnection.js portion of this patch.
Depends on: 921656
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: