Enforce State Transition Rules in SIPCC

VERIFIED FIXED in mozilla24

Status

()

defect
P1
normal
VERIFIED FIXED
7 years ago
6 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
SIPCC doesn't enforce states very well when faced with error conditions.
Whiteboard: [WebRTC], [blocking-webrtc+]
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.
Assignee: nobody → snandaku
I keep getting outstanding defect emails about this defect, is there any movement on this work?
Attachment #671155 - Attachment is obsolete: true
Attachment #671155 - Flags: review?(ethanhugg)
Attachment #671155 - Flags: review?(emannion)
Attachment #671155 - Flags: review?(ekr)
Assignee: snandaku → adam
Duplicate of this bug: 797516
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-]
Priority: P2 → P1
Attachment #742497 - Attachment is obsolete: true
Attachment #743712 - Attachment is obsolete: true
Attachment #743716 - Attachment is obsolete: true
Attachment #743797 - Attachment is obsolete: true
Posted patch Part 4: signalingState tests (obsolete) — Splinter Review
Attachment #742379 - Flags: review?(ethanhugg)
Attachment #742502 - Flags: review?(rjesup)
Attachment #745895 - Flags: review?(ekr)
Attachment #745896 - Flags: review?(jsmith)
Attachment #742502 - Flags: review?(rjesup) → review+
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-
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?
(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.
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.
Attachment #745895 - Attachment is obsolete: true
Attachment #745895 - Flags: review?(ekr)
Attachment #742379 - Attachment is obsolete: true
Attachment #742502 - Attachment is obsolete: true
Attachment #750792 - Attachment is obsolete: true
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+
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+
Comment on attachment 750797 [details] [diff] [review]
Part 3: Send Signaling State from SIPCC to PeerConnection

Un-bit-rotting
Attachment #750797 - Flags: review?(ekr)
Posted patch Part 4: signalingState tests (obsolete) — Splinter Review
Attachment #745896 - Attachment is obsolete: true
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-
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+
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?
(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.
Whoops; didn't update the Makefile.in for the previous try. Re-trying:

https://tbpl.mozilla.org/?tree=Try&rev=42486f4f99de
Attachment #750795 - Flags: checkin+
Attachment #750796 - Flags: checkin+
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.
(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.
(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).
Attachment #750797 - Attachment is obsolete: true
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)
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.
(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.
Posted patch Part 4: signalingState tests (obsolete) — Splinter Review
Attachment #750854 - Attachment is obsolete: true
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)
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
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?
Bug 784519 - Part 3: Send Signaling State from SIPCC to PeerConnection
Attachment #755405 - Attachment is obsolete: true
Bug 784519 - Part 3: Send Signaling State from SIPCC to PeerConnection
Attachment #758029 - Attachment is obsolete: true
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+
Attachment #755735 - Attachment is obsolete: true
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.
Attachment #758042 - Flags: review?(ekr)
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
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+
(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.
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+
Attachment #758042 - Flags: checkin+
Attachment #758044 - Flags: checkin+
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
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
Argh. The try/hg foo failed for that push. Retrying:
https://tbpl.mozilla.org/?tree=Try&rev=237234896a59
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
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"'
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
I'm going to watch the Android clobber build here before I try to reland:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=347f88a0effa
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.
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.
Attachment #758042 - Flags: checkin+
Attachment #758044 - Flags: checkin+
(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.
Attachment #758045 - Attachment is obsolete: true
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?
(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.
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.
Flags: needinfo?(adam)
Attachment #759862 - Flags: review?(ekr) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40c7b5d714d
Whiteboard: [WebRTC], [blocking-webrtc-], [leave-open] → [WebRTC], [blocking-webrtc-]
Attachment #759862 - Flags: checkin+
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
Closed: 6 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
You need to log in before you can comment on or make changes to this bug.