1149838 - We should not suppress negotiationneeded before the first offer/answer exchange

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Byron Campen [:bwc]]

This is proving to be a point of confusion.

Comment 1

[Byron Campen [:bwc]]

Attachment: 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 27615baf707759a949c1b8b13848d69ac58d69f3 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8587023 [details]
MozReview Request: bz://1149838/bwc

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be1166230bb4

Comment 3

[Martin Thomson [:mt:]]

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.

Comment 4

[Byron Campen [:bwc]]

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/

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8587023 [details]
MozReview Request: bz://1149838/bwc

https://treeherder.mozilla.org/#/jobs?repo=try&revision=215ade5aad6a

Comment 6

[Martin Thomson [:mt:]]

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.

Comment 7

[Byron Campen [:bwc]]

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

[Martin Thomson [:mt:]]

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

Comment 9

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f319aba71b59

Comment 10

[Byron Campen [:bwc]]

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/

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8587023 [details]
MozReview Request: bz://1149838/bwc

Carry forward r=mt

Comment 12

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/681b04addd40

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/681b04addd40

Comment 14

[Nils Ohlmeier [:drno]]

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.

Comment 17

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1149838: Do not suppress onnegotiationneeded before the first offer/answer.

Comment 18

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1150966: Check whether |streams_| is null on stats methods in NrIceMediaStream.

Comment 19

[Jean-Yves Perrier [:teoli]]

Updated: https://developer.mozilla.org/en-US/docs/Web/Events/negotiationneeded
and
https://developer.mozilla.org/en-US/Firefox/Releases/40#WebRTC