1175609 - onnegotiationneeded fires synchronously

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This is a regression in the sense that we never fired negotiationneeded before FF 40.

Comment 2

[Maire Reavy [:mreavy]]

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.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

It's probably too late to address this for 41, but I think we should definitely up-prioritize this because it causes script-incompatibility.

Comment 5

[Eric Shepherd [:sheppy]]

Compat issues to disclose and describe in docs and compat tables.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?)

Comment 7

[Byron Campen [:bwc]]

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.

Comment 8

[Byron Campen [:bwc]]

Yeah, it looks like we'll pick up on tracks, but not datachannel, added outside of stable.

Comment 9

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1175609: Bring onnegotiationneeded in line with spec.

Bug 1175609: Bring onnegotiationneeded in line with spec.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1175609 - queue a task to fire negotiationneeded.

Bug 1175609 - queue a task to fire negotiationneeded.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 12

[Martin Thomson [:mt:]]

I think that Byron has something that will work better on the basis that the Promise bump only queues a microtask.

Comment 13

[Martin Thomson [:mt:]]

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.

Comment 14

[Byron Campen [:bwc]]

(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.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8688185 [details]
MozReview Request: Bug 1175609 - queue a task to fire negotiationneeded.

Sounds like Byron's patch has it.

Comment 17

[Byron Campen [:bwc]]

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/

Comment 18

[Byron Campen [:bwc]]

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/

Comment 19

[Martin Thomson [:mt:]]

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.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad891c3f905

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1ad891c3f905