Closed
Bug 1149838
Opened 9 years ago
Closed 9 years ago
We should not suppress negotiationneeded before the first offer/answer exchange
Categories
(Core :: WebRTC: Signaling, defect)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bwc, Assigned: bwc)
References
Details
(Keywords: APIchange, dev-doc-complete)
Attachments
(2 files, 1 obsolete file)
This is proving to be a point of confusion.
Assignee | ||
Comment 1•9 years ago
|
||
/r/6497 - Bug 1149838: Do not suppress onnegotiationneeded before the first offer/answer. Pull down this commit: hg pull -r 27615baf707759a949c1b8b13848d69ac58d69f3 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8587023 [details] MozReview Request: bz://1149838/bwc https://treeherder.mozilla.org/#/jobs?repo=try&revision=be1166230bb4
Attachment #8587023 -
Flags: review?(martin.thomson)
Comment 3•9 years ago
|
||
Comment on attachment 8587023 [details] MozReview Request: bz://1149838/bwc https://reviewboard.mozilla.org/r/6495/#review5373 I think that we can probably do this differently; if onn-n- fulfilled a promise then we could await that promise immediately after adding tracks or creating data channels. This check is very much disconnected from the cause and that sort of loose checking can hide bugs. I don't think that this changes the `setupNegotiationCallback` call sites. It changes all the media and channel adds, but I don't think that there are many (I've identified what I think we need). ::: dom/media/tests/mochitest/dataChannel.js (Diff revision 1) > function PC_LOCAL_CREATE_DATA_CHANNEL(test) { > var channel = test.pcLocal.createDataChannel({}); > is(channel.binaryType, "blob", channel + " is of binary type 'blob'"); > is(channel.readyState, "connecting", channel + " is in state: 'connecting'"); > > is(test.pcLocal.signalingState, STABLE, > "Create datachannel does not change signaling state"); If you wanted to await onnn after addition, I think that this is a good spot. ::: dom/media/tests/mochitest/pc.js (Diff revision 1) > this.attachMedia(stream, type, 'local'); Await here. ::: dom/media/tests/mochitest/pc.js (Diff revision 1) > + checkNegotiationCallback : function() { > + if (this.expectingNegotiationNeeded) { > + ok(this.observedNegotiationNeeded, this + " observed a negotiationneeded event") > + } else { > + ok(!this.observedNegotiationNeeded, this + " did not observe a negotiationneeded event"); > + } > + this.expectingNegotiationNeeded = false; > + this.observedNegotiationNeeded = false; > + }, This wouldn't be needed. ::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html (Diff revision 1) > + test.pcLocal.expectingNegotiationNeeded = true; > test.pcLocal._pc.addTrack(track, stream); > test.pcLocal.expectedLocalTrackInfoById[track.id] = { > type: track.kind, > streamId: stream.id > }; > }); > test.pcLocal.constraints = [{ video: true, audio:true }]; // fool tests Await here. ::: dom/media/tests/mochitest/pc.js (Diff revision 1) > + setupNegotiationCallback : function() { > + this.onnegotiationneeded = event => this.observedNegotiationNeeded = true; > + }, This could reset the promise. The one-shot event wrapper is good here: if this function isn't called and the event appears, then we should be erroring. If the event fires twice, that's a big problem.
Attachment #8587023 -
Flags: review?(martin.thomson)
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8587023 [details] MozReview Request: bz://1149838/bwc /r/6497 - Bug 1149838: Do not suppress onnegotiationneeded before the first offer/answer. Pull down this commit: hg pull -r 9cbc71011264a06b6e30a3aa39ecdebf0e06dd56 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8587023 [details] MozReview Request: bz://1149838/bwc https://treeherder.mozilla.org/#/jobs?repo=try&revision=215ade5aad6a
Attachment #8587023 -
Flags: review?(martin.thomson)
Comment 6•9 years ago
|
||
Comment on attachment 8587023 [details] MozReview Request: bz://1149838/bwc https://reviewboard.mozilla.org/r/6495/#review5449 A couple of issues and a nit. I trust you know how to solve them. ::: dom/media/tests/mochitest/pc.js (Diff revision 2) > if (options.negotiated) { > remotePromise = localPromise.then(localChannel => { > // externally negotiated - we need to open from both ends > options.id = options.id || channel.id; // allow for no id on options > var remoteChannel = this.pcRemote.createDataChannel(options); > return remoteChannel.opened; > }); > } > > + return Promise.all([this.pcLocal.observedNegotiationNeeded, > + this.pcRemote.observedNegotiationNeeded]).then(() => { Is this going to work when options.negotiated is not set? In that case, the remote side won't generate the event. ::: dom/media/tests/mochitest/pc.js (Diff revision 2) > + this.onnegotiationneeded = event => resolve(); this.onnegotiationneeded = resolve; That passes the event through, but that's harmless. ::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html (Diff revision 2) > stream.getTracks().forEach(track => { > + test.pcLocal.expectNegotiationNeeded(); > test.pcLocal._pc.addTrack(track, stream); > test.pcLocal.expectedLocalTrackInfoById[track.id] = { > type: track.kind, > streamId: stream.id > }; > + return test.pcLocal.observedNegotiationNeeded; > }); This swallows the onnn event silently. Moving the return outside the `.forEach` block. If you want to be pendantic, change the `.forEach` to a `.map` and run Promise.all over the result.
Attachment #8587023 -
Flags: review?(martin.thomson) → review+
Assignee | ||
Comment 7•9 years ago
|
||
https://reviewboard.mozilla.org/r/6495/#review5453 > Is this going to work when options.negotiated is not set? In that case, the remote side won't generate the event. If options.negotiated is not set, either pcRemote.observedNegotiationNeeded will be undefined (which means we just wait on pcLocal), or it will be an already resolved promise from earlier. > This swallows the onnn event silently. > > Moving the return outside the `.forEach` block. If you want to be pendantic, change the `.forEach` to a `.map` and run Promise.all over the result. Gah, I need to get used to this.
Comment 8•9 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #7) > If options.negotiated is not set, either pcRemote.observedNegotiationNeeded > will be undefined (which means we just wait on pcLocal), or it will be an > already resolved promise from earlier. Ooo, that's tricky. I didn't realize that worked. I just checked and Promise.all([Promise.resolve(), null]) does resolve; it's not obvious though, maybe a comment to that effect?
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f319aba71b59
Flags: needinfo?(docfaraday)
Assignee | ||
Updated•9 years ago
|
Attachment #8587023 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8587023 [details] MozReview Request: bz://1149838/bwc /r/6497 - Bug 1149838: Do not suppress onnegotiationneeded before the first offer/answer. Pull down this commit: hg pull -r fe206969527184140a764869f1eae24fe03fe8cb https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8587023 [details]
MozReview Request: bz://1149838/bwc
Carry forward r=mt
Attachment #8587023 -
Flags: review+
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/681b04addd40
Flags: needinfo?(docfaraday)
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/681b04addd40
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 14•9 years ago
|
||
I think this is worth documenting, because if someone has written any code which assumes onnegotiationneeded only fires for re-negotiations his code will be surprised by this change.
Keywords: APIchange,
dev-doc-needed
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8587023 -
Attachment is obsolete: true
Attachment #8619935 -
Flags: review+
Attachment #8619936 -
Flags: review+
Assignee | ||
Comment 17•9 years ago
|
||
Assignee | ||
Comment 18•9 years ago
|
||
Comment 19•9 years ago
|
||
Updated: https://developer.mozilla.org/en-US/docs/Web/Events/negotiationneeded and https://developer.mozilla.org/en-US/Firefox/Releases/40#WebRTC
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•