Enforce State Transition Rules in SIPCC

VERIFIED FIXED in mozilla24

Status

()

P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: ehugg, Assigned: abr)

Tracking

Trunk
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments, 16 obsolete attachments)

66.53 KB, patch
abr
: review+
abr
: checkin+
Details | Diff | Splinter Review
11.36 KB, patch
abr
: review+
abr
: checkin+
Details | Diff | Splinter Review
89.97 KB, patch
abr
: review+
abr
: checkin+
Details | Diff | Splinter Review
23.96 KB, patch
abr
: review+
abr
: checkin+
Details | Diff | Splinter Review
39.57 KB, patch
ekr
: review+
abr
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
SIPCC doesn't enforce states very well when faced with error conditions.

Updated

6 years ago
Whiteboard: [WebRTC], [blocking-webrtc+]

Comment 1

6 years ago
Created attachment 671155 [details] [diff] [review]
Signaling State Transition - PC V1

Comment 2

6 years ago
Comment on attachment 671155 [details] [diff] [review]
Signaling State Transition - PC V1

Review of attachment 671155 [details] [diff] [review]:
-----------------------------------------------------------------

updated PC state transitions
few state updates in fsmdef

Shared a google doc with state transition for further discussion
Attachment #671155 - Flags: review?(ethanhugg)
Attachment #671155 - Flags: review?(emannion)
Attachment #671155 - Flags: review?(ekr)
Will this patch also include state transitions within the GSM state machine, i.e. the fsmdef_function_table?
Comment on attachment 671155 [details] [diff] [review]
Signaling State Transition - PC V1

Review of attachment 671155 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +110,5 @@
>  }
>  
>  nsresult PeerConnectionCtx::Cleanup() {
> +  if(mCCM.get() != NULL)
> +  {

I thought we are to use UNIX style parentheses.

@@ +143,5 @@
>      }
> +  } else if(CC_STATE_OOS == state) {
> +      //SIPCC is down, clean up stuff.
> +      CSFLogDebug(logTag,"%s Sipcc is Down .. Cleaning up PeerConnection");
> +      Cleanup();

I would be good to print out the state number to aid debugging, especially since we get some locks here.

Updated

6 years ago
Assignee: nobody → snandaku
I keep getting outstanding defect emails about this defect, is there any movement on this work?
(Reporter)

Updated

6 years ago
Attachment #671155 - Attachment is obsolete: true
Attachment #671155 - Flags: review?(ethanhugg)
Attachment #671155 - Flags: review?(emannion)
Attachment #671155 - Flags: review?(ekr)

Updated

6 years ago
Assignee: snandaku → adam
(Assignee)

Updated

6 years ago
Duplicate of this bug: 797516
(Assignee)

Updated

6 years ago
Priority: -- → P2
The dup of this was already blocking-webrtc-; this is a nice-to-have I believe.
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc-]
(Assignee)

Updated

6 years ago
Priority: P2 → P1
(Assignee)

Comment 8

6 years ago
Created attachment 742379 [details] [diff] [review]
Part 1: Enforce State Transition Rules in SIPCC
(Assignee)

Comment 9

6 years ago
Created attachment 742497 [details] [diff] [review]
Part 2: Fix success callback event names
(Assignee)

Comment 10

6 years ago
Created attachment 742502 [details] [diff] [review]
Part 2: Fix success callback event names
(Assignee)

Updated

6 years ago
Attachment #742497 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
Created attachment 743712 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection
(Assignee)

Comment 12

6 years ago
Created attachment 743716 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection
(Assignee)

Updated

6 years ago
Attachment #743712 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
Created attachment 743797 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection
(Assignee)

Updated

6 years ago
Attachment #743716 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
Created attachment 745895 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection
(Assignee)

Updated

6 years ago
Attachment #743797 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
Created attachment 745896 [details] [diff] [review]
Part 4: signalingState tests
(Assignee)

Updated

6 years ago
Attachment #742379 - Flags: review?(ethanhugg)
(Assignee)

Updated

6 years ago
Attachment #742502 - Flags: review?(rjesup)
(Assignee)

Updated

6 years ago
Attachment #745895 - Flags: review?(ekr)
(Assignee)

Updated

6 years ago
Attachment #745896 - Flags: review?(jsmith)

Updated

6 years ago
Attachment #742502 - Flags: review?(rjesup) → review+
(Reporter)

Comment 16

6 years ago
Comment on attachment 742379 [details] [diff] [review]
Part 1: Enforce State Transition Rules in SIPCC

