Closed Bug 1175609 Opened 5 years ago Closed 5 years ago

onnegotiationneeded fires synchronously

Categories

(Core :: WebRTC, defect, P2)

40 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
Blocking Flags:

People

(Reporter: jib, Assigned: bwc)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

STR:
- Open URL and hit [Start!]

Expected output:
  pc1.addStream 1
  pc1.addStream 2
  pc1.negotiationneeded
  Connected!

Actual output:
  pc1.addStream 1
  pc1.negotiationneeded
  pc1.addStream 2
  Connected!

The spec [1] says: "If the signaling state is stable, schedule a task to check the negotiation-needed state and, if still set, fire an negotiationneeded event on connection."

This can probably be solved easily with a setTimeout in PeerConnection.js

[1] http://w3c.github.io/webrtc-pc/#firing-an-event
This is a regression in the sense that we never fired negotiationneeded before FF 40.
See Also: → 1032848
I believe this is a spec compliance issue and not causing any real pain (at this point) for developers.  So I'm making this a P3.  jib -- If I'm missing something and this needs to be a higher priority, please chime in and adjust the priority accordingly.
Blocks: webrtc_spec
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
I worry the fact that it fires prematurely can cause pain for scripts, and if this behavior makes it to release, then scripts will have to contend with too much:

1. FF < 40: negotiationneeded not firing
2. FF 41:   negotiationneeded firing early
3. FF > 41: negotiationneeded firing at the right time.

Repercussions:

For example, in the fiddle in the URL, the offer generated may contain (but not always!) only one of the tracks rather than both.

I say "not always", because:
1. This problem is probably hidden today because Bug 1169386 has not landed. 
2. Once bug 1169386 lands, bug 1171634 will make it happen only some of the time.
It's probably too late to address this for 41, but I think we should definitely up-prioritize this because it causes script-incompatibility.
Priority: P3 → P2
Compat issues to disclose and describe in docs and compat tables.
Keywords: dev-doc-needed
I hit this bug today while writing a hangup demo showing how to use datachannel.onclose to close a call correctly. It is meant to be run in two tabs in the same browser: https://jsfiddle.net/jib1/cef91vdg/

The workaround is on line 15:

    pc.onnegotiationneeded = e => Promise.resolve() // <-- Bug 1175609
      .then(() => pc.createOffer().then(d => pc.setLocalDescription(d)))
      .then(() => send({ "sdp": pc.localDescription })).catch(failed);

Basically, without the workaround, adding both a stream and a datachannel causes only one of them to be negotiated, which is tricky to figure out (and may even be a bug? shouldn't onnegotiationneeded fire again in stable?)
Flags: needinfo?(docfaraday)
I'm pretty sure I taught the code to fire negotiationneeded again if a track was added outside of stable, but I can try to give it a look.
Yeah, it looks like we'll pick up on tracks, but not datachannel, added outside of stable.
Flags: needinfo?(docfaraday)
Bug 1175609: Bring onnegotiationneeded in line with spec.
Bug 1175609 - queue a task to fire negotiationneeded.
Attachment #8688185 - Flags: review?(martin.thomson)
Whoops, looks like we both added patches.

Mine just queues a task in PeerConnection.js. Does your patch just deal with dataChannels, or does it also queue a task? If no queueing, great. If yes, would it simplify the c++ code to queue the task in JS?
Flags: needinfo?(docfaraday)
I think that Byron has something that will work better on the basis that the Promise bump only queues a microtask.
More generally, I think that if we are relying on infrastructure that dispatches events synchronously, we are in a very bad place.  `dispatchEvent` should only dispatch on the next iteration of the event loop.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #11)
> Whoops, looks like we both added patches.
> 
> Mine just queues a task in PeerConnection.js. Does your patch just deal with
> dataChannels, or does it also queue a task? If no queueing, great. If yes,
> would it simplify the c++ code to queue the task in JS?

   The code I wrote prevents the event from firing synchronously, but it also fixes some potential problems with gratuitous dispatches that simply queuing a task won't fix.
Flags: needinfo?(docfaraday)
(In reply to Martin Thomson [:mt:] from comment #13)
> `dispatchEvent` should only dispatch on the next iteration of the event loop.

I don't think that's right. dispatchEvent is synchronous, as the STRs in comment 0 show. Specs are relying on this as well when they write (from the link in comment 0):

"If the signaling state is stable, schedule a task to check the negotiation-needed state and, if still set, fire an negotiationneeded event on connection."

If it didn't fire synchronously, then there would be no way to guarantee that negotiation-needed was set when the event fires.
Comment on attachment 8688185 [details]
MozReview Request: Bug 1175609 - queue a task to fire negotiationneeded.

Sounds like Byron's patch has it.
Attachment #8688185 - Attachment is obsolete: true
Attachment #8688185 - Flags: review?(martin.thomson)
Assignee: nobody → docfaraday
Comment on attachment 8688144 [details]
MozReview Request: Bug 1175609: Bring onnegotiationneeded in line with spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25299/diff/1-2/
Attachment #8688144 - Flags: review?(martin.thomson)
Comment on attachment 8688144 [details]
MozReview Request: Bug 1175609: Bring onnegotiationneeded in line with spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25299/diff/2-3/
Attachment #8688144 - Flags: review?(martin.thomson) → review+
Comment on attachment 8688144 [details]
MozReview Request: Bug 1175609: Bring onnegotiationneeded in line with spec.

https://reviewboard.mozilla.org/r/25299/#review22985

I like it.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1ad891c3f905
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.