Review of attachment 742379 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3804,5 @@
>  
>      FSM_DEBUG_SM(DEB_F_PREFIX"Entered.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
>      config_get_value(CFGID_SDPMODE, &sdpmode, sizeof(sdpmode));
>      if (!sdpmode) {
> +        FSM_DEBUG_SM(DEB_F_PREFIX"dcb is NULL.",

This message says dcb is null, shouldn't it instead say we can't do this in sdpmode?
Attachment #742379 - Flags: review?(ethanhugg) → review+
Comment on attachment 745896 [details] [diff] [review]
Part 4: signalingState tests

Review of attachment 745896 [details] [diff] [review]:
-----------------------------------------------------------------

review- for signaling state checks being unreliable.

There's some smaller cleanup needed as well.

We'll need a separate reviewer for signaling tests also.

::: dom/media/tests/mochitest/pc.js
@@ +233,5 @@
>    [
>      'PC_LOCAL_GUM',
>      function (test) {
>        test.pcLocal.getAllUserMedia(function () {
> +        is(test.pcLocal.signalingState,"stable","(1) Signaling state");

It feels a bit awkward to be doing signaling state checks as part of the local and remote gum commands. Maybe we should add a new command that happens before PC local offer that checks correct creation state?

@@ +254,5 @@
>      'PC_LOCAL_CREATE_OFFER',
>      function (test) {
>        test.pcLocal.createOffer(function () {
> +        is(test.pcLocal.signalingState,"stable","(3) Signaling state");
> +        is(test.pcLocal.signalingStateChangeCount,0,"Local: No state change on create offer");

For all of these internal handshake signaling state checks, see the comment below about the reliability problem that exists with how these checks are being done vs. asynchronous nature of the event handler, onsignalingstatechange.

@@ +443,5 @@
> +
> +  // Hook for validating onsignalingstatechange events
> +  self.lastSignalingStateChange = null;
> +  self.signalingStateChangeCount = 0;
> +  this._pc.onsignalingstatechange = function(event) {

This isn't going to be reliable in CI. The onsignalingstatechange event is fired asynchronously away from the handshake during phases of the handshake. Whether that fires on time or not can vary based on if there's bugs present in the code or not.

What you should do here instead is that if you are expecting the callback to fire as a result of a handshake action, then you should wait for this event to fire before completing the command. If you are not expecting the event to fire, then you should have a stubbed failure test fire if the event handler unexpectedly fires.

If you move to an expectation model of when you expect the event to fire vs. not, then you could greatly simplify the checks shown above in the commands list into a reliable approach.

@@ +630,5 @@
>  
>    /**
> +   * Adds an ICE candidate and automatically handles the failure case.
> +   *
> +   * @param {object} candidate

Nit - Shouldn't this be mozRTCIceCandidate as the object type?

::: dom/media/tests/mochitest/test_peerConnection_addCandidateInHaveLocalOffer.html
@@ +27,5 @@
> +        var exception = null;
> +        try {
> +          test.pcLocal.addIceCandidate(
> +            {candidate:"1 1 UDP 2130706431 192.168.2.1 50005 typ host",
> +             sdpMLineIndex: 1}, function() {});

The function callback here shouldn't be empty - if you receive a success callback here, then that's unexpected. So that should be unexpectedCallbackAndFinish(new Error).

@@ +38,5 @@
> +      }
> +    ]]);
> +
> +    test.run();
> +  }, true);

FYI - You need to rebase. I recently landed a patch for the WebRTC mochitests on FxAndroid that removes the need for passing in the 2nd argument of true.

::: dom/media/tests/mochitest/test_peerConnection_setLocalAnswerInHaveLocalOffer.html
@@ +26,5 @@
> +      function (test) {
> +        var exception = null;
> +        test.pcLocal._last_offer.type="answer";
> +        try {
> +          test.pcLocal.setLocalDescription(test.pcLocal._last_offer, function() {});

The function callback here shouldn't be empty - if you receive a success callback here, then that's unexpected. So that should be unexpectedCallbackAndFinish(new Error).

::: dom/media/tests/mochitest/test_peerConnection_setLocalAnswerInStable.html
@@ +26,5 @@
> +      function (test) {
> +        var exception = null;
> +        test.pcLocal._last_offer.type="answer";
> +        try {
> +          test.pcLocal.setLocalDescription(test.pcLocal._last_offer, function() {});

The function callback here shouldn't be empty - if you receive a success callback here, then that's unexpected. So that should be unexpectedCallbackAndFinish(new Error).

::: dom/media/tests/mochitest/test_peerConnection_setLocalOfferInHaveRemoteOffer.html
@@ +25,5 @@
> +      "PC_REMOTE_SET_LOCAL_OFFER",
> +      function (test) {
> +        var exception = null;
> +        try {
> +          test.pcRemote.setLocalDescription(test.pcLocal._last_offer, function() {});

The function callback here shouldn't be empty - if you receive a success callback here, then that's unexpected. So that should be unexpectedCallbackAndFinish(new Error).

::: dom/media/tests/mochitest/test_peerConnection_setRemoteAnswerInHaveRemoteOffer.html
@@ +26,5 @@
> +      function (test) {
> +        var exception = null;
> +        test.pcLocal._last_offer.type="answer";
> +        try {
> +          test.pcRemote.setRemoteDescription(test.pcLocal._last_offer, function() {});

The function callback here shouldn't be empty - if you receive a success callback here, then that's unexpected. So that should be unexpectedCallbackAndFinish(new Error).

::: dom/media/tests/mochitest/test_peerConnection_setRemoteAnswerInStable.html
@@ +26,5 @@
> +      function (test) {
> +        var exception = null;
> +        test.pcLocal._last_offer.type="answer";
> +        try {
> +          test.pcLocal.setRemoteDescription(test.pcLocal._last_offer, function() {});

The function callback here shouldn't be empty - if you receive a success callback here, then that's unexpected. So that should be unexpectedCallbackAndFinish(new Error).

::: dom/media/tests/mochitest/test_peerConnection_setRemoteOfferInHaveLocalOffer.html
@@ +25,5 @@
> +      "PC_LOCAL_SET_REMOTE_OFFER",
> +      function (test) {
> +        var exception = null;
> +        try {
> +          test.pcLocal.setRemoteDescription(test.pcLocal._last_offer, function() {});

The function callback here shouldn't be empty - if you receive a success callback here, then that's unexpected. So that should be unexpectedCallbackAndFinish(new Error).
Attachment #745896 - Flags: review?(jsmith) → review-
(Assignee)

Comment 18

6 years ago
First two patches landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b321cd729e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d5ff743bb6b
Whiteboard: [WebRTC], [blocking-webrtc-] → [WebRTC], [blocking-webrtc-], [leave-open]
If you do that, won't test_peerConnection_bug840344.html fail from an unexpected INVALID_STATE error callback, until I back you out in https://hg.mozilla.org/integration/mozilla-inbound/rev/13c45a7334fc?
(Assignee)

Comment 20

6 years ago
(In reply to Phil Ringnalda (:philor) from comment #19)
> If you do that, won't test_peerConnection_bug840344.html fail from an
> unexpected INVALID_STATE error callback, until I back you out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/13c45a7334fc?

Erk. Sorry about that; I'd kept in mind that these could each be applied independently, but forgot that the bug840344 test case has been completely invalidated by the new behavior. So I guess I'll have to land all four of these as a single unit.
(Assignee)

Comment 21

6 years ago
Jason: I'm in the process of fixing the other issues you identified in the patch, but wanted to respond to this one nit just so you know why it won't be changed in the next rev.

(In reply to Jason Smith [:jsmith] from comment #17)
> Comment on attachment 745896 [details] [diff] [review]
> Part 4: signalingState tests
> @@ +630,5 @@
> >  
> >    /**
> > +   * Adds an ICE candidate and automatically handles the failure case.
> > +   *
> > +   * @param {object} candidate
> 
> Nit - Shouldn't this be mozRTCIceCandidate as the object type?

Not right now. Presently, we're just passing around hashes with the proper keys in them. That might change in the future, but I'd prefer to document the current state of things.
(Assignee)

Comment 22

6 years ago
Created attachment 750792 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection
(Assignee)

Updated

6 years ago
Attachment #745895 - Attachment is obsolete: true
Attachment #745895 - Flags: review?(ekr)
(Assignee)

Comment 23

6 years ago
Created attachment 750795 [details] [diff] [review]
Part 1: Enforce State Transition Rules in SIPCC
(Assignee)

Updated

6 years ago
Attachment #742379 - Attachment is obsolete: true
(Assignee)

Comment 24

6 years ago
Created attachment 750796 [details] [diff] [review]
Part 2: Fix success callback event names
(Assignee)

Updated

6 years ago
Attachment #742502 - Attachment is obsolete: true
(Assignee)

Comment 25

6 years ago
Created attachment 750797 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection
(Assignee)

Updated

6 years ago
Attachment #750792 - Attachment is obsolete: true
(Assignee)

Comment 26

6 years ago
Comment on attachment 750795 [details] [diff] [review]
Part 1: Enforce State Transition Rules in SIPCC

Un-bit-rotting. Carrying forward r+ from ehugg
Attachment #750795 - Flags: review+
(Assignee)

Comment 27

6 years ago
Comment on attachment 750796 [details] [diff] [review]
Part 2: Fix success callback event names

Un-bit-rotting. Carrying forward r+ from jesup
Attachment #750796 - Flags: review+
(Assignee)

Comment 28

6 years ago
Comment on attachment 750797 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection

Un-bit-rotting
Attachment #750797 - Flags: review?(ekr)
(Assignee)

Comment 29

6 years ago
Created attachment 750854 [details] [diff] [review]
Part 4: signalingState tests
(Assignee)

Updated

6 years ago
Attachment #745896 - Attachment is obsolete: true
(Assignee)

Comment 30

6 years ago
Comment on attachment 750854 [details] [diff] [review]
Part 4: signalingState tests

Jason: Aside from the comment nit that I explain above, this patch incorporates all of your suggestions.

EKR: I'm asking for your review of the signaling_unittests.cpp -- you can safely ignore the remainder, as Jason is looking at those changes.
Attachment #750854 - Flags: review?(jsmith)
Attachment #750854 - Flags: review?(ekr)
Comment on attachment 750797 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection

Review of attachment 750797 [details] [diff] [review]:
-----------------------------------------------------------------

Adam,

This is a big patch so I reviewed in rietveld.

http://firefox-codereview.appspot.com/17001/

Review copied here:

http://firefox-codereview.appspot.com/17001/diff/1/dom/media/PeerConnection.js
File dom/media/PeerConnection.js (right):

http://firefox-codereview.appspot.com/17001/diff/1/dom/media/PeerConnection.js#newcode253
dom/media/PeerConnection.js:253: // These should line up with the state
machine in fsmdef.c
I am not sure this is what we want. I tend to think that we want to
enforce the states in fsmdef.

http://firefox-codereview.appspot.com/17001/diff/1/dom/media/PeerConnection.js#newcode267
dom/media/PeerConnection.js:267: 'setLocalDescription(offer)':
I'm not sure I love these names....

It makes them kind of look like subroutine calls, which I guess is the
point.

http://firefox-codereview.appspot.com/17001/diff/1/dom/media/PeerConnection.js#newcode535
dom/media/PeerConnection.js:535: throw new
Components.Exception(message);
My feeling is that this should be asynchronous. Let's discuss.

http://firefox-codereview.appspot.com/17001/diff/1/dom/media/PeerConnection.js#newcode780
dom/media/PeerConnection.js:780: switch (this._getPC().signalingState) {
I would prefer a table here.

http://firefox-codereview.appspot.com/17001/diff/1/dom/media/PeerConnection.js#newcode796
dom/media/PeerConnection.js:796: case
Ci.IPeerConnection.kSignalingClosed:
Can this happen?

http://firefox-codereview.appspot.com/17001/diff/1/dom/media/PeerConnection.js#newcode1028
dom/media/PeerConnection.js:1028: switch (state){
switch (state) {

http://firefox-codereview.appspot.com/17001/diff/1/dom/media/PeerConnection.js#newcode1032
dom/media/PeerConnection.js:1032: break;
Fix indent.

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
(right):

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#newcode150
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:150:
"mCallState = %d (%s), mFsmState = %d (%s)",
line up with open paren.

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#newcode172
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:172:
case FSMDEF_S_STABLE:
I would prefer a map table here, since it seems like all of these arms
behave identically.

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c
File media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c
(right):

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c#newcode58
media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c:58:
CCAPP_DEBUG(DEB_F_PREFIX"Entering", DEB_F_PREFIX_ARGS(SIP_CC_PROV,
fname));
s/fname/__FUNCTION__/

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c#newcode60
media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c:60: if (
data != NULL){
if ( data ) {

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/sipcc/core/common/ui.c
File media/webrtc/signaling/src/sipcc/core/common/ui.c (right):

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/sipcc/core/common/ui.c#newcode740
media/webrtc/signaling/src/sipcc/core/common/ui.c:740:
msg.update.ccFeatUpd.data.mwi_status.status = status;
Why?

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
File media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c (right):

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c#newcode3466
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3466: if (fcb->state
!= FSMDEF_S_STABLE &&
Why not event->state here?

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c#newcode3466
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3466: if (fcb->state
!= FSMDEF_S_STABLE &&
Why not event->state?

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.cpp
File media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.cpp
(right):

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.cpp#newcode66
media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.cpp:66:
const char * fsmdef_state_name (int state);
Surely this can be declared somewhere? perhaps in fsmdef_states.h

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.h
File media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.h
(right):

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.h#newcode38
media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.h:38:
fsmdef_states_t getFsmState ();
Should this be const?

http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.h#newcode46
media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.h:46:
virtual std::string fsmStateToString (fsmdef_states_t state);
Const?

http://firefox-codereview.appspot.com/17001/
Attachment #750797 - Flags: review?(ekr) → review-
Comment on attachment 750854 [details] [diff] [review]
Part 4: signalingState tests

Review of attachment 750854 [details] [diff] [review]:
-----------------------------------------------------------------

I also reviewed this in Rietveld because it was very complicated.

https://firefox-codereview.appspot.com/18001/

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
File media/webrtc/signaling/test/signaling_unittests.cpp (right):

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:568:
SignalingAgent(std::string aName) : pc(nullptr), name(aName) {
const std::string& aName

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:680: MediaStream *stream =
nullptr) {
I don't think I am excited about these default arguments.

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:682:
nsRefPtr<DOMMediaStream> domMediaStream = nullptr;
nsRefPtr has a sensible default ctor of nullptr.

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:686: domMediaStream = new
DOMMediaStream();
What is the implication of passing in a DOMMediaStream() not connected to
anything?

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:689:
domMediaStream->SetHintContents(hint);
Are we still doing hints here? We should be able to get stuff out of the stream.

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:690:
pc->AddStream(domMediaStream);
Check the return code here.

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:694: void
RemoveStream(DOMMediaStream *stream = nullptr) {
Default arguments again.

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:784: RemoveStream();
This seems wrong. You actually need a stream argument here for the API concept.

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:909: std::string name;
Can this be const?

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:1107:
ASSERT_EQ(a2_.signaling_state(),
Let's put these in the CreateOffer(), SetLocal(), etc. calls.

I realize this is a problem below, but I would like this tested
in all the relevant places... Maybe have it take a default argument
with an expected state?

https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/te...
media/webrtc/signaling/test/signaling_unittests.cpp:2057:
ASSERT_EQ(a1_.pObserver->lastStatusCode,
Check state here and below.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +151,5 @@
> +    kReadyState = IPeerConnectionObserver::kReadyState,
> +    kIceState = IPeerConnectionObserver::kIceState,
> +    kSdpState = IPeerConnectionObserver::kSdpState,
> +    kSipccState = IPeerConnectionObserver::kSipccState,
> +    kSignalingState = IPeerConnectionObserver::kSignalingState

Since these seem to be the same, maybe we should just skip the enum alias

@@ +160,5 @@
>      stateSuccess,
>      stateError
>    };
>  
> +  TestObserver(sipcc::PeerConnectionImpl *peerConnection, std::string aName) :

const std::string&

@@ +180,5 @@
>    sipcc::PeerConnectionImpl::Error lastStatusCode;
>    uint32_t lastStateType;
>    int addIceSuccessCount;
>    bool onAddStreamCalled;
> +  std::string name;

const std::string

@@ +196,5 @@
>  TestObserver::OnCreateOfferSuccess(const char* offer)
>  {
>    lastString = strdup(offer);
>    state = stateSuccess;
> +  cout << name << ": onCreateOfferSuccess" << offer << endl;

Can we put quotes around the offer, here and below.

@@ +228,1 @@
>      << " (" << message << ")" << endl;

Line up <<
Attachment #750854 - Flags: review?(ekr) → review-
Comment on attachment 750854 [details] [diff] [review]
Part 4: signalingState tests

Review of attachment 750854 [details] [diff] [review]:
-----------------------------------------------------------------

For mochitest changes - very close. You've got one remaining issue with a potential race condition that could cause intermittent failures.

::: dom/media/tests/mochitest/pc.js
@@ +425,5 @@
> +  pcw._pc.onsignalingstatechange = function() {
> +    pcw._pc.onsignalingstatechange = unexpectedCallbackAndFinish(new Error);
> +    is(pcw._pc.signalingState, state, pcw.label + ": State is " + state + " in onsignalingstatechange");
> +    pcw.signalingChangeEvent = true;
> +    if (pcw.commandSuccess) {

So there's one potential race condition that could happen here or below that could get you stuck in the test - you could end up in a case where you could get false in this if statement here and below in pcw.signalingChangeEvent if the timing of when the events fire just right to have this happen.

What I'd suggest to do here to mitigate this is have one of the functions handle the "wait for these conditions to be true" before proceeding to the next command. That means do the following in one of the two functions:

while(!pcw.commandSuccess || !pcw.SignalingChangeEvent) {
   // sleep for some period of time before doing another check
}

text.next();

That will ensure that we only move forward if all conditions are met, even if timing plays against us. If we get stuck in the loop, then the mochitest will eventually timeout.
Attachment #750854 - Flags: review?(jsmith) → review-
(Assignee)

Comment 34

6 years ago
Jason:  I don't think the race you highlighted can happen. Keep in mind that all of this must execute on the main thread. I concede that the ordering of the success event versus the onsigalingstatechange event might be variable (I don't know enough about event processing to know whether they can be reordered -- I doubt it, but can't refute it); however, I'm 100% certain that one of them will run to completion before the other one starts. There is no concurrency possible here.
Comment on attachment 750854 [details] [diff] [review]
Part 4: signalingState tests

Okay, fair enough. I'll r+ then.
Attachment #750854 - Flags: review- → review+
(Assignee)

Comment 36

6 years ago
I'm pushing the first two patches to try (after removing dom/media/tests/mochitest/test_peerConnection_bug840344.html) to verify that they don't cause problems. I want to land the bottom half of this patch so I don't have to keep un-rotting it. The removal of the now-defunct mochi test is part of the "Part 4" patch that Jason has already approved.

https://tbpl.mozilla.org/?tree=Try&rev=d2833313df28
(In reply to Adam Roach [:abr] from comment #36)
> I'm pushing the first two patches to try (after removing
> dom/media/tests/mochitest/test_peerConnection_bug840344.html) to verify that
> they don't cause problems. I want to land the bottom half of this patch so I
> don't have to keep un-rotting it. The removal of the now-defunct mochi test
> is part of the "Part 4" patch that Jason has already approved.

Oh. I didn't even notice in the patch I got flagged review on that was test was being removed (saw it on the other patch just now). Why are we removing that test specifically?
(Assignee)

Comment 38

6 years ago
(In reply to Jason Smith [:jsmith] from comment #37)
> Oh. I didn't even notice in the patch I got flagged review on that was test
> was being removed (saw it on the other patch just now). Why are we removing
> that test specifically?

That test was checking for unsafe behavior when a specific (invalid) set of API calls was made. With the state machine checking, that invalid sequence is detected and thrown out much earlier in processing (either by throwing an exception or by calling an error callback, depending on the outcome of a conversation I need to conclude with EKR).

In any case, test_peerConnection_bug840344.html is trying to call "setLocalDescription(answer)" in the state "stable". I could fix it to check that the right thing happens under those circumstances, but that would make it identical to the new test_peerConnection_setLocalAnswerInStable.html.
Ah okay. If it ends up being identical to the newer test, then that's fine.
(Assignee)

Comment 40

6 years ago
Whoops; didn't update the Makefile.in for the previous try. Re-trying:

https://tbpl.mozilla.org/?tree=Try&rev=42486f4f99de
(Assignee)

Updated

6 years ago
Attachment #750795 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #750796 - Flags: checkin+
(Assignee)

Comment 42

6 years ago
To whomever merges this to m-c: note that only half of the patches for this bug have landed. Please do not close the bug after merging.
(In reply to Adam Roach [:abr] from comment #42)
> To whomever merges this to m-c: note that only half of the patches for this
> bug have landed. Please do not close the bug after merging.

You already have leave open in the whiteboard - you should be good here.
(Assignee)

Comment 45

6 years ago
(In reply to Eric Rescorla (:ekr) from comment #31)

> http://firefox-codereview.appspot.com/17001/diff/1/media/webrtc/signaling/src/sipcc/core/common/ui.c#newcode740
> media/webrtc/signaling/src/sipcc/core/common/ui.c:740:
> msg.update.ccFeatUpd.data.mwi_status.status = status;
> Why?

Because it was going in the wrong place. I'm sure this was broken as received, but it was in the way when I was performing some refactoring pertaining to this patch.
(Assignee)

Comment 46

6 years ago
(In reply to Eric Rescorla (:ekr) from comment #31)
> Why not event->state?

Semantically, what we want to compare to is the state reflected in the FCB, since that's authoritative. For example, if later changes were to call fsm_change_state(), then the change is reflected in the FCB, but not in the event. Even if the prospect of calling fsm_change_state() in this function is implausible, using event->state runs the risk of doing The Wrong Thing if it gets cargo-culted into some other part of the code.

The code changes from "event" to "fsm" -- here and elsewhere -- arose out of an attempt to rationalize the code's confusion between events and states. Untangling that mess ended up being more profound than was warranted by this bug; however, the changes to clean up related code do technically make it "more correct," and pave the way for future efforts to clean that particular mess up in the future (if we decide such clean up is worth the effort).
(Assignee)

Comment 47

6 years ago
Created attachment 755405 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection
(Assignee)

Updated

6 years ago
Attachment #750797 - Attachment is obsolete: true
(Assignee)

Comment 48

6 years ago
Comment on attachment 755405 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection

The interdiff on this appears to be completely broken (it shows the reverse of changes I made in some places), probably because I had to un-rot the patch a few times to match the state of the tree.

I believe this addresses each of your comments from the previous review. You had a couple of "why" questions in your review, which I've addressed in comments above -- those two comments did not result in any code changes.
Attachment #755405 - Flags: review?(ekr)
(Assignee)

Comment 49

6 years ago
Comment on attachment 750854 [details] [diff] [review]
Part 4: signalingState tests

Review of attachment 750854 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +151,5 @@
> +    kReadyState = IPeerConnectionObserver::kReadyState,
> +    kIceState = IPeerConnectionObserver::kIceState,
> +    kSdpState = IPeerConnectionObserver::kSdpState,
> +    kSipccState = IPeerConnectionObserver::kSipccState,
> +    kSignalingState = IPeerConnectionObserver::kSignalingState

Due to an "hg qref" error, I fixed these in the patch that I already landed.

@@ +196,5 @@
>  TestObserver::OnCreateOfferSuccess(const char* offer)
>  {
>    lastString = strdup(offer);
>    state = stateSuccess;
> +  cout << name << ": onCreateOfferSuccess" << offer << endl;

Ah, see, here's what I meant by having some swamp cleaning in the review. :) But I agree with the sentiment, so I'll fix it.
(Assignee)

Comment 50

6 years ago
(In reply to Eric Rescorla (:ekr) from comment #32)

> https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/
> te...
> media/webrtc/signaling/test/signaling_unittests.cpp:680: MediaStream *stream
> =
> nullptr) {
> I don't think I am excited about these default arguments.

I... really don't know what to do with that comment.

> https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/
> te...
> media/webrtc/signaling/test/signaling_unittests.cpp:686: domMediaStream = new
> DOMMediaStream();
> What is the implication of passing in a DOMMediaStream() not connected to
> anything?

I'm not entirely certain. This function is a trivial refactoring of the code of the code that already existed in CreateOffer() and CreateAnswer(). If you check the code currently present in CreateAnswer(), it does exactly this:

> nsRefPtr<DOMMediaStream> domMediaStream = new DOMMediaStream();

I could chase down this rabbit hole if you insist, but I'm happy moving forward with the assertion that my refactoring does not change existing behavior -- behavior unrelated to my changes -- without knowing exactly why or whether that behavior is 100% correct.


> https://firefox-codereview.appspot.com/18001/diff/1/media/webrtc/signaling/
> te...
> media/webrtc/signaling/test/signaling_unittests.cpp:689:
> domMediaStream->SetHintContents(hint);
> Are we still doing hints here? We should be able to get stuff out of the
> stream.

A quick glance through PeerConnectionImpl leads me to believe that the behavior is still hint-based. If you think this is wrong, I would ask you to open a new bug on that issue.

I have addressed the remaining points you raise in the upcoming patch.
(Assignee)

Comment 51

6 years ago
Created attachment 755735 [details] [diff] [review]
Part 4: signalingState tests
(Assignee)

Updated

6 years ago
Attachment #750854 - Attachment is obsolete: true
(Assignee)

Comment 52

6 years ago
Comment on attachment 755735 [details] [diff] [review]
Part 4: signalingState tests

EKR: This should address all the comments that I haven't otherwise responded to.

Jason: You'll probably want to re-review the mochi tests. Based on conversations between me and EKR, and subsequent discussions on the webrtc mailing list, we determined that it is more appropriate to use error callbacks for operations in invalid states (rather than exceptions, which is what we had implemented before). There are some changes in pc.js (e.g., setRemoteDescriptionAndFail), and every new mochi test has been updated to expect error callbacks rather than exceptions.
Attachment #755735 - Flags: review?(jsmith)
Attachment #755735 - Flags: review?(ekr)

Updated

6 years ago
Attachment #755735 - Flags: review?(jsmith) → review+
Comment on attachment 755735 [details] [diff] [review]
Part 4: signalingState tests

Review of attachment 755735 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/test_peerConnection_addCandidateInHaveLocalOffer.html
@@ +30,5 @@
> +             sdpMLineIndex: 1}),
> +          function(err) {
> +            is(err.name, "INVALID_STATE", "Error is INVALID_STATE");
> +            test.next();
> +          } );

Nit - extra whitespace for each mochitest in this patch at this point of the test.
Another rietveld review:
https://firefox-codereview.appspot.com/17001/

One bug and a bunch of style-type issues

https://firefox-codereview.appspot.com/17001/diff/6001/dom/media/PeerConnecti...
File dom/media/PeerConnection.js (right):

https://firefox-codereview.appspot.com/17001/diff/6001/dom/media/PeerConnecti...
dom/media/PeerConnection.js:997: case
Ci.IPeerConnectionObserver.kSignalingState:
Embedded switches are pretty gross. Suggest break out the ICE handler.

https://firefox-codereview.appspot.com/17001/diff/6001/dom/media/PeerConnecti...
dom/media/PeerConnection.js:1034: // Unhandled state type
Can we make this log?

https://firefox-codereview.appspot.com/17001/diff/6001/media/webrtc/signaling...
File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h (right):

https://firefox-codereview.appspot.com/17001/diff/6001/media/webrtc/signaling...
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:144:
kSignalingHaveRemotePranswer = 5,
These are not in the same order as fsm.h, which has:

    /* WebRTC States */
    FSMDEF_S_STABLE,
    FSMDEF_S_HAVE_LOCAL_OFFER,
    FSMDEF_S_HAVE_REMOTE_OFFER,
    FSMDEF_S_HAVE_REMOTE_PRANSWER,
    FSMDEF_S_HAVE_LOCAL_PRANSWER,
    FSMDEF_S_CLOSED,

https://firefox-codereview.appspot.com/17001/diff/6001/media/webrtc/signaling...
File media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp (right):

https://firefox-codereview.appspot.com/17001/diff/6001/media/webrtc/signaling...
media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:399: if
(static_cast<unsigned>(aIndex) >= mRemoteSourceStreams.Length()) {
unsigned int, please. Also, please check that aIndex >0

https://firefox-codereview.appspot.com/17001/diff/6001/media/webrtc/signaling...
File media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c (right):

https://firefox-codereview.appspot.com/17001/diff/6001/media/webrtc/signaling...
media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c:1421:
sessUpd->update.ccSessionUpd.data.state_data.fsm_state;
This code appears to use 4 space indent.

https://firefox-codereview.appspot.com/17001/diff/6001/media/webrtc/signaling...
File media/webrtc/signaling/src/sipcc/core/common/ui.c (right):

https://firefox-codereview.appspot.com/17001/diff/6001/media/webrtc/signaling...
media/webrtc/signaling/src/sipcc/core/common/ui.c:1561: va_list args)
Please line up with opening paren.

https://firefox-codereview.appspot.com/17001/diff/6001/media/webrtc/signaling...
File media/webrtc/signaling/src/sipcc/include/fsmdef_states.h (right):

https://firefox-codereview.appspot.com/17001/diff/6001/media/webrtc/signaling...
media/webrtc/signaling/src/sipcc/include/fsmdef_states.h:36:
FSMDEF_S_HAVE_LOCAL_PRANSWER,
Not in fact the same order.

PCImpl has local, remote, local, remote

Updated

6 years ago
Attachment #755405 - Flags: review?(ekr) → review-
Comment on attachment 755735 [details] [diff] [review]
Part 4: signalingState tests

Review of attachment 755735 [details] [diff] [review]:
-----------------------------------------------------------------

This is just a review of the C++ code. I think it still neeeds some work.

Review on rietveld:
https://firefox-codereview.appspot.com/18001/

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
File media/webrtc/signaling/test/signaling_unittests.cpp (right):

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:46: static const char
*ansiColorOff = "\x1b[0m";
Please make these conditional.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:54: class Prefix : public
std::streambuf
This seems more complicated than needed. Wouldn't it be easier to just implement
something that accepted << and did a character-by-character write?

Or alternately, since you don't really seem to use it with anything complicated,
how about a static function along the lines of:

void WriteWithPrefix(const std::string& prefix, const std::string &stuff) {
...

}

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:61: foundLF(true) { }
Whitespace here please.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:64: }
Whitespace here plase.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:66: int overflow (int c) {
Shouldn't this be virtual?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:76: private:
Isn't this nonportable to systems with '\r' as the end of line?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:675: return
static_cast<sipcc::PeerConnectionImpl::SignalingState>(res);
Should we check that res is in-range?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:742:
sipcc::PeerConnectionImpl::kSignalingStable) {
I think I would prefer that we actually explicitly passed endState. Or
alternately, make two variants of CreateOffer. Default arguments kinda suck?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:769:
ASSERT_EQ(pObserver->state, TestObserver::stateSuccess);
Is there a reason why you can't just ASSERT_TRUE_WIAT(...stateSuccess);

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:775: void
CreateOfferExpectError(sipcc::MediaConstraints& constraints) {
How is this expecting error? It seems to be expecting success.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:777:
ASSERT_TRUE_WAIT(pObserver->state != TestObserver::stateNoResponse,
Again, can we just ASSERT_TRUE_WAIT(stateSucces).

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:784: uint32_t sdpCheck =
DONT_CHECK_AUDIO|
You're killing me with the default arguments.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:801: 
Extra whitespace.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:804:
ASSERT_TRUE_WAIT(pObserver->state != TestObserver::stateNoResponse,
ASSERT_TRUE_WAIT again...

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:827:
ASSERT_EQ(pc->RemoveStream(domMediaStream_), NS_OK);
Maybe we should remove this entirely since it's never called and kinda hosed.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:842:
sipcc::PeerConnectionImpl::kSignalingInvalid) {
Default arguments again

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:848: }
This seems like we could just ask the caller to provide the value rather than
having this logic.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:868:
sipcc::PeerConnectionImpl::kSignalingStable);
Again, I'm kinda sad about this logic. Let's just pass the value explicitly

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:1173:
a1_.CreateOffer(aconstraints, offerAnswerFlags, offerSdpCheck);
Just to confirm, you have added the checks in the calls?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:2231: //
SetRemoteDescription call fails.
What else would it be?
Attachment #755735 - Flags: review?(ekr) → review-
Comment on attachment 755735 [details] [diff] [review]
Part 4: signalingState tests

Review of attachment 755735 [details] [diff] [review]:
-----------------------------------------------------------------

This is just a review of the C++ code. I think it still neeeds some work.

Review on rietveld:
https://firefox-codereview.appspot.com/18001/

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
File media/webrtc/signaling/test/signaling_unittests.cpp (right):

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:46: static const char
*ansiColorOff = "\x1b[0m";
Please make these conditional.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:54: class Prefix : public
std::streambuf
This seems more complicated than needed. Wouldn't it be easier to just implement
something that accepted << and did a character-by-character write?

Or alternately, since you don't really seem to use it with anything complicated,
how about a static function along the lines of:

void WriteWithPrefix(const std::string& prefix, const std::string &stuff) {
...

}

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:61: foundLF(true) { }
Whitespace here please.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:64: }
Whitespace here plase.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:66: int overflow (int c) {
Shouldn't this be virtual?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:76: private:
Isn't this nonportable to systems with '\r' as the end of line?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:675: return
static_cast<sipcc::PeerConnectionImpl::SignalingState>(res);
Should we check that res is in-range?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:742:
sipcc::PeerConnectionImpl::kSignalingStable) {
I think I would prefer that we actually explicitly passed endState. Or
alternately, make two variants of CreateOffer. Default arguments kinda suck?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:769:
ASSERT_EQ(pObserver->state, TestObserver::stateSuccess);
Is there a reason why you can't just ASSERT_TRUE_WIAT(...stateSuccess);

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:775: void
CreateOfferExpectError(sipcc::MediaConstraints& constraints) {
How is this expecting error? It seems to be expecting success.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:777:
ASSERT_TRUE_WAIT(pObserver->state != TestObserver::stateNoResponse,
Again, can we just ASSERT_TRUE_WAIT(stateSucces).

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:784: uint32_t sdpCheck =
DONT_CHECK_AUDIO|
You're killing me with the default arguments.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:801: 
Extra whitespace.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:804:
ASSERT_TRUE_WAIT(pObserver->state != TestObserver::stateNoResponse,
ASSERT_TRUE_WAIT again...

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:827:
ASSERT_EQ(pc->RemoveStream(domMediaStream_), NS_OK);
Maybe we should remove this entirely since it's never called and kinda hosed.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:842:
sipcc::PeerConnectionImpl::kSignalingInvalid) {
Default arguments again

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:848: }
This seems like we could just ask the caller to provide the value rather than
having this logic.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:868:
sipcc::PeerConnectionImpl::kSignalingStable);
Again, I'm kinda sad about this logic. Let's just pass the value explicitly

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:1173:
a1_.CreateOffer(aconstraints, offerAnswerFlags, offerSdpCheck);
Just to confirm, you have added the checks in the calls?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:2231: //
SetRemoteDescription call fails.
What else would it be?
Comment on attachment 755735 [details] [diff] [review]
Part 4: signalingState tests

Review of attachment 755735 [details] [diff] [review]:
-----------------------------------------------------------------

This is just a review of the C++ code. I think it still neeeds some work.

Review on rietveld:
https://firefox-codereview.appspot.com/18001/

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
File media/webrtc/signaling/test/signaling_unittests.cpp (right):

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:46: static const char
*ansiColorOff = "\x1b[0m";
Please make these conditional.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:54: class Prefix : public
std::streambuf
This seems more complicated than needed. Wouldn't it be easier to just implement
something that accepted << and did a character-by-character write?

Or alternately, since you don't really seem to use it with anything complicated,
how about a static function along the lines of:

void WriteWithPrefix(const std::string& prefix, const std::string &stuff) {
...

}

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:61: foundLF(true) { }
Whitespace here please.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:64: }
Whitespace here plase.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:66: int overflow (int c) {
Shouldn't this be virtual?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:76: private:
Isn't this nonportable to systems with '\r' as the end of line?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:675: return
static_cast<sipcc::PeerConnectionImpl::SignalingState>(res);
Should we check that res is in-range?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:742:
sipcc::PeerConnectionImpl::kSignalingStable) {
I think I would prefer that we actually explicitly passed endState. Or
alternately, make two variants of CreateOffer. Default arguments kinda suck?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:769:
ASSERT_EQ(pObserver->state, TestObserver::stateSuccess);
Is there a reason why you can't just ASSERT_TRUE_WIAT(...stateSuccess);

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:775: void
CreateOfferExpectError(sipcc::MediaConstraints& constraints) {
How is this expecting error? It seems to be expecting success.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:777:
ASSERT_TRUE_WAIT(pObserver->state != TestObserver::stateNoResponse,
Again, can we just ASSERT_TRUE_WAIT(stateSucces).

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:784: uint32_t sdpCheck =
DONT_CHECK_AUDIO|
You're killing me with the default arguments.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:801: 
Extra whitespace.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:804:
ASSERT_TRUE_WAIT(pObserver->state != TestObserver::stateNoResponse,
ASSERT_TRUE_WAIT again...

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:827:
ASSERT_EQ(pc->RemoveStream(domMediaStream_), NS_OK);
Maybe we should remove this entirely since it's never called and kinda hosed.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:842:
sipcc::PeerConnectionImpl::kSignalingInvalid) {
Default arguments again

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:848: }
This seems like we could just ask the caller to provide the value rather than
having this logic.

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:868:
sipcc::PeerConnectionImpl::kSignalingStable);
Again, I'm kinda sad about this logic. Let's just pass the value explicitly

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:1173:
a1_.CreateOffer(aconstraints, offerAnswerFlags, offerSdpCheck);
Just to confirm, you have added the checks in the calls?

https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/signaling...
media/webrtc/signaling/test/signaling_unittests.cpp:2231: //
SetRemoteDescription call fails.
What else would it be?
(Assignee)

Comment 58

5 years ago
Created attachment 758029 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection

Bug 784519 - Part 3: Send Signaling State from SIPCC to PeerConnection
(Assignee)

Updated

5 years ago
Attachment #755405 - Attachment is obsolete: true
(Assignee)

Comment 59

5 years ago
Created attachment 758042 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection

Bug 784519 - Part 3: Send Signaling State from SIPCC to PeerConnection
(Assignee)

Updated

5 years ago
Attachment #758029 - Attachment is obsolete: true
(Assignee)

Comment 60

5 years ago
Created attachment 758044 [details] [diff] [review]
Part 4: signalingState mochi tests
(Assignee)

Comment 61

5 years ago
Created attachment 758045 [details] [diff] [review]
Part 5: signalingState unit tests
(Assignee)

Comment 62

5 years ago
Comment on attachment 758044 [details] [diff] [review]
Part 4: signalingState mochi tests

Carrying forward r+ from jsmith
Attachment #758044 - Attachment description: Part 4: signalingState tests → Part 4: signalingState mochi tests
Attachment #758044 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #755735 - Attachment is obsolete: true
(Assignee)

Comment 63

5 years ago
I went ahead and split the tests (previously patch 4) into two parts, now patches 4 and 5. They have different reviewers, live in different parts of the tree, and are likely to land at different times -- so splitting them up seemed to make the most sense.
(Assignee)

Updated

5 years ago
Attachment #758042 - Flags: review?(ekr)
(Assignee)

Comment 64

5 years ago
Just making sure I didn't botch anything in the final round of changes (this includes patches 3 and 4, but not 5):
https://tbpl.mozilla.org/?tree=Try&rev=17d40635ff27
(Assignee)

Comment 65

5 years ago
Comment on attachment 758042 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection

[Copying review from reitveld, with an r+ via IM]

LGTM with one bug below.


https://firefox-codereview.appspot.com/17001/diff/23001/dom/media/PeerConnection.js
File dom/media/PeerConnection.js (right):

https://firefox-codereview.appspot.com/17001/diff/23001/dom/media/PeerConnection.js#newcode1015
dom/media/PeerConnection.js:1015:
this._dompc.changeIceConnectionState("completed");
This doesn't look correct. kIceWaiting means that all gathering is done.

I believe this is still "new"

https://firefox-codereview.appspot.com/17001/diff/23001/dom/media/PeerConnection.js#newcode1031
dom/media/PeerConnection.js:1031:
this._dompc.changeIceConnectionState("connected");
So we never go to completed? I guess that's because nICEr doesn't...

https://firefox-codereview.appspot.com/17001/diff/23001/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
(right):

https://firefox-codereview.appspot.com/17001/diff/23001/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#newcode1282
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1282:
CSFLogDebug(logTag, "%s: for %p", __FUNCTION__, (void *) this);
Suggest logging the handle rather than the pointer. Though I guess this
is another patch.

https://firefox-codereview.appspot.com/17001/
Attachment #758042 - Flags: review?(ekr) → review+
(Assignee)

Comment 66

5 years ago
(In reply to Adam Roach [:abr] from comment #65)

> Suggest logging the handle rather than the pointer. Though I guess this
> is another patch.

I'm fixing the ICE state problem, but the logging issue will be resolved as Bug 879477.
(Assignee)

Comment 67

5 years ago
Comment on attachment 758042 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection

https://hg.mozilla.org/integration/mozilla-inbound/rev/90968836cce3
Attachment #758042 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #758042 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #758044 - Flags: checkin+
(Assignee)

Comment 70

5 years ago
Okay, that's really strange. I just ran crashtests locally several times, and didn't see the crash/exit problem from that log even once. The push log looks like it happened for every run. I wonder whether it's some other patch -- or possibly a clobber issue. In any case, I'm pushing another try, this time for crashtests, to see if the problem is real:

https://tbpl.mozilla.org/?tree=Try&rev=0f53d190e647
(Assignee)

Comment 71

5 years ago
I just noticed that the m-i landing had problems in mochi-3 (which I similarly cannot repro locally). I've cancelled the previous try and issued a new one to run crashtests and mochi-3:

https://tbpl.mozilla.org/?tree=Try&rev=3b9d282a4084
(Assignee)

Comment 72

5 years ago
Argh. The try/hg foo failed for that push. Retrying:
https://tbpl.mozilla.org/?tree=Try&rev=237234896a59
(Assignee)

Comment 73

5 years ago
Aside from some unrelated issues with debug builds, that push looks good for mochi-3 and crashers. Relanding:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/263154173cc8
   https://hg.mozilla.org/integration/mozilla-inbound/rev/347f88a0effa
(Assignee)

Comment 74

5 years ago
Backing out:
   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5187458d46
   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd648fee58ec

---

The current theory is that this requires an update to CLOBBER in order to work.

> philor: 'and if it works file a bug against build config, "going from aaabbb to cccddd required a clobber"'
(Assignee)

Comment 75

5 years ago
To be clear, the component that a "needs clobber" bug would be filed against is: 

https://bugzilla.mozilla.org/enter_bug.cgi?product=mozilla.org&component=Release%20Engineering%3A%20Automation%20%28General%29
(Assignee)

Comment 76

5 years ago
I'm going to watch the Android clobber build here before I try to reland:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=347f88a0effa
(Assignee)

Comment 77

5 years ago
Okay. The clobber build on Android was clean. Also, all the builds that hadn't started by the time philor clobbered the tree have clean mochi and crash (yaay!) while the ones that were already going at that time failed, just like before. So I think the magic chicken to wave for this patchset is touching CLOBBER (which I added to the Part 3 patch).

Third time's a charm; keeping my fingers crossed:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/017bc63ae2fa
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ca78349dea64
No, it would be against https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Build%20Config - release engineering runs commands, not quite "make -f client.mk build" but roughly that; it's build config that either reviewed a makefile that doesn't actually work, or is going to yell at someone for not getting review from a build config peer for their makefile patch which doesn't actually work.
(Assignee)

Comment 80

5 years ago
Build system bug for Android build failures: Bug 880702

The mochi/crash failures appear to have been my own fault (doh!), as I didn't update the UUID in IPeerConnection.idl.
(Assignee)

Updated

5 years ago
Attachment #758042 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #758044 - Flags: checkin+
(Assignee)

Comment 81

5 years ago
(In reply to Eric Rescorla (:ekr) from comment #57)
> Comment on attachment 755735 [details] [diff] [review]
> Part 4: signalingState tests
> 
> Review of attachment 755735 [details] [diff] [review]:
> -----------------------------------------------------------------
...
> Or alternately, since you don't really seem to use it with anything
> complicated,
> how about a static function along the lines of:
> 
> void WriteWithPrefix(const std::string& prefix, const std::string &stuff)

I've gone with this instead:
  std::string indent(const std::string &s, int width = 4)

> media/webrtc/signaling/test/signaling_unittests.cpp:66: int overflow (int c)
> {
> Shouldn't this be virtual?

Just as an aside here (irrelevant since the class has gone away), but declaring a method as virtual in one class is sufficient to cause it to be virtual in all its descendants. It's a matter of style whether you consider redeclaring such methods as virtual to add clarity or clutter.


> https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/
> signaling...
> media/webrtc/signaling/test/signaling_unittests.cpp:76: private:
> Isn't this nonportable to systems with '\r' as the end of line?

Not with streams. The mapping from "\n" to the underlying platform's notion of "end-of-line" happens at a lower layer than we're operating at here.


> https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/
> signaling...
> media/webrtc/signaling/test/signaling_unittests.cpp:675: return
> static_cast<sipcc::PeerConnectionImpl::SignalingState>(res);
> Should we check that res is in-range?

This is entirely compile-time typechecking sugar. All we would gain from enforcing a range on it would be loss of information. If the state comes out as something unexpected and out of range, I would strongly prefer it show up at the ASSERT_EQ() statement that makes use of signaling_state().

 
> https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/
> signaling...
> media/webrtc/signaling/test/signaling_unittests.cpp:742:
> sipcc::PeerConnectionImpl::kSignalingStable) {
> I think I would prefer that we actually explicitly passed endState. Or
> alternately, make two variants of CreateOffer. Default arguments kinda suck?

Ugh. The prospect of making redundant copies of a method seems rather extreme overkill. Is your assertion that default arguments as a general programming level construct are bad? If so, I disagree vehemently. I would understand objections if I were using, say, variadic functions here; but default arguments contribute significantly to code readability unless the defaults are just flat-out broken.



> https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/
> signaling...
> media/webrtc/signaling/test/signaling_unittests.cpp:769:
> ASSERT_EQ(pObserver->state, TestObserver::stateSuccess);
> Is there a reason why you can't just ASSERT_TRUE_WIAT(...stateSuccess);

Yes. If the state changes from stateNoResponse to stateError, we want to stop waiting right away rather than hanging around for the timeout to fire.


> https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/
> signaling...
> media/webrtc/signaling/test/signaling_unittests.cpp:775: void
> CreateOfferExpectError(sipcc::MediaConstraints& constraints) {
> How is this expecting error? It seems to be expecting success.

I think you'd need to ask Enda. In any case, it appears to be unused, so I'm removing it.


> https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/
> signaling...
> media/webrtc/signaling/test/signaling_unittests.cpp:827:
> ASSERT_EQ(pc->RemoveStream(domMediaStream_), NS_OK);
> Maybe we should remove this entirely since it's never called and kinda hosed.

I think, if we remove it, we run the risk of naïvely forgetting to put it back in after Bug 840728 is fixed.



> https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/
> signaling...
> media/webrtc/signaling/test/signaling_unittests.cpp:848: }
> This seems like we could just ask the caller to provide the value rather than
> having this logic.

Given that the correct ending state is trivially derivable from the inputs in all but degenerate cases, that seems to be a far more intrusive approach than providing defaults that are correct in the vast majority of the cases, with overrides necessary only for corner cases. You're talking about needlessly complicating on the order of 64 calling sites.

It kinda seems like the primary use case for which default arguments were introduced to the language.


> https://firefox-codereview.appspot.com/18001/diff/4001/media/webrtc/
> signaling...
> media/webrtc/signaling/test/signaling_unittests.cpp:2231: //
> SetRemoteDescription call fails.
> What else would it be?

One could easily imagine bad logic in sipcc setting it to closed ("something went wrong! abort!") or stable ("I wanted a remote description and got a remote description, let's start this session"), both of which would be an error that I want the unit test driver to catch.
(Assignee)

Comment 82

5 years ago
Created attachment 759862 [details] [diff] [review]
Part 5: signalingState unit tests
(Assignee)

Updated

5 years ago
Attachment #758045 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #759862 - Flags: review?(ekr)
Comment on attachment 758044 [details] [diff] [review]
Part 4: signalingState mochi tests

Review of attachment 758044 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/head.js
@@ +181,5 @@
>     */
>    return function(aObj) {
>      var where = error.fileName + ":" + error.lineNumber;
>      if (aObj && aObj.name && aObj.message) {
> +      ok(false, "Unexpected callback/event from " + where + " with name = '" +

So far I can see this method is for callbacks only. Over on bug 796894 (which has not been landed yet) I added another method specifically for events. I don't think we should fold in events here.

::: dom/media/tests/mochitest/pc.js
@@ +410,5 @@
> + * happens, then the next test in the command chain is triggered. After
> + * running, this sets the event handler so that it will fail the test if
> + * it fires again before we expect it. This is intended to be used in
> + * conjunction with checkStateInCallback, below.
> + *

Can we do it similarly to what I do in the datachannel patch? If it really should only fire once, we might want to also catch unexpected events of this type.

@@ +423,5 @@
> +function PCT_expectStateChange(pcw, state, test) {
> +  pcw.signalingChangeEvent = false;
> +  pcw._pc.onsignalingstatechange = function() {
> +    pcw._pc.onsignalingstatechange = unexpectedCallbackAndFinish(new Error);
> +    is(pcw._pc.signalingState, state, pcw.label + ": State is " + state + " in onsignalingstatechange");

I have been told for my patch not to add checks like those inside the framework but in the template.

@@ +489,5 @@
>      self.attachMedia(event.stream, 'video', 'remote');
>    };
> +
> +  // Make sure no signaling state changes are fired until we expect them to
> +  this._pc.onsignalingstatechange = unexpectedCallbackAndFinish(new Error);

This will not correctly print the event details in the error callback.
Comment on attachment 758044 [details] [diff] [review]
Part 4: signalingState mochi tests

Review of attachment 758044 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/pc.js
@@ +265,5 @@
>    ],
>    [
>      'PC_LOCAL_SET_LOCAL_DESCRIPTION',
>      function (test) {
> +      test.expectStateChange(test.pcLocal, "have-local-offer", test);

Also any reason why you are passing in the test as last argument? This method is already a member of the test and knows about any internal state.
Comment on attachment 758044 [details] [diff] [review]
Part 4: signalingState mochi tests

Review of attachment 758044 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/pc.js
@@ +426,5 @@
> +    pcw._pc.onsignalingstatechange = unexpectedCallbackAndFinish(new Error);
> +    is(pcw._pc.signalingState, state, pcw.label + ": State is " + state + " in onsignalingstatechange");
> +    pcw.signalingChangeEvent = true;
> +    if (pcw.commandSuccess) {
> +      test.next();

Sorry to add one more note but this totally busts the way how templates can be extended. None 
of our framework methods should call test.next() itself, but should always allow to specify a onSuccess callback which will be called instead. test.next() should only be called from the templates or custom steps of the command chain.

This makes it kinda hard to to update my datachannel framework changes. :/ Can we get this fixed please?
Henrik - The mochitest patches are already landed at this point. File a followup for the fixes needed here.
This bug is still open, so I do not see the necessity to file a new bug for it. Lets wait for Adam's reply and if he wants to fix it. I don't really have the time for that. So long I'm blocked with my datachannel test patch.
Flags: needinfo?(adam)
(In reply to Henrik Skupin (:whimboo) from comment #87)
> This bug is still open, so I do not see the necessity to file a new bug for
> it. Lets wait for Adam's reply and if he wants to fix it. I don't really
> have the time for that. So long I'm blocked with my datachannel test patch.

Why are these patch changes blocking you on your data channel automation patch?
Blocks: 796894
(In reply to Jason Smith [:jsmith] from comment #88)
> Why are these patch changes blocking you on your data channel automation
> patch?

See comment 85?
(In reply to Henrik Skupin (:whimboo) from comment #89)
> (In reply to Jason Smith [:jsmith] from comment #88)
> > Why are these patch changes blocking you on your data channel automation
> > patch?
> 
> See comment 85?

That tests as written here we're not designed to handle the data channel framework you were working on - they were written before the updated patches showed up on your bug. These are valid comments, but I'd expect this would need to be fixed on your patch, not this patch.
I don't want to start a large discussion on that while the problem is most likely also clear for Adam. Just in short again, the code as pointed out above should NOT call test.next() but should execute a given onSuccess() callback, so that other code in the same command can hook in. As of now both expectStateChange() and checkStateInCallback() call test.next() and given that I have to do event checks in the same command, only signaling OR datachannel tests can be executed but not both.
(Assignee)

Comment 92

5 years ago
Comment on attachment 758044 [details] [diff] [review]
Part 4: signalingState mochi tests

Review of attachment 758044 [details] [diff] [review]:
-----------------------------------------------------------------

Henrik -- I'm sympathetic to the fact that your patch has rotted due to the changes I've landed, but the vast majority of your comments functionally resolve to my failure to predict what you would be doing in the future. I think most (if not all) of the changes you propose need to be included in your impending patch.

::: dom/media/tests/mochitest/head.js
@@ +181,5 @@
>     */
>    return function(aObj) {
>      var where = error.fileName + ":" + error.lineNumber;
>      if (aObj && aObj.name && aObj.message) {
> +      ok(false, "Unexpected callback/event from " + where + " with name = '" +

For the event that currently calls this, the details of the event are unimportant. If you want to redirect the onsignalingstatechange event to use your new function as part of your impending patch, feel free -- it doesn't help, but it doesn't hurt.

::: dom/media/tests/mochitest/pc.js
@@ +265,5 @@
>    ],
>    [
>      'PC_LOCAL_SET_LOCAL_DESCRIPTION',
>      function (test) {
> +      test.expectStateChange(test.pcLocal, "have-local-offer", test);

I didn't chase it to ground, but my original attempt (in which I passed in test.next) failed because "test" ended up being undefined by the time the JS got around to calling next().

@@ +410,5 @@
> + * happens, then the next test in the command chain is triggered. After
> + * running, this sets the event handler so that it will fail the test if
> + * it fires again before we expect it. This is intended to be used in
> + * conjunction with checkStateInCallback, below.
> + *

This does check unexpected events of this type -- note that the signal handler sets "pcw._pc.onsignalingstatechange = unexpectedCallbackAndFinish(new Error);"

@@ +423,5 @@
> +function PCT_expectStateChange(pcw, state, test) {
> +  pcw.signalingChangeEvent = false;
> +  pcw._pc.onsignalingstatechange = function() {
> +    pcw._pc.onsignalingstatechange = unexpectedCallbackAndFinish(new Error);
> +    is(pcw._pc.signalingState, state, pcw.label + ": State is " + state + " in onsignalingstatechange");

That's where I originally had it, but Jason wanted to ensure that the check worked regardless of which order the event versus the callback happened (hence this and the following function). If you can come up with an alternate design that makes you happier and still achieves the same goal, feel free to move things around.

@@ +426,5 @@
> +    pcw._pc.onsignalingstatechange = unexpectedCallbackAndFinish(new Error);
> +    is(pcw._pc.signalingState, state, pcw.label + ": State is " + state + " in onsignalingstatechange");
> +    pcw.signalingChangeEvent = true;
> +    if (pcw.commandSuccess) {
> +      test.next();

I tried to do that, but (as I mention above), passing test.next() as a success callback didn't work.

@@ +489,5 @@
>      self.attachMedia(event.stream, 'video', 'remote');
>    };
> +
> +  // Make sure no signaling state changes are fired until we expect them to
> +  this._pc.onsignalingstatechange = unexpectedCallbackAndFinish(new Error);

The event details, in this case, are completely worthless. All we need to do is determine that the event happened when it shouldn't have.
(Assignee)

Updated

5 years ago
Flags: needinfo?(adam)

Updated

5 years ago
Attachment #759862 - Flags: review?(ekr) → review+
Blocks: 881658
No longer blocks: 796894
(Assignee)

Comment 93

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40c7b5d714d
Whiteboard: [WebRTC], [blocking-webrtc-], [leave-open] → [WebRTC], [blocking-webrtc-]
(Assignee)

Updated

5 years ago
Attachment #759862 - Flags: checkin+
(Assignee)

Comment 94

5 years ago
Copying EKR's final review for Part 5 here:

lgtm with a minor nit


http://firefox-codereview.appspot.com/18001/diff/12001/media/webrtc/signaling/test/signaling_unittests.cpp
File media/webrtc/signaling/test/signaling_unittests.cpp (right):

http://firefox-codereview.appspot.com/18001/diff/12001/media/webrtc/signaling/test/signaling_unittests.cpp#newcode1954
media/webrtc/signaling/test/signaling_unittests.cpp:1954: <<
indent(a1_.getLocalDescription()) << endl << endl;
Please either use std:: or not consistently on the same line of code
here and below.

http://firefox-codereview.appspot.com/18001/
https://hg.mozilla.org/mozilla-central/rev/c40c7b5d714d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Marking verified per automated tests landing with the patch running cleanly without blowing up on try.
Status: RESOLVED → VERIFIED

Updated

5 years ago
Depends on: 952145
You need to log in before you can comment on or make changes to this bug.