790517 - ICE, DTLS, and generic transport stack for WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Patch 1: Import nrappkit; generic application development kit/portable runtime; Dependency for nICEr. This is an import with a small number of changes that will be upstreamed soon.

Comment 2

[Eric Rescorla (:ekr)]

Attachment: Patch 2: nICEr; ICE stack; This is an import with a small number of changes that we plan to upstream.

Comment 3

[Eric Rescorla (:ekr)]

Attachment: Patch 3: build nICEr and nrappkit

Comment 4

[Eric Rescorla (:ekr)]

Attachment: Patch 4: Generic media transport subsystem with modules for ICE and DTLS

Comment 5

[Eric Rescorla (:ekr)]

IMPORTANT NOTES FOR REVIEWERS:

1. Much of this code is preexisting code. Any suspected security issues MUST be marked as sensitive/private.
2. This code depends on the sigslot package which has not yet been imported. See:
https://hg.mozilla.org/projects/alder/file/704aa4c3234d/media/webrtc/trunk/third_party/libjingle/source/talk/base/sigslot.h
3. This code depends on NSS 3.14, which has not yet been released or uplifted into m-c.

For reasons 2 and 3 above, this patchset currently will not compile in m-c.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Bug 766689 has the licensing patch, r+'d by gerv

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 660319 [details] [diff] [review]
Patch 3: build nICEr and nrappkit

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

::: media/mtransport/third_party/Makefile.in
@@ +1,2 @@
> +# ***** BEGIN LICENSE BLOCK *****
> +# Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL 2 (and was cpearce involved?  Doesn't matter if it's MPL2 anyways)

::: media/mtransport/third_party/import.py
@@ +13,5 @@
> +
> +def die(msg):
> +    sys.stderr.write('ERROR:' + msg + '\n')
> +    sys.exit(1)
> +    

Trailing spaces

@@ +43,5 @@
> +    
> +
> +
> +            
> +            

trailing lines/spaces

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

The local variable style of foo_ is a Googlism, but it is used consistently.  I'll ping some others on if they need to have it match mozilla mFoo style.

r- for now.  I'll add r? for a netwerk peer and ted for make stuff, and bsmith for transportlayerdtls

::: media/mtransport/build/Makefile.in
@@ +1,2 @@
> +# ***** BEGIN LICENSE BLOCK *****
> +# Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL2 (entire module, unless there's a reason not to)

::: media/mtransport/dtlsidentity.cpp
@@ +1,1 @@
> +#include "nspr.h"

No license

@@ +16,5 @@
> +
> +// Helper class to avoid having a crapload of if (!NULL) statements at
> +// the end to clean up. The way you use this is you instantiate the
> +// object as scoped_c_ptr<PtrType> obj(value, destructor);
> +// TODO([e-mail]): Move this to some generic location

I suspect TODO(email) isn't good for checkin; that's a google-ism.  Just TODO:
Check the entire module
Should we open a bug on this, or break this out now, or leave as TODO?

@@ +54,5 @@
> +  }
> +
> +  PK11RSAGenParams rsaparams;
> +  rsaparams.keySizeInBits = 1024;
> +  rsaparams.pe = 0x010001;

Opaque constants - may be ok, but as a non-RSA person the second means nothing

@@ +212,5 @@
> +  std::string str("");
> +  char group[3];
> +  
> +  for (std::size_t i=0; i < size; i++) {
> +    PR_snprintf(group, 3, "%.2x", digest[i]);

3 -> sizeof(group)

::: media/mtransport/logging.h
@@ +17,5 @@
> +  LogCtx(const char* name) : module_(PR_NewLogModule(name)) {}
> +  LogCtx(std::string& name) : module_(PR_NewLogModule(name.c_str())) {}
> +
> +  PRLogModuleInfo* module() const { return module_; }
> +  

trailing space  - quite a few files show this; they're easy to find in Splinter Review

::: media/mtransport/m_cpp_utils.h
@@ +8,5 @@
> +#define m_cpp_utils_h__
> +
> +#include "mozilla/Attributes.h"
> +
> +#define DISALLOW_ASSIGNMENT(T) \

Don't we have copies of these in mfbt?

::: media/mtransport/nr_socket_prsock.cpp
@@ +183,5 @@
> +  PRNetAddr *naddr) 
> +  {
> +    int _status;
> +    
> +    memset(naddr, 0, sizeof(PRNetAddr));

Stylistic, but I tend to prefer sizeof(*naddr) to avoid type changes messing me up - but this is super-minor and arguable

@@ +213,5 @@
> +        memcpy(naddr->ipv6.ip._S6_un._S6_u8,
> +               &addr->u.addr6.sin6_addr.__u6_addr.__u6_addr8, 16);
> +#endif
> +#else
> +        // TODO([e-mail]): make IPv6 work

File a bug please.

::: media/mtransport/nr_socket_prsock.h
@@ +95,5 @@
> +  void fire_callback(int how);
> +
> +  PRFileDesc *fd_;
> +  nr_transport_addr my_addr_;
> +  NR_async_cb cbs_[2];

It would be nice if this weren't a raw number; I realize nrappkit doesn't export a number of items in the 'enum' - either add something to that, or use WAIT_WRITE+1 here

::: media/mtransport/nr_timer.cpp
@@ +50,5 @@
> +                       int l, void **handle) {
> +  nsresult rv;
> + 
> +  // TODO([e-mail]): See if this is too expensive and if so cache
> +  nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);

Resolve this one please.  I'll note that almost all instances in the tree assign to an mBlahTimer.  It's not insanely expensive, but likely best to cache.

@@ +59,5 @@
> +
> +  timer->InitWithCallback(new nrappkitTimerCallback(cb, arg), timeout,
> +                           nsITimer::TYPE_ONE_SHOT);
> +  
> +  timer->AddRef();

Why AddRef() here?  I'll note that nsDocShell.cpp uses this idiom, and doesn't AddRef() here and release on callback - see OnPingTimeout())

@@ +74,5 @@
> +
> +int NR_async_timer_cancel(void *handle) {
> +  nsITimer *timer = static_cast<nsITimer *>(handle);
> +  
> +  timer->Cancel();

Do you need to Release() here?

::: media/mtransport/nricectx.cpp
@@ +188,5 @@
> +  // Streams which do not exist should never fail.
> +  PR_ASSERT(s);
> +
> +  ctx->SetState(ICE_CTX_FAILED);
> +  s -> SignalFailed(s);

remove spaces

@@ +235,5 @@
> +    nr_crypto_vtbl = &nr_ice_crypto_nss_vtbl;
> +    initialized = true;
> +    
> +    // Set the priorites for candidate type preferences
> +    NR_reg_set_uchar((char *)"ice.pref.type.srv_rflx",100);

Are all these priorities correct?  Do we need a bug on setting them?  What's the overall plan here?

@@ +268,5 @@
> +      NR_reg_set_uchar((char *)"ice.pref.interface.virbr0", 233);
> +      NR_reg_set_uchar((char *)"ice.pref.interface.wlan0", 232);
> +    }
> +    
> +    NR_reg_set_string((char *)"ice.stun.server.0.addr", (char *)"216.93.246.14");

Comment that this needs attention; open a bug to fix it if it isn't open (it may be)

::: media/mtransport/nricemediastream.cpp
@@ +181,5 @@
> +         << name_ << "'");
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  char addr[64];

Where does 64 come from?  Any case where it's not enough?  Is it always much more than needed?

::: media/mtransport/standalone/Makefile.in
@@ +48,5 @@
> +MODULE = mtransport_s
> +LIBRARY_NAME = mtransport_s
> +FORCE_STATIC_LIB= 1
> +ifeq (WINNT,$(OS_TARGET))
> +VISIBILITY_FLAGS =

Do we really need this?

@@ +74,5 @@
> +	$(NULL)
> +
> +
> +export:: $(MTRANSPORT_CPPSRCS)
> +	$(INSTALL) $^ .

Ok, this is an odd construct; please at least comment what we're doing here and especially why (LCPPSSRCS/INSTALL/etc)

::: media/mtransport/test/Makefile.in
@@ +85,5 @@
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/plugin \
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/event \
> + $(NULL)
> +
> +# SCTP DEFINES

All the SCTP defines can be merged down to a simple -D__Userspace_os_foo=1 - see netwerk/sctp/datachannel/Makefile.in

@@ +146,5 @@
> +  nrappkit_unittest.cpp \
> +  sockettransportservice_unittest.cpp \
> +  transport_unittests.cpp \
> +  runnable_utils_unittest.cpp \
> +#  sctp_unittest.cpp \

Is this still broken?  Should be ifdef MOZ_SCTP

::: media/mtransport/test/sctp_unittest.cpp
@@ +25,5 @@
> +#include "mtransport_test_utils.h"
> +#include "runnable_utils.h"
> +
> +// XXX FIX
> +#include "../../../netwerk/sctp/src/usrsctp.h"

Add a -I to the Makefile/gyp

::: media/mtransport/test/sockettransportservice_unittest.cpp
@@ +111,5 @@
> +
> +  void OnSocketReady(PRFileDesc *fd, PRInt16 outflags) {
> +    unsigned char buf[1600];
> +    
> +    PRInt32 rv;

Please scrub module for PRintX -> intx_t, etc

::: media/mtransport/transportflow.cpp
@@ +28,5 @@
> +    old_layer->SignalStateChange.disconnect(this);
> +    old_layer->SignalPacketReceived.disconnect(this);
> +  }
> +  layer->SignalStateChange.connect(this, &TransportFlow::StateChange);
> +  layer->SignalPacketReceived.connect(this, &TransportFlow::PacketReceived);

I assume .disconnect/.connect are 'safe' and also don't lose signals?

::: media/mtransport/transportlayer.h
@@ +7,5 @@
> +#ifndef transportlayer_h__
> +#define transportlayer_h__
> +
> +// Pull in sigslot from libjingle. If we remove libjingle, we will
> +// need to either bring that in separately or refactor this code

This comment is probably moot

::: media/mtransport/transportlayerdtls.cpp
@@ +242,5 @@
> +
> +static PRStatus TransportLayerGetpeername(PRFileDesc *f, PRNetAddr *addr) {
> +  // TODO([e-mail]): Modify to return unique names for each channel
> +  // somehow, as opposed to always the same static address. The current
> +  // implementation messes up the session cache, which is why it's off

Since we're looking to check this in, please file a bug for this issue

@@ +411,5 @@
> +  return NS_OK;
> +}
> +
> +// TODO([e-mail]): make sure this is called from STS. Otherwise
> +// we have thread safety issues

Sounds like a bug should be filed

@@ +559,5 @@
> +      MLOG(PR_LOG_ERROR, LAYER_INFO << "State change of lower layer to INIT forbidden");
> +      SetState(ERROR);
> +      break;
> +    case CONNECTING:
> +      break;

Add a blank line before CONNECTING for consistency - and is there a reason there's nothing at all in this case (no SetState), not even a log?

@@ +644,5 @@
> +  }
> +
> +  // Now try a recv if we're open, since there might be data left
> +  if (state_ == OPEN) {
> +    unsigned char buf[2000];

What if the MTU > 2000?  (9K MTU GigE, FDDI, etc)
Why 2000?

@@ +728,5 @@
> +  return SECSuccess;
> +}
> +
> +nsresult TransportLayerDtls::SetSrtpCiphers(std::vector<PRUint16> ciphers) {
> +  // TODO([e-mail]): We should check these

Is this a must-do, would-be-nice, ?
Do we need a bug on this?

@@ +828,5 @@
> +  PR_ASSERT(peer_cert_ == NULL);
> +
> +  switch (verification_mode_) {
> +    case VERIFY_UNSET:
> +      // Jump to error exit

Is there a missing goto or return here?  Maybe just delete the comment, or // cleanup cert and return failure

@@ +855,5 @@
> +        // Matches all digests, we are good to go
> +        peer_cert_ = peer_cert;
> +        return SECSuccess;
> +      }
> +    }

break; (or 1 line up)

::: media/mtransport/transportlayerdtls.h
@@ +62,5 @@
> +
> +
> +  enum Role { CLIENT, SERVER};
> +  enum Verification { VERIFY_UNSET, VERIFY_ALLOW_ALL, VERIFY_DIGEST};
> +  const static int kMaxDigestLength = 64;

Does this value come from somewhere external?  Spec, or NSS?

::: media/mtransport/transportlayerloopback.cpp
@@ +22,5 @@
> +#include "transportlayerloopback.h"
> +
> +MLOG_INIT("mtransport");
> +
> +nsresult TransportLayerLoopback::Init() {

How is loopback used?  Is it needed in non-Debug builds?

::: media/mtransport/transportlayerprsock.cpp
@@ +87,5 @@
> +  }
> +
> +  MLOG(PR_LOG_DEBUG, LAYER_INFO << "OnSocketReady(flags=" << outflags << ")");
> +  
> +  unsigned char buf[1600];

What if the MTU >1600 bytes?  (GigE, fddi, etc)
Why 1600?  (and 2000 elsewhere)
Should it be "unsigned char" or "uint8_t"?  (This probably applies a bunch of places)

::: toolkit/toolkit-tiers.mk
@@ -136,2 @@
>    media/webrtc \
> -  media/mtransport/third_party \

This is supposedly removed by the patch, but it's not in m-c...

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

r? Josh for networking (please redirect to other network peers as appropriate if you aren't comfortable doing it - biesi, mcmanus?)
r? ted for makefiles in here.  One in particular you'll want to look at (see my review)
r? bsmith for transportlayerdtls.*

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 660318 [details] [diff] [review]
Patch 2: nICEr; ICE stack; This is an import with a small number of changes that we plan to upstream.

Marking for review by networking and build; note this is an imported existing library (and imported by the author), so I would not expect a line-by-line review.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 660317 [details] [diff] [review]
Patch 1: Import nrappkit; generic application development kit/portable runtime; Dependency for nICEr. This is an import with a small number of changes that will be upstreamed soon.

Marking for review; note this is an imported existing library (and imported by the author), so I would not expect a line-by-line review.

Roc, if you feel we need to have someone different review this import, let us know.  Gerv has ok'd the licenses.  (bsmedberg or ehsan are possible, maybe dbaron).

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 660317 [details] [diff] [review]
Patch 1: Import nrappkit; generic application development kit/portable runtime; Dependency for nICEr. This is an import with a small number of changes that will be upstreamed soon.

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

I'm not thrilled about importing yet another platform abstraction library, but at least this one is fairly lightweight.

Why is there a version of queue.h for each platform? They look the same.

How much of this are we actually going to use? For example, are we actually going to be spawning the registry daemon?

Comment 13

[Eric Rescorla (:ekr)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)

> I'm not thrilled about importing yet another platform abstraction library,
> but at least this one is fairly lightweight.

Me neither, but rewriting nICEr to use an existing abstraction seemed
even worse.


> Why is there a version of queue.h for each platform? They look the same.

They are. It's an artifact of our portability system. Originally this code
ran on bsd which didn't need it, so this is how it gets filled in for
other platforms.


> How much of this are we actually going to use? For example, are we actually
> going to be spawning the registry daemon?

I have tried to remove things which are not needed. For instance registryd is not
included, unless I screwed up.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Eric Rescorla from comment #13)
> > Why is there a version of queue.h for each platform? They look the same.
> 
> They are. It's an artifact of our portability system. Originally this code
> ran on bsd which didn't need it, so this is how it gets filled in for
> other platforms.

Would it be too much to ask to avoid that duplication?

> I have tried to remove things which are not needed. For instance registryd
> is not
> included, unless I screwed up.

Ah, I was reading the README, which is now inaccurate. Maybe there should be a separate annex which describes which parts of the README actually apply to our import.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 660318 [details] [diff] [review]
Patch 2: nICEr; ICE stack; This is an import with a small number of changes that we plan to upstream.

Per mcmanus, reassign external import of nICEr ICE library to biesi

Comment 16

[Patrick McManus [:mcmanus]]

(In reply to Randell Jesup [:jesup] from comment #15)

> Per mcmanus, reassign external import of nICEr ICE library to biesi

Christian I'm just asking if you want it rubber stamped or read line by line.. if its the latter you can reassign it back to my queue.

Comment 17

[Eric Rescorla (:ekr)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> (In reply to Eric Rescorla from comment #13)
> > > Why is there a version of queue.h for each platform? They look the same.
> > 
> > They are. It's an artifact of our portability system. Originally this code
> > ran on bsd which didn't need it, so this is how it gets filled in for
> > other platforms.
> 
> Would it be too much to ask to avoid that duplication?

No, I can probably fix it.

 
> > I have tried to remove things which are not needed. For instance registryd
> > is not
> > included, unless I screwed up.
> 
> Ah, I was reading the README, which is now inaccurate. Maybe there should be
> a separate annex which describes which parts of the README actually apply to
> our import.

I could write something here.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 660318 [details] [diff] [review]
Patch 2: nICEr; ICE stack; This is an import with a small number of changes that we plan to upstream.

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

Please, please, add a README file describing where this library is coming from. With a name like that this is literally impossible to google for (I failed). Also, please add a .diff file for the changes you made relative to upstream; as it is, it's impossible to tell (especially since it's unclear which version you imported).

::: media/mtransport/third_party/nICEr/nicer.gyp
@@ +209,5 @@
> +          ]
> +      }]
> +}
> +
> +# gcc -g    -c -o ice_candidate.d /Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../ice/ice_candidate.c -MM -MG -DUSE_TURN -DDARWIN -DHAVE_SIN_LEN  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../ice/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../net/ -DUSE_ICE -DUSE_RFC_3489_BACKWARDS_COMPATIBLE -DUSE_STUND_0_96 -DUSE_STUN_PEDANTIC -DUSE_TURN -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../stun/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../util/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../crypto/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../ice/test/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../net/test/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../stun/test/ -g -Werror -Wall -Wno-parentheses -DHAVE_STRDUP -D__UNUSED__="__attribute__((unused))" -Drestrict=__restrict__ -I../../../../nrappkit/src/make/darwin -I../../../../nrappkit//src/util -I../../../../nrappkit//src/util/libekr -I../../../../nrappkit//src/port/darwin/include -I../../../../nrappkit//src/share -I../../../../nrappkit//src/registry -I../../../../nrappkit//src/stats -DOPENSSL -I../../../../openssl-0.9.8g/include -DSANITY_CHECKS

What's up with this comment and the ones below?

To clarify, this gyp file is written by you, it doesn't come from upstream?

Comment 19

[Eric Rescorla (:ekr)]

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #18)
> Comment on attachment 660318 [details] [diff] [review]
> Patch 2: nICEr; ICE stack; This is an import with a small number of changes
> that we plan to upstream.
> 
> Review of attachment 660318 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please, please, add a README file describing where this library is coming
> from. With a name like that this is literally impossible to google for (I
> failed). 

I can do that.


> Also, please add a .diff file for the changes you made relative to
> upstream; as it is, it's impossible to tell (especially since it's unclear
> which version you imported).

My plan was to push the changes upstream. Is a .diff really helpful
in that case?


> ::: media/mtransport/third_party/nICEr/nicer.gyp
> @@ +209,5 @@
> > +          ]
> > +      }]
> > +}
> > +
> > +# gcc -g    -c -o ice_candidate.d /Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../ice/ice_candidate.c -MM -MG -DUSE_TURN -DDARWIN -DHAVE_SIN_LEN  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../ice/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../net/ -DUSE_ICE -DUSE_RFC_3489_BACKWARDS_COMPATIBLE -DUSE_STUND_0_96 -DUSE_STUN_PEDANTIC -DUSE_TURN -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../stun/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../util/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../crypto/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../ice/test/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../net/test/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../stun/test/ -g -Werror -Wall -Wno-parentheses -DHAVE_STRDUP -D__UNUSED__="__attribute__((unused))" -Drestrict=__restrict__ -I../../../../nrappkit/src/make/darwin -I../../../../nrappkit//src/util -I../../../../nrappkit//src/util/libekr -I../../../../nrappkit//src/port/darwin/include -I../../../../nrappkit//src/share -I../../../../nrappkit//src/registry -I../../../../nrappkit//src/stats -DOPENSSL -I../../../../openssl-0.9.8g/include -DSANITY_CHECKS
> 
> What's up with this comment and the ones below?

They are placeholders from my conversion from make -> gyp. I started by taking a build
line + all the sources and gradually moving them into gyp as needed. I guess I didn't clean
up. I can do that.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Eric Rescorla from comment #19)
> (In reply to Christian :Biesinger (don't email me, ping me on IRC) from
> > Also, please add a .diff file for the changes you made relative to
> > upstream; as it is, it's impossible to tell (especially since it's unclear
> > which version you imported).
> 
> My plan was to push the changes upstream. Is a .diff really helpful
> in that case?

Yes, in that mods intended for upstreaming are by definition new code, not imported code, and so brook more review.  (Admittedly on an import you're trusting the import source to be stable; that's kinda on the importer to vet the import point correctly such that line-by-line review isn't warranted.)

I had my mods for usrsctp as a separate patch (which was reviewed and has now been upstreamed and is gone).

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

If the changes been accepted upstream, I'm fine with not having a diff. Have they?

Comment 22

[Eric Rescorla (:ekr)]

I'm one of the upstream maintainers. My plan was to rereview them and then upstream them.

That said, I certainly don't have a problem with delivering a patch for separate review for hte
reasons Jesup indicated.

Comment 23

[Eric Rescorla (:ekr)]

Attachment: Patch 1: Import nrappkit; generic application development kit/portable runtime; Dependency for nICEr. This is an import with a small number of changes that will be upstreamed soon.

Comment 24

[Eric Rescorla (:ekr)]

Attachment: Patch 3: build nICEr and nrappkit

Comment 25

[Eric Rescorla (:ekr)]

(In reply to Eric Rescorla from comment #17)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> > (In reply to Eric Rescorla from comment #13)
> > > > Why is there a version of queue.h for each platform? They look the same.
> > > 
> > > They are. It's an artifact of our portability system. Originally this code
> > > ran on bsd which didn't need it, so this is how it gets filled in for
> > > other platforms.
> > 
> > Would it be too much to ask to avoid that duplication?
> 
> No, I can probably fix it.
> 
>  
> > > I have tried to remove things which are not needed. For instance registryd
> > > is not
> > > included, unless I screwed up.
> > 
> > Ah, I was reading the README, which is now inaccurate. Maybe there should be
> > a separate annex which describes which parts of the README actually apply to
> > our import.
> 
> I could write something here.

See revised patch.

Comment 26

[Eric Rescorla (:ekr)]

Attachment: Patch 2: nICEr; ICE stack; This is an import with a small number of changes that we plan to upstream.

Comment 27

[Eric Rescorla (:ekr)]

Attachment: Changes to nICEr to be upstreamed

Comment 28

[Eric Rescorla (:ekr)]

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #21)
> If the changes been accepted upstream, I'm fine with not having a diff. Have
> they?

I have uploaded a diff in https://bugzilla.mozilla.org/attachment.cgi?id=661654&action=edit if you
want to review it. Since we will be upstreaming these changes, I would rather not import the diff into m-c.

Comment 29

[(not currently active) Ted Mielczarek]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

::: media/mtransport/README
@@ +1,1 @@
> +Generic transport support

This is so unhelpful you might as well not include it.

::: media/mtransport/build/Makefile.in
@@ +34,5 @@
> +#
> +# ***** END LICENSE BLOCK *****
> +
> +
> +DEPTH = ../../..

DEPTH = @DEPTH@, here and anywhere else.

@@ +73,5 @@
> +  $(NULL)
> +
> +CPPSRCS = \
> +	$(MTRANSPORT_LCPPSRCS) \
> +	$(NULL)

No tabs, two-space indent.

@@ +78,5 @@
> +
> +include $(srcdir)/../objs.mk
> +
> +export:: $(MTRANSPORT_CPPSRCS)
> +	$(INSTALL) $^ .

What is up with this? We shouldn't have to do this. I'm giving this patch r- for this.

::: media/mtransport/objs.mk
@@ +45,5 @@
> +  transportlayerloopback.cpp \
> +  transportlayerprsock.cpp \
> +  $(NULL)
> +
> +MTRANSPORT_CPPSRCS = $(addprefix $(topsrcdir)/media/mtransport/, $(MTRANSPORT_LCPPSRCS))
\ No newline at end of file

You could ostensibly just wrap the whole list in $(addprefix) and drop this extra assignment.

::: media/mtransport/standalone/Makefile.in
@@ +34,5 @@
> +#
> +# ***** END LICENSE BLOCK *****
> +
> +
> +DEPTH = ../../..

Same complaints as the other makefile.

::: media/mtransport/test/Makefile.in
@@ +34,5 @@
> +#
> +# ***** END LICENSE BLOCK *****
> +
> +
> +DEPTH = ../../../

Here too.

@@ +55,5 @@
> +		  $(DEPTH)/media/mtransport/third_party/nICEr/nicer_nicer/$(LIB_PREFIX)nicer.$(LIB_SUFFIX) \
> +		  $(DEPTH)/media/mtransport/third_party/nrappkit/nrappkit_nrappkit/$(LIB_PREFIX)nrappkit.$(LIB_SUFFIX) \
> +		  $(DEPTH)/media/webrtc/trunk/testing/gtest_gtest/$(LIB_PREFIX)gtest.$(LIB_SUFFIX) \
> +		  $(DEPTH)/netwerk/sctp/src/$(LIB_PREFIX)nksctp_s.$(LIB_SUFFIX) \
> +		  $(NULL)

No tabs, two-space indent.

@@ +130,5 @@
> +   -DCALLBACK_API=1 \
> +   -DSCTP_DEBUG=1 \
> +   $(NULL)
> +
> +# TODO: need to expand for other platforms

Sounds like we can take this TODO out since it works for our Tier-1 platforms.

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 661654 [details] [diff] [review]
Changes to nICEr to be upstreamed

ekr said in the comment you might want to review the upstream patch (patch to be upstreamed) against nICEr per previous comments, but he didn't mark it for review.  Marking for review, if you want to.

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 661653 [details] [diff] [review]
Patch 2: nICEr; ICE stack; This is an import with a small number of changes that we plan to upstream.

marking updated library import for import (re)review

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 661650 [details] [diff] [review]
Patch 3: build nICEr and nrappkit

Patch was updated but not marked for review; marking

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 661649 [details] [diff] [review]
Patch 1: Import nrappkit; generic application development kit/portable runtime; Dependency for nICEr. This is an import with a small number of changes that will be upstreamed soon.

Patch was updated, but wasn't re-marked for review. Marking.  Does it look good now, roc?

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 661649 [details] [diff] [review]
Patch 1: Import nrappkit; generic application development kit/portable runtime; Dependency for nICEr. This is an import with a small number of changes that will be upstreamed soon.

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

::: media/mtransport/third_party/nrappkit/IMPORT_FILES
@@ +68,5 @@
> +              src/port/win32/include/csi_platform.h
> +              src/port/win32/include/sys/queue.h
> +              src/port/linux/include/csi_platform.h
> +              src/port/linux/include/linux_funcs.h
> +              src/port/linux/include/sys/queue.h

This is no longer accurate?

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 661654 [details] [diff] [review]
Changes to nICEr to be upstreamed

I'm fine with not reviewing this one myself

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 661653 [details] [diff] [review]
Patch 2: nICEr; ICE stack; This is an import with a small number of changes that we plan to upstream.

+# gcc -g    -c -o ice_candidate.d /Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../ice/ice_candidate.c -MM -MG -DUSE_TURN -DDARWIN -DHAVE_SIN_LEN  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../ice/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../net/ -DUSE_ICE -DUSE_RFC_3489_BACKWARDS_COMPATIBLE -DUSE_STUND_0_96 -DUSE_STUN_PEDANTIC -DUSE_TURN -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../stun/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../util/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../crypto/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../ice/test/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../net/test/  -I/Users/ekr/dev/alder/media/mtransport/third_party/nICEr/src/make/darwin/../../stun/test/ -g -Werror -Wall -Wno-parentheses -DHAVE_STRDUP -D__UNUSED__="__attribute__((unused))" -Drestrict=__restrict__ -I../../../../nrappkit/src/make/darwin -I../../../../nrappkit//src/util -I../../../../nrappkit//src/util/libekr -I../../../../nrappkit//src/port/darwin/include -I../../../../nrappkit//src/share -I../../../../nrappkit//src/registry -I../../../../nrappkit//src/stats -DOPENSSL -I../../../../openssl-0.9.8g/include -DSANITY_CHECKS
+
+
+# "./src/ice/test/ice_test.c",

I thought you were going to remove that

+SVN revision 9873

so is it identical to that revision, or does it have a few changes? Sorry to be a pain but I really think that for anyone who may in the future try to match up what's in our tree with upstream, it would be much easier if what we have is either 100% identical or we provide a .diff in our tree to make it so.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, we usually store in our tree copies of the patches between upstream and the copy in our tree.

Comment 38

[Eric Rescorla (:ekr)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> Yeah, we usually store in our tree copies of the patches between upstream
> and the copy in our tree.

OK. I will add patches and re-submit for review.

Comment 39

[Eric Rescorla (:ekr)]

Attachment: Patch 2: nICEr; ICE stack; This is an import with a small number of changes that we plan to upstream.

Comment 40

[Eric Rescorla (:ekr)]

Attachment: Patch 1: Import nrappkit; generic application development kit/portable runtime; Dependency for nICEr. This is an import with a small number of changes that will be upstreamed soon.

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 663377 [details] [diff] [review]
Patch 2: nICEr; ICE stack; This is an import with a small number of changes that we plan to upstream.

+Only in /Users/ekr/dev/mtransport-import-references/nICEr/src/: test
+Only in /Users/ekr/dev/mtransport-import-references/nICEr/src/: testua

I'd probably remove these "Only in" lines from the .diff, but either way, r=biesi

Comment 42

[(not currently active) Ted Mielczarek]

Comment on attachment 661650 [details] [diff] [review]
Patch 3: build nICEr and nrappkit

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

::: media/mtransport/third_party/Makefile.in
@@ +18,5 @@
> +# the Initial Developer. All Rights Reserved.
> +#
> +# Contributor(s):
> +#  Chris Pearce <[e-mail]>
> +#  Eric Rescorla <[e-mail]>	

nit: trailing whitespace

@@ +35,5 @@
> +#
> +# ***** END LICENSE BLOCK *****
> +
> +
> +DEPTH = ../../..

DEPTH = @DEPTH@

@@ +45,5 @@
> +
> +DIRS = \
> +  nrappkit \
> +  nICEr \
> +  $(NULL)

Do nrappkit and nICEr link anything? If not you can probably use PARALLEL_DIRS here.

::: media/mtransport/third_party/import.py
@@ +18,5 @@
> +DISTRO = sys.argv[1]
> +IMPORT_DIR = sys.argv[2]
> +FILES = []
> +
> +f = open("%s/IMPORT_FILES"%IMPORT_DIR)

Whitespace around the % operator would make this file a little easier to read.

@@ +22,5 @@
> +f = open("%s/IMPORT_FILES"%IMPORT_DIR)
> +for l in f:
> +    l = l.strip()
> +    l = l.strip("'")
> +    m = re.match('^#', l)

You could just say if l.startswith("#"): continue

@@ +25,5 @@
> +    l = l.strip("'")
> +    m = re.match('^#', l)
> +    if m is not None:
> +        continue
> +    if l == "":

if not l:

@@ +43,5 @@
> +    
> +
> +
> +            
> +            

There's a lot of trailing whitespace in this file.

Comment 43

[Eric Rescorla (:ekr)]

Attachment: Patch 3: build nICEr and nrappkit r=ted

Comment 44

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

Here's the review of:
[+]media/mtransport/dtlsidentity.cpp (new file) Reviewed
[+]media/mtransport/dtlsidentity.h (new file) Reviewed
[+]media/mtransport/logging.h (new file) Reviewed

Still working on transportlayerdtls.cpp and transportlayerdtls.h

::: media/mtransport/dtlsidentity.cpp
@@ +17,5 @@
> +// Helper class to avoid having a crapload of if (!NULL) statements at
> +// the end to clean up. The way you use this is you instantiate the
> +// object as scoped_c_ptr<PtrType> obj(value, destructor);
> +// TODO([e-mail]): Move this to some generic location
> +template <class T> class scoped_c_ptr {

Once bug 767241 is reviewed, you can use the types in bug 767241 so that this class is not necessary any more. That can be done in a follow-up.

Personally, I would prefer this code to be switched from this Googly style to Mozilla's style (mMember vs member_ and camelCase vs horrible_underscores, return type of a function on a separate line, ns[A][C][Auto]String instead of std::string, "using namespace mozilla"). I'm not a huge fan of the Mozilla style personally, but I think that it is important to be at least consistent with ourselves. 

Even putting style aside, it is difficult to reason about the edge-case behavior of std::string and other STL containers given that we generally never use them and also that we have non-standard error handling (e.g. no exceptions). I am not going to mention this further during this review because there are more important issues to address first, and the landing shouldn't block on it, but it should be done in a follow-up.

Ensure that all lines wrap ~80 characters so that future side-by-side diffs will be easier to review in the future.

Also, I won't point out issues like trailing whitespace that Splinter points out on its own.

@@ +42,5 @@
> +  void (*d_)(T *);
> +};
> +
> +// Auto-generate an identity based on name=name
> +mozilla::RefPtr<DtlsIdentity> DtlsIdentity::Generate(const std::string name) {

At this point, how do you know that NSS has been initialized and that it hasn't been shut down?

It seems like, at a minimum, you must implement the nsNSSShutdownObject protocol wherever you use NSS.

Why do we have to pass the name as an input to this function? Couldn't we just generate a random name? Because the certificate is sent in the clear, the name is sent in the clear. So, it is better to design the API so that the application cannot choose the name, so that we don't have to worry about the application unintentionally leaking any sensitive information (e.g. PII) through the name. If the application needs access to the name, it could get it through a GetName() method on this class. Also, see my comment on collisions below.

The convention for functions that return already-addrefed values is to return already_AddRefed<T>.

Add a comment that says that this function generates a self-signed X.509v3 certificate and that it is based this code on NSS's certutil.c.

@@ +46,5 @@
> +mozilla::RefPtr<DtlsIdentity> DtlsIdentity::Generate(const std::string name) {
> +  SECStatus rv;
> +
> +  std::string subject_name_string = "CN=" + name;
> +  CERTName *subject_name = CERT_AsciiToName(

Needs to use a smart pointer to avoid leaking.

@@ +54,5 @@
> +  }
> +
> +  PK11RSAGenParams rsaparams;
> +  rsaparams.keySizeInBits = 1024;
> +  rsaparams.pe = 0x010001;

... = 65537; // We are too paranoid to use 3 as the exponent.

@@ +60,5 @@
> +  scoped_c_ptr<SECKEYPrivateKey> private_key(SECKEY_DestroyPrivateKey);
> +  scoped_c_ptr<SECKEYPublicKey> public_key(SECKEY_DestroyPublicKey);
> +  SECKEYPublicKey *pubkey;
> +
> +  private_key = PK11_GenerateKeyPair(PK11_GetInternalSlot(),

Check that PK11_GetInternalSlot() does not fail.

@@ +79,5 @@
> +  if (!certreq.get()) {
> +    return NULL;
> +  }
> +
> +  PRTime notBefore = PR_Now() - (86400UL * PR_USEC_PER_SEC);

Please make 86400UL clearer, e.g. (60 * 60 * 24 * whatever), and add a comment explaining why you chose the range -X to +30X as opposed to some other range.

@@ +89,5 @@
> +    return NULL;
> +  }
> +
> +  unsigned long serial;
> +  // Note: This serial in principle could collide, but it's unlikely

Is it unlikely enough? NSS simply will not allow you to create a CERTCertificate object that has the same (issuer, serial) pair as another CERTCertificate object. So, the probability of collision is tied to the probability that this identity will be useless and cause connections that use it to fail.

For that reason, we should add a random component to the issuer name that further reduces the likelihood of collisions (e.g. 128 bits).

@@ +110,5 @@
> +  if (rv != SECSuccess)
> +    return NULL;
> +
> +  // Set version to X509v3.
> +  *(certificate.get()->version.data) = 2;

= SEC_CERTIFICATE_VERSION_3;

@@ +113,5 @@
> +  // Set version to X509v3.
> +  *(certificate.get()->version.data) = 2;
> +  certificate.get()->version.len = 1;
> +
> +  SECItem innerDER;

This leaks if later function calls fail.

@@ +119,5 @@
> +  innerDER.data = NULL;
> +
> +  if (!SEC_ASN1EncodeItem(arena, &innerDER, certificate.get(),
> +                          SEC_ASN1_GET(CERT_CertificateTemplate)))
> +    return NULL;

Use braces at lesat for statements with multi-line conditions.

@@ +121,5 @@
> +  if (!SEC_ASN1EncodeItem(arena, &innerDER, certificate.get(),
> +                          SEC_ASN1_GET(CERT_CertificateTemplate)))
> +    return NULL;
> +
> +  SECItem *signedCert 

Leaks if SEC_DerSignData fails.

@@ +141,5 @@
> +}
> +
> +
> +
> +DtlsIdentity::~DtlsIdentity() {

Use smart pointer members instead of explicit destructor unless implementing the nsNSSShutDownObject protocol makes smart pointers unhelpful.

@@ +153,5 @@
> +nsresult DtlsIdentity::ComputeFingerprint(const std::string algorithm,
> +                                          unsigned char *digest,
> +                                          std::size_t size,
> +                                          std::size_t *digest_length) {
> +  PR_ASSERT(cert_);

if you implement the nsNSSShutdownObject protocol then this assertion will not be valid, I think.

@@ +170,5 @@
> +
> +  if (algorithm == "sha-1") {
> +    ht = HASH_AlgSHA1;
> +  } else if (algorithm == "sha-224") {
> +    ht = HASH_AlgSHA224;

It doesn't make sense to support SHA-1 and SHA-224 for these protocols, AFAICT. Let's remove this support now, or file a follow-up bug to remove it. And, let's ensure we're always sending SHA2 hashes.

@@ +186,5 @@
> +  PR_ASSERT(ho);
> +  if (!ho)
> +    return NS_ERROR_INVALID_ARG;
> +
> +  PR_ASSERT(ho->length >= 20);  // Double check

This is not necessary.

@@ +195,5 @@
> +  SECStatus rv = HASH_HashBuf(ho->type, digest,
> +                              cert->derCert.data,
> +                              cert->derCert.len);
> +
> +  // This should not happen

the blank line and this comment are not necessary.

@@ +219,5 @@
> +    }
> +    str += group;
> +  }
> +
> +  PR_ASSERT(str.size() == (size * 3 - 1));  // Check result length

s/PR_ASSERT/MOZ_ASSERT everywhere.

@@ +226,5 @@
> +
> +// Parse a fingerprint in RFC 4572 format.
> +// Note that this tolerates some badly formatted data, in particular:
> +// (a) arbitrary runs of colons 
> +// (b) colons at the beginning or end.

I don't think we should support badly-formatted fingerprints given that we're implementing a brand-new thing. And, especially, this code seems to silently do confusing things for inputs like "A:F:0". I would not expect that to get parsed to { 0xAF } like it appears to with this code; I'd expect it to get parsed to { 0xA, 0xF, 0x0 }.

@@ +228,5 @@
> +// Note that this tolerates some badly formatted data, in particular:
> +// (a) arbitrary runs of colons 
> +// (b) colons at the beginning or end.
> +nsresult DtlsIdentity::ParseFingerprint(const std::string fp,
> +                                        unsigned char *digest,

Why not replace digest/size/length with "string & digest /*out*/" ?

@@ +233,5 @@
> +                                        size_t size,
> +                                        size_t *length) {
> +  size_t offset = 0;
> +  bool top_half = true;
> +  unsigned char val = 0;

use uint8_t for val since it isn't storing text;

@@ +246,5 @@
> +    if (top_half && (fp[i] == ':')) {
> +      continue;
> +    } else if ((fp[i] >= '0') && (fp[i] <= '9')) {
> +      val |= fp[i] - '0';
> +    } else if ((fp[i] >= 'a') && (fp[i] <= 'f')) {

lowercase should be considered invalid based on my understanding of http://tools.ietf.org/html/rfc4572#section-5.

::: media/mtransport/dtlsidentity.h
@@ +46,5 @@
> +
> +#include "mozilla/RefPtr.h"
> +#include "nsISupportsImpl.h"
> +
> +class DtlsIdentity : public mozilla::RefCounted<DtlsIdentity> {

It is good enough for this to be refcounted, or does the refcounting need to be thread-safe too?

Need to disallow copying and assignment, right?

@@ +80,5 @@
> +  SECKEYPrivateKey *privkey_;
> +  CERTCertificate *cert_;
> +};
> +
> +

Too many blank lines.

::: media/mtransport/logging.h
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: [e-mail]

I heard we're not supposed to put these credits in any more, as part of the MPL 2.0 stuff. Check with Gerv.

Also, media/mtransport is not a good home for this, IMO. MFBT or XPCOM would be better places. (Can be moved in a follow-up.)

@@ +13,5 @@
> +
> +
> +class LogCtx {
> + public:
> +  LogCtx(const char* name) : module_(PR_NewLogModule(name)) {}

The pattern you are using will cause unnecessary static initializers to run at startup, AFAICT.

It would be better to call PR_NewLogModule lazily at the time of logging, to avoid the static initializer.

@@ +22,5 @@
> +private:
> +  PRLogModuleInfo* module_;
> +};
> +
> +

too many blank lines.

@@ +23,5 @@
> +  PRLogModuleInfo* module_;
> +};
> +
> +
> +#define MLOG_INIT(n) \

Assuming you address my comment about initializing the log module lazily, "MLOG_INIT" is a bad name for this. Maybe just MLOG_MODULE?

The prefix for Mozilla-defined macros is MOZ_, not M.

@@ +28,5 @@
> +  static LogCtx mlog_ctx(n)
> +
> +#define MLOG(level, b) \
> +  do { if (mlog_ctx.module()) {                            \
> +          std::stringstream str;                           \

I am concerned about bringing the iostream library into Gecko, just for the sake of logging. Are we sure that this isn't adding overhead that we didn't have before?

::: media/mtransport/nr_socket_prsock.cpp
@@ +74,5 @@
> +   ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> +   POSSIBILITY OF SUCH DAMAGE.
> +   
> +
> +   [e-mail]  Thu Dec 20 20:14:49 2001

Somebody else needs to check the licensing issues here.

::: media/mtransport/transportlayerdtls.cpp
@@ +5,5 @@
> +// Original author: [e-mail]
> +
> +#include <queue>
> +
> +

too many blank lines.

@@ +45,5 @@
> +// because this is only useful for NSPR.
> +struct Packet {
> +  Packet() : data_(NULL), len_(0), offset_(0) {}
> +  ~Packet() {
> +    delete data_;

This should be delete []. But, really, we should just use ScopedDeleteArray.

@@ +53,5 @@
> +    memcpy(data_, data, len);
> +    len_ = len;
> +  }
> +
> +  unsigned char *data_;

ScopedDeleteArray<uint8_t>

@@ +59,5 @@
> +  PRInt32 offset_;
> +};
> +
> +void NSPRHelper::PacketReceived(const void *data, PRInt32 len) {
> +  input_.push(new Packet());

input_.push(new Packet(data, len));

Again, I am unsure what std::vector/queue will do on OOM.

@@ +117,5 @@
> +}
> +
> +static PRInt32 TransportLayerAvailable(PRFileDesc *f) {
> +  UNIMPLEMENTED;
> +  return -1;

In these methods, we must always call PR_SetError before returning an error value (-1 or PR_FAILURE or whatever).

@@ +204,5 @@
> +// This function does not support peek.
> +static PRInt32 TransportLayerSend(PRFileDesc *f, const void *buf, PRInt32 amount,
> +                   PRIntn flags, PRIntervalTime to) {
> +  PRInt32 written = TransportLayerWrite(f, buf, amount);
> +

remove blank line.

@@ +242,5 @@
> +
> +static PRStatus TransportLayerGetpeername(PRFileDesc *f, PRNetAddr *addr) {
> +  // TODO([e-mail]): Modify to return unique names for each channel
> +  // somehow, as opposed to always the same static address. The current
> +  // implementation messes up the session cache, which is why it's off

Since we're turning off the session cache anyway, can we just make this UNIMPLEMENTED like the other things?

@@ +340,5 @@
> +  TransportLayerReserved,
> +  TransportLayerReserved
> +};
> +
> +TransportLayerDtls::~TransportLayerDtls() {

Double-check everything is getting cleaned up.

@@ +354,5 @@
> +  // Get the transport service as an event target
> +  nsresult rv;
> +  target_ = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
> +
> +  if (!NS_SUCCEEDED(rv)) {

Don't use !NS_SUCCEEDED(rv); use NS_FAILED(rv) instead.

@@ +360,5 @@
> +    return rv;
> +  }
> +
> +  timer_ = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
> +  if (!NS_SUCCEEDED(rv)) {

ditto.

@@ +393,5 @@
> +                                          const unsigned char *digest_value,
> +                                          size_t digest_len) {
> +  // Defensive programming
> +  if (verification_mode_ != VERIFY_UNSET &&
> +    verification_mode_ != VERIFY_DIGEST)

indent two more spaces, and use braces around the body.

@@ +411,5 @@
> +  return NS_OK;
> +}
> +
> +// TODO([e-mail]): make sure this is called from STS. Otherwise
> +// we have thread safety issues

See how this is checked in SSLServerCertVerification.cpp.

@@ +817,5 @@
> +  CERTCertificate *peer_cert = NULL;
> +  peer_cert = SSL_PeerCertificate(fd);
> +
> +  // We are not set up to take this being called multiple
> +  // times. Change this if we ever add renegotiation.

We should be calling the libssl function to disable renegotiation.

::: media/mtransport/transportlayerdtls.h
@@ +26,5 @@
> +#include "transportlayer.h"
> +
> +struct Packet;
> +
> +class NSPRHelper {

Please add a comment explaining how this class "helps".

@@ +32,5 @@
> +  NSPRHelper(TransportLayer *output) :
> +      output_(output),
> +      input_() {}
> +
> +  void PacketReceived(const void *data, PRInt32 len);

PR* (including especially PRBool) is out; now use int32_t, bool, and friends (without qualifying them with std::).

@@ +40,5 @@
> + private:
> +  DISALLOW_COPY_ASSIGN(NSPRHelper);
> +
> +  TransportLayer *output_;
> +  std::queue<Packet *> input_;

make sure std::queue handles OOM correctly.

@@ +43,5 @@
> +  TransportLayer *output_;
> +  std::queue<Packet *> input_;
> +};
> +
> +class TransportLayerDtls : public TransportLayer {

it isn't clear how this works with Gecko's NSS lifecycle; how do we ensure NSS has been initialized and that it hasn't been shut down? Do we need to implement the nsNSSShutdownObject protocol here?

@@ +47,5 @@
> +class TransportLayerDtls : public TransportLayer {
> +public:
> + TransportLayerDtls() :
> +     TransportLayer(DGRAM),
> +     identity_(NULL),

s/NULL/nullptr/g.

@@ +62,5 @@
> +
> +
> +  enum Role { CLIENT, SERVER};
> +  enum Verification { VERIFY_UNSET, VERIFY_ALLOW_ALL, VERIFY_DIGEST};
> +  const static int kMaxDigestLength = 64;

It's better to just use nsAutoCString and avoid the issue altogether. This isn't a performance- or space- critical buffer.

@@ +68,5 @@
> +  // DTLS-specific operations
> +  void SetRole(Role role) { role_ = role;}
> +  Role role() { return role_; }
> +
> +  void SetIdentity(mozilla::RefPtr<DtlsIdentity> identity) {

already_AddRefed<DtlsIdentity> identity.

@@ +73,5 @@
> +    identity_ = identity;
> +  }
> +  nsresult SetVerificationAllowAll();
> +  nsresult SetVerificationDigest(const std::string digest_algorithm,
> +                                 const unsigned char *digest_value,

Again, there's no need to use raw pointers here, AFAICT. ns[Dependent|Auto]CString can efficiently handle all of this without the tedious pointer manipulations, if we just replace "digest_value" and "digest_len" with:

    nsACString & digest /*out*/

@@ +82,5 @@
> +
> +  nsresult ExportKeyingMaterial(const std::string& label,
> +                                bool use_context,
> +                                const std::string& context,
> +                                unsigned char *out,

Same here. nsACString & out and then we can forget about buffer overflows.

@@ +85,5 @@
> +                                const std::string& context,
> +                                unsigned char *out,
> +                                unsigned int outlen);
> +
> +  const CERTCertificate *GetPeerCert() const { return peer_cert_; }

Usually a function that returns a CERTCertificate* will call CERT_DupCertificate() to give the caller a copy (really just increase the refcount). Since that's the existing convention, and since CERT_DupCertificate is cheap, I think we should just use that existing convention here and in dtlsidentity.

@@ +88,5 @@
> +
> +  const CERTCertificate *GetPeerCert() const { return peer_cert_; }
> +
> +  // Transport layer overrides.
> +  virtual nsresult InitInternal();

Use MOZ_OVERRIDE for the methods declared in the superclasses.

@@ +106,5 @@
> +  // A single digest to check
> +  class VerificationDigest {
> +   public:
> +    VerificationDigest(std::string algorithm,
> +                       const unsigned char *value, size_t len) {

This can also be simplified by using the string API.

@@ +151,5 @@
> +
> +  Role role_;
> +  Verification verification_mode_;
> +  std::vector<mozilla::RefPtr<VerificationDigest> > digests_;
> +

Need to make sure all these C-typed objects are cleaned up in the destructor.

@@ +153,5 @@
> +  Verification verification_mode_;
> +  std::vector<mozilla::RefPtr<VerificationDigest> > digests_;
> +
> +  PRFileDesc *pr_fd_;
> +  PRFileDesc *ssl_fd_;

ScopedPRFileDesc?

@@ +155,5 @@
> +
> +  PRFileDesc *pr_fd_;
> +  PRFileDesc *ssl_fd_;
> +  mozilla::ScopedDeletePtr<NSPRHelper> helper_;
> +  CERTCertificate *peer_cert_;

ScpopedCERTCertificate?

Comment 45

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

When you post this for the next round of reviews, please split this patch into the parts that you want me to review, and the parts that you want mcmanus to review, to make it easier to track who is supposed to review what. Go ahead and merge the reviewed patches back together before checking them in.

Now I've added transportlayerdtls.cpp + transportlayerdtls.h to my review.

Let me know if there are other parts of this patch (or other patches) I am supposed to review.

::: media/mtransport/dtlsidentity.cpp
@@ +13,5 @@
> +#include "logging.h"
> +
> +MLOG_INIT("mtransport");
> +
> +// Helper class to avoid having a crapload of if (!NULL) statements at

Let's avoid "crapload".

::: media/mtransport/transportlayer.cpp
@@ +45,5 @@
> +  }
> +}
> +
> +const std::string& TransportLayer::flow_id() { 
> +    static std::string empty;

static const std::string empty;

::: media/mtransport/transportlayer.h
@@ +31,5 @@
> +  virtual const std::string id() { return name; } \
> +  static std::string ID() { return name; }
> +
> +// Abstract base class for network transport layers.
> +class TransportLayer : public sigslot::has_slots<> {

Document why you created a new I/O layering API instead of using the NSPR I/O layering mechanism.

@@ +51,5 @@
> +  // Called to initialize
> +  nsresult Init();  // Called by Insert() to set up -- do not override
> +  virtual nsresult InitInternal() { return NS_OK; } // Called by Init
> +
> +  // Inserted

Please take some time to provide better documentation for the methods of this class. Ideally, I should be able to review the TransportLayer <-> NSPR I/O layer stuff and the way it interacts with the TransportLayer based on the documentation in this class.

::: media/mtransport/transportlayerdtls.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: [e-mail]
> +

Name this file TransportLayerIOLayer, to match the SOCKS and PSM layers.

@@ +5,5 @@
> +// Original author: [e-mail]
> +
> +#include <queue>
> +
> +

Document how this is layer is supposed to be used. It is confusing that it implements an NSPR IO layer, but it doesn't really get used the same way that normal I/O layers get used. I would expect that a DTLS I/O layer would be implemented on top of a libssl I/O layer that is layered on top of a UDP I/O layer, using the normal NSPR layer mechanism. Why wasn't it done this way? That is very useful information to the reader/maintainer of this code.

I believe part of the answer is that TransportLayer is a "push" mechanism, whereas NSPR IO layers use a "pull" model, so this layer translates the "push" semantics into "pull" semantics. And, The reason TransportLayer uses "push" semantics is to avoid inappropriate buffering for other kinds of uses (where this TransportLayerIOLayer isn't used). This adapter adds buffering that is necessary to support the pull semantics.

Explain why buffering is OK here, but not elsewhere.

@@ +59,5 @@
> +  PRInt32 offset_;
> +};
> +
> +void NSPRHelper::PacketReceived(const void *data, PRInt32 len) {
> +  input_.push(new Packet());

What happens if we receive a lot of input that we don't process? Will be just consume all the memory until we die? I think we need to impose some reasonable cap on how much data can be buffered, and have some better strategy for dealing with too much data other than "crash." Can be done in a follow-up bug if you file the bug now.

Is PacketReceived() always called on the same thread as the other methods? (Similar questions apply to the other methods, especially Write()). Document the threading model for NSPRHelper above its class definition.

@@ +189,5 @@
> +// Note: this is always nonblocking and assumes a zero timeout.
> +// This function does not support peek.
> +static PRInt32 TransportLayerRecv(PRFileDesc *f, void *buf, PRInt32 amount,
> +                   PRIntn flags, PRIntervalTime to) {
> +  PR_ASSERT(flags == 0);

MOZ_ASSERT(flags == 0);
MOZ_ASSERT(to == 0);

In general, use MOZ_ASSERT instead of PR_ASSERT.

@@ +191,5 @@
> +static PRInt32 TransportLayerRecv(PRFileDesc *f, void *buf, PRInt32 amount,
> +                   PRIntn flags, PRIntervalTime to) {
> +  PR_ASSERT(flags == 0);
> +
> +  if (flags != 0) {

if (flags != 0 || to != 0)

@@ +200,5 @@
> +  return TransportLayerRead(f, buf, amount);
> +}
> +
> +// Note: this is always nonblocking and assumes a zero timeout.
> +// This function does not support peek.

PR_Send() doesn't have a peek flag.

@@ +209,5 @@
> +  return written;
> +}
> +
> +static PRInt32 TransportLayerRecvfrom(PRFileDesc *f, void *buf, PRInt32 amount,
> +                       PRIntn flags, PRNetAddr *addr, PRIntervalTime to) {

It seems like every method has its second line of args indented differently.

Use Mozilla style:

// parameters aligned at "("
static PRInt32
TransportLayerRecvfrom(PRFileDesc *f, void *buf, PRInt32 amount,
                       PRIntn flags, PRNetAddr *addr, PRIntervalTime to)
{ // <-- note where this is
}

or:

// When wrapping parameters onto the second line, indent some consistent number of spaces
static PRInt32
TransportLayerRecvfrom(PRFileDesc *f, void *buf, PRInt32 amount,
  PRIntn flags, PRNetAddr *addr, PRIntervalTime to)
{ <-- still on its own line.
}

@@ +242,5 @@
> +
> +static PRStatus TransportLayerGetpeername(PRFileDesc *f, PRNetAddr *addr) {
> +  // TODO([e-mail]): Modify to return unique names for each channel
> +  // somehow, as opposed to always the same static address. The current
> +  // implementation messes up the session cache, which is why it's off

I think that besides the session, libssl might call this method to confirm that it has connected. If so, and that's why we still need to implement this even though the session cache is off, then add a comment to that effect here.

@@ +301,5 @@
> +  UNIMPLEMENTED;
> +  return -1;
> +}
> +
> +static const struct PRIOMethods nss_methods = {

TransportLayerMethods would be a better name, since this doesn't really have anything to do with NSS, and because that is the convention used elsewhere (in PSM and Necko).

@@ +419,5 @@
> +  if (!downward_) {
> +    MLOG(PR_LOG_ERROR, "DTLS layer with nothing below. This is useless");
> +    return false;
> +  }
> +  helper_ = new NSPRHelper(downward_);

rename this to nsprIOLayer or similar.

@@ +487,5 @@
> +  }
> +
> +  if (mode_ != DGRAM) {
> +    // Disable SSLv3
> +    rv = SSL_OptionSet(ssl_fd, SSL_ENABLE_SSL3, PR_FALSE);

Use the new SSL version range API to set the version range to (TLS 1.1, TLS 1.1), instead of using SSL_OptionSet. And, do this for both modes (datagram vs. stream).

There are many other options that should be set, especially the ones about renegotiation and cipher suites. Right now you are inheriting the PSM settings, because you do not set them yourself. But, we should have our own settings for WebRTC--especialy in cases where our PSM settings are weak. For example, WebRTC should require the strict renegotiation extension be supported by the peer.

This code should set SSL_ENABLE_SESSION_TICKETS = false, SSL_NO_CACHE = true, SSL_ENABLE_DEFLATE = false, SSL_ENABLE_RENEGOTIATION = SSL_RENEGOTIATE_NEVER (or SSL_RENEGOTIATE_REQUIRES_XTN), SSL_REQUIRE_SAFE_NEGOTIATION = true, SSL_ENABLE_FALSE_START = false (at least until we can analyze it). Also set SSL_NO_LOCKS according to the threading model you have.

@@ +536,5 @@
> +  downward_->SignalPacketReceived.connect(this, &TransportLayerDtls::PacketReceived);
> +
> +  if (downward_->state() == OPEN) {
> +    Handshake();
> +  }

StateChange(downward_, OPEN);?

@@ +606,5 @@
> +        if (mode_ == DGRAM) {
> +          PRIntervalTime timeout;
> +          rv = DTLS_GetHandshakeTimeout(ssl_fd_, &timeout);
> +          if (rv == SECSuccess) {
> +            PRUint32 timeout_ms = PR_IntervalToMilliseconds(timeout);

reminder to switch NSPR types to C++0x types (without std:: qualification) everywhere.

@@ +679,5 @@
> +  PRInt32 rv = PR_Send(ssl_fd_, data, len, 0, PR_INTERVAL_NO_WAIT);
> +
> +  if (rv > 0) {
> +    // We have data
> +    MLOG(PR_LOG_DEBUG, LAYER_INFO << "Wrote " << rv << " bytes to NSS");

s/NSS/SSL layer/

@@ +710,5 @@
> +
> +  TransportLayerDtls *stream = reinterpret_cast<TransportLayerDtls *>(arg);
> +
> +  if (!stream->identity_) {
> +    MLOG(PR_LOG_ERROR, "No identity available");

You must always call PR_SetError before returning SECFailure.

@@ +721,5 @@
> +  }
> +
> +  *pRetKey = SECKEY_CopyPrivateKey(stream->identity_->privkey());
> +  if (!pRetKey) {
> +    CERT_DestroyCertificate(*pRetCert);

*pRetCert = nullptr;

@@ +723,5 @@
> +  *pRetKey = SECKEY_CopyPrivateKey(stream->identity_->privkey());
> +  if (!pRetKey) {
> +    CERT_DestroyCertificate(*pRetCert);
> +    return SECFailure;
> +  }

newline here.

@@ +743,5 @@
> +  }
> +
> +  return NS_OK;
> +}
> +

Can we encapsulate the context inside of TransportLayerDTLS, instead of making it parameterized and passed from the outside? What do the higher layers pass as the context? Which callers pass use_context == false?

@@ +775,5 @@
> +
> +  return stream->AuthCertificateHook(fd, checksig, isServer);
> +}
> +
> +SECStatus TransportLayerDtls::CheckDigest(mozilla::RefPtr<VerificationDigest> digest,

Why not VerificationDigest * digest? We don't usually pass RefPtrs as parameters.

@@ +790,5 @@
> +                                       &computed_digest_len);
> +  if (!NS_SUCCEEDED(res)) {
> +    MLOG(PR_LOG_ERROR, "Could not compute peer fingerprint for digest " <<
> +         digest->algorithm_);
> +    // Go to end

You must call PR_SetError() before returning SECFailure. (check this everywhere in your code)

@@ +798,5 @@
> +  if (computed_digest_len != digest->len_) {
> +    MLOG(PR_LOG_ERROR, "Digest is wrong length " << digest->len_ <<
> +         " should be " << computed_digest_len << " for algorithm " <<
> +         digest->algorithm_);
> +    return SECFailure;

another case of missing PR_SEtError();

@@ +803,5 @@
> +  }
> +
> +  if (memcmp(digest->value_, computed_digest, computed_digest_len) != 0) {
> +    MLOG(PR_LOG_ERROR, "Digest does not match");
> +    return SECFailure;

ditto.

@@ +814,5 @@
> +SECStatus TransportLayerDtls::AuthCertificateHook(PRFileDesc *fd,
> +                                                  PRBool checksig,
> +                                                  PRBool isServer) {
> +  CERTCertificate *peer_cert = NULL;
> +  peer_cert = SSL_PeerCertificate(fd);

Leaks peer_cert on failure. use ScopedCERTCertificate.

@@ +819,5 @@
> +
> +  // We are not set up to take this being called multiple
> +  // times. Change this if we ever add renegotiation.
> +  PR_ASSERT(!auth_hook_called_);
> +  if (auth_hook_called_)

PR_SetERrror.

@@ +820,5 @@
> +  // We are not set up to take this being called multiple
> +  // times. Change this if we ever add renegotiation.
> +  PR_ASSERT(!auth_hook_called_);
> +  if (auth_hook_called_)
> +    return SECFailure;

Blank line here, or braces around "return SECFailure;"

@@ +829,5 @@
> +
> +  switch (verification_mode_) {
> +    case VERIFY_UNSET:
> +      // Jump to error exit
> +      break;

blank lines between cases (everywhere).

@@ +835,5 @@
> +      peer_cert_ = peer_cert;
> +      return SECSuccess;
> +
> +    case VERIFY_DIGEST: {
> +      CERTCertificate *peer_cert =

Leaks. use ScopedCERTCertificate.

@@ +841,5 @@
> +
> +      PR_ASSERT(digests_.size() != 0);
> +      // Check all the provided digests
> +
> +      SECStatus res= SECFailure;

Add space after res; also, we usually use "rv" as the name for variables like this.

@@ +860,5 @@
> +  }
> +
> +  if (peer_cert) {
> +    CERT_DestroyCertificate(peer_cert);
> +  }

Call PR_SetError before returning SECFailure;

::: media/mtransport/transportlayerdtls.h
@@ +26,5 @@
> +#include "transportlayer.h"
> +
> +struct Packet;
> +
> +class NSPRHelper {

Please name this class (and file) TransportLayerSocketInfo, so that it parallels nsNSSSocketInfo and nsSOCKSSocketInfo. This will make it more obvious what it does.

Document the threading model for this class. Which methods can be called concurrently, if any? There is not any synchronization, so I presume that this is single-thread.

@@ +43,5 @@
> +  TransportLayer *output_;
> +  std::queue<Packet *> input_;
> +};
> +
> +class TransportLayerDtls : public TransportLayer {

Capitalize Dtls as DTLS, and also capitalize this file to match the name of the class (similarly elsewhere).

Comment 46

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I estimate that there are going to be at least two more rounds of reviews. So, it would be good to get the next review started and completed on Wednesday, so that we can do the final review and landing on Friday.

Comment 47

[Eric Rescorla (:ekr)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

::: media/mtransport/build/Makefile.in
@@ +1,2 @@
> +# ***** BEGIN LICENSE BLOCK *****
> +# Version: MPL 1.1/GPL 2.0/LGPL 2.1

So I copied this from another file in Firefox. I can do whatever is appropriate here but I erred on the side of not messing with it.

::: media/mtransport/dtlsidentity.cpp
@@ +16,5 @@
> +
> +// Helper class to avoid having a crapload of if (!NULL) statements at
> +// the end to clean up. The way you use this is you instantiate the
> +// object as scoped_c_ptr<PtrType> obj(value, destructor);
> +// TODO([e-mail]): Move this to some generic location

Yeah, this is a googlism. Would you like a mass change to TODO?

@@ +17,5 @@
> +// Helper class to avoid having a crapload of if (!NULL) statements at
> +// the end to clean up. The way you use this is you instantiate the
> +// object as scoped_c_ptr<PtrType> obj(value, destructor);
> +// TODO([e-mail]): Move this to some generic location
> +template <class T> class scoped_c_ptr {

1. We can't use nsACString. It causes linkage errors.
2. Per discussion on #necko, I believe std::string is OK here

@@ +42,5 @@
> +  void (*d_)(T *);
> +};
> +
> +// Auto-generate an identity based on name=name
> +mozilla::RefPtr<DtlsIdentity> DtlsIdentity::Generate(const std::string name) {

1. Per our discussion the other day, I am going to handle the initialization/shutdown stuff in PeerConnection with the assumption that callers of this API will do the tirhg thing.
2. I don't have a problem with generating a random name, but that's not something we should impose as a requirement on the next layer up. I'm happy to have a nullary Generate function that takes a random name.
3. WRT already_AddRefed<T>, OK, but frankly, it strikes me as false optimization.

@@ +54,5 @@
> +  }
> +
> +  PK11RSAGenParams rsaparams;
> +  rsaparams.keySizeInBits = 1024;
> +  rsaparams.pe = 0x010001;

I believe I copied this from the NSS code. What are you thinking? A comment?

@@ +89,5 @@
> +    return NULL;
> +  }
> +
> +  unsigned long serial;
> +  // Note: This serial in principle could collide, but it's unlikely

OK.

@@ +170,5 @@
> +
> +  if (algorithm == "sha-1") {
> +    ht = HASH_AlgSHA1;
> +  } else if (algorithm == "sha-224") {
> +    ht = HASH_AlgSHA224;

I can make us always generate SHA-2, but I don't see a problem with accepting SHA-1.

@@ +226,5 @@
> +
> +// Parse a fingerprint in RFC 4572 format.
> +// Note that this tolerates some badly formatted data, in particular:
> +// (a) arbitrary runs of colons 
> +// (b) colons at the beginning or end.

I believe that A:F:0 is actually rejected.

I'm tolerating bad formatting because it makes the decoder somewhat simpler. I'm happy to file a bug to tighten this later.

@@ +228,5 @@
> +// Note that this tolerates some badly formatted data, in particular:
> +// (a) arbitrary runs of colons 
> +// (b) colons at the beginning or end.
> +nsresult DtlsIdentity::ParseFingerprint(const std::string fp,
> +                                        unsigned char *digest,

It would have to be std::string<uint8_t>. Is there really not some buffer construct that is more obviously opaque binary bytes?

::: media/mtransport/dtlsidentity.h
@@ +46,5 @@
> +
> +#include "mozilla/RefPtr.h"
> +#include "nsISupportsImpl.h"
> +
> +class DtlsIdentity : public mozilla::RefCounted<DtlsIdentity> {

1. Yes, this is a hangover from when I thought REfCounted<T> was OK.
2. Yes, I'll add something here.

::: media/mtransport/logging.h
@@ +17,5 @@
> +  LogCtx(const char* name) : module_(PR_NewLogModule(name)) {}
> +  LogCtx(std::string& name) : module_(PR_NewLogModule(name.c_str())) {}
> +
> +  PRLogModuleInfo* module() const { return module_; }
> +  

I'll go through en masse.

::: media/mtransport/m_cpp_utils.h
@@ +8,5 @@
> +#define m_cpp_utils_h__
> +
> +#include "mozilla/Attributes.h"
> +
> +#define DISALLOW_ASSIGNMENT(T) \

I believe not. We have MOZ_DELETE but when I asked on #developers there was no macro like this. I would be happy to contribute this macro to MFBT if it's not going to be painful.

::: media/mtransport/nr_timer.cpp
@@ +50,5 @@
> +                       int l, void **handle) {
> +  nsresult rv;
> + 
> +  // TODO([e-mail]): See if this is too expensive and if so cache
> +  nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);

OK

@@ +59,5 @@
> +
> +  timer->InitWithCallback(new nrappkitTimerCallback(cb, arg), timeout,
> +                           nsITimer::TYPE_ONE_SHOT);
> +  
> +  timer->AddRef();

I think I got this right, but I will double-check.

@@ +74,5 @@
> +
> +int NR_async_timer_cancel(void *handle) {
> +  nsITimer *timer = static_cast<nsITimer *>(handle);
> +  
> +  timer->Cancel();

I suspect so. will look into this.

::: media/mtransport/nricectx.cpp
@@ +235,5 @@
> +    nr_crypto_vtbl = &nr_ice_crypto_nss_vtbl;
> +    initialized = true;
> +    
> +    // Set the priorites for candidate type preferences
> +    NR_reg_set_uchar((char *)"ice.pref.type.srv_rflx",100);

The overall plan is to write some code to investigate the interface parameters and then set preferences appropriately. The temporary plan is to hardwire some of the best-known ones and use the auto-setter for hte rest. Maybe we should file a bug?

@@ +268,5 @@
> +      NR_reg_set_uchar((char *)"ice.pref.interface.virbr0", 233);
> +      NR_reg_set_uchar((char *)"ice.pref.interface.wlan0", 232);
> +    }
> +    
> +    NR_reg_set_string((char *)"ice.stun.server.0.addr", (char *)"216.93.246.14");

Will add a comment with the bug #.

::: media/mtransport/nricemediastream.cpp
@@ +181,5 @@
> +         << name_ << "'");
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  char addr[64];

I believe it is one of those "always enough" values. Like, enough for a hex representation of IPv6 with colons. Maybe a comment?

::: media/mtransport/standalone/Makefile.in
@@ +48,5 @@
> +MODULE = mtransport_s
> +LIBRARY_NAME = mtransport_s
> +FORCE_STATIC_LIB= 1
> +ifeq (WINNT,$(OS_TARGET))
> +VISIBILITY_FLAGS =

I am not sure. Ted?

@@ +74,5 @@
> +	$(NULL)
> +
> +
> +export:: $(MTRANSPORT_CPPSRCS)
> +	$(INSTALL) $^ .

Right. this is part of the dual-compilation thing. Ted, are we planning to get rid of that?

::: media/mtransport/transportflow.cpp
@@ +28,5 @@
> +    old_layer->SignalStateChange.disconnect(this);
> +    old_layer->SignalPacketReceived.disconnect(this);
> +  }
> +  layer->SignalStateChange.connect(this, &TransportFlow::StateChange);
> +  layer->SignalPacketReceived.connect(this, &TransportFlow::PacketReceived);

Can you define "safe"?

::: media/mtransport/transportlayerdtls.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: [e-mail]
> +

It seems to me that matching them is exactly what we don't want to do.

In any case, there are a bunch of different TransportLayer*s in this directory, so they certainly can't all be called TransportLayerIOLayer.

@@ +5,5 @@
> +// Original author: [e-mail]
> +
> +#include <queue>
> +
> +

1. The documentation here (if any) should go into TransportLayer.h and TransportFlow.h, since they all share the same semantics.
2. WRT to the more general implementation question you are asking. we have to adapt four different I/O models: 
    (a) NSPR
    (b) nICEr [recall that DTLS is *not* sitting directly on top of UDP]
    (c) SCTP
    (d) SRTP

This sort of event driven model is a much better fit than a pull model like NSPRs, especially since we woudl then have to recast all this other stuff as NSPR I/O layers and because SRTP talks directly to nICEr as well as to the DTLS stack.

3. I don't understand the question about buffering.

@@ +59,5 @@
> +  PRInt32 offset_;
> +};
> +
> +void NSPRHelper::PacketReceived(const void *data, PRInt32 len) {
> +  input_.push(new Packet());

1. On OOM, std::vector either throws std::bad_alloc or crashes, depending on whether -fno-exceptions is set.
2. WRT PacketReceived, what happens here is that TransportLayerDtls::PacketReceived calles NSPRHelper::PacketReceived, which immediately calls PR_Recv() afterward.
That said, I do think it's potentially possible to have some pileup here depending exactly how NSS handles multiple DTLS records in the same frame. I can generate a bug.

I can document the threading model, but briefly everything important has to happen on the same thread.

@@ +117,5 @@
> +}
> +
> +static PRInt32 TransportLayerAvailable(PRFileDesc *f) {
> +  UNIMPLEMENTED;
> +  return -1;

OK, I will modify UNIMPLEMENTED to do that.

@@ +209,5 @@
> +  return written;
> +}
> +
> +static PRInt32 TransportLayerRecvfrom(PRFileDesc *f, void *buf, PRInt32 amount,
> +                       PRIntn flags, PRNetAddr *addr, PRIntervalTime to) {

WRT to the indent, these are supposed to line up with the "(" but a mass rename messed this up.

@@ +242,5 @@
> +
> +static PRStatus TransportLayerGetpeername(PRFileDesc *f, PRNetAddr *addr) {
> +  // TODO([e-mail]): Modify to return unique names for each channel
> +  // somehow, as opposed to always the same static address. The current
> +  // implementation messes up the session cache, which is why it's off

Yes, it's called.

@@ +536,5 @@
> +  downward_->SignalPacketReceived.connect(this, &TransportLayerDtls::PacketReceived);
> +
> +  if (downward_->state() == OPEN) {
> +    Handshake();
> +  }

I don't think so... Why?

@@ +644,5 @@
> +  }
> +
> +  // Now try a recv if we're open, since there might be data left
> +  if (state_ == OPEN) {
> +    unsigned char buf[2000];

The usual "more than enough room for IP" theory, since we don't know the interface MTU. I would be ok with making this larger but then as I think you suggested, it shouldn't be on the stack. Thoughts?

@@ +728,5 @@
> +  return SECSuccess;
> +}
> +
> +nsresult TransportLayerDtls::SetSrtpCiphers(std::vector<PRUint16> ciphers) {
> +  // TODO([e-mail]): We should check these

I think it's a nice-to-have.

@@ +828,5 @@
> +  PR_ASSERT(peer_cert_ == NULL);
> +
> +  switch (verification_mode_) {
> +    case VERIFY_UNSET:
> +      // Jump to error exit

I do not believe that there is a missing goto. This break jumps out of the switch and thus to a SECFailure return.

Do you think there is a logic error here, or just that the comment isn't clear.

@@ +855,5 @@
> +        // Matches all digests, we are good to go
> +        peer_cert_ = peer_cert;
> +        return SECSuccess;
> +      }
> +    }

This is just belt-and-suspenders for when we add another case?

::: media/mtransport/transportlayerdtls.h
@@ +26,5 @@
> +#include "transportlayer.h"
> +
> +struct Packet;
> +
> +class NSPRHelper {

1. Which class? NSPRHelper or TransportLayerDtls?
2. Yes, I'll add something.

@@ +32,5 @@
> +  NSPRHelper(TransportLayer *output) :
> +      output_(output),
> +      input_() {}
> +
> +  void PacketReceived(const void *data, PRInt32 len);

I believe Jesup is planning to take care of this with some script of his.

@@ +40,5 @@
> + private:
> +  DISALLOW_COPY_ASSIGN(NSPRHelper);
> +
> +  TransportLayer *output_;
> +  std::queue<Packet *> input_;

It handles out of memory in the usual way. By throwing an exception and if -fno-exceptions, by crashing.

@@ +62,5 @@
> +
> +
> +  enum Role { CLIENT, SERVER};
> +  enum Verification { VERIFY_UNSET, VERIFY_ALLOW_ALL, VERIFY_DIGEST};
> +  const static int kMaxDigestLength = 64;

It's the size of the largest known digest, SHA-512. Comment?

We can't use ns*String for linkage reasons.

@@ +68,5 @@
> +  // DTLS-specific operations
> +  void SetRole(Role role) { role_ = role;}
> +  Role role() { return role_; }
> +
> +  void SetIdentity(mozilla::RefPtr<DtlsIdentity> identity) {

already_AddRefed and mozilla::RefPtr do not play nice together, and indeed apparently
there is some controversy about whether you should use nsCOMPtr and RefPtr together

https://bugzilla.mozilla.org/show_bug.cgi?id=776358

@@ +82,5 @@
> +
> +  nsresult ExportKeyingMaterial(const std::string& label,
> +                                bool use_context,
> +                                const std::string& context,
> +                                unsigned char *out,

I don't see how this removes buffer overflows, since there is going to be a memcpy inside NSS.

@@ +85,5 @@
> +                                const std::string& context,
> +                                unsigned char *out,
> +                                unsigned int outlen);
> +
> +  const CERTCertificate *GetPeerCert() const { return peer_cert_; }

OK.

@@ +153,5 @@
> +  Verification verification_mode_;
> +  std::vector<mozilla::RefPtr<VerificationDigest> > digests_;
> +
> +  PRFileDesc *pr_fd_;
> +  PRFileDesc *ssl_fd_;

Sure. I didn't know that this and ScopedCERTCertificate existed.

::: media/mtransport/transportlayerloopback.cpp
@@ +22,5 @@
> +#include "transportlayerloopback.h"
> +
> +MLOG_INIT("mtransport");
> +
> +nsresult TransportLayerLoopback::Init() {

It's just for the test harness. But we do want the unit tests to have this, so it needs to be built in opt builds.

::: media/mtransport/transportlayerprsock.cpp
@@ +87,5 @@
> +  }
> +
> +  MLOG(PR_LOG_DEBUG, LAYER_INFO << "OnSocketReady(flags=" << outflags << ")");
> +  
> +  unsigned char buf[1600];

Yeah, this is just my Ethernet myopia.

As for unsigned char, I'm not sure what the advantage of uint8_t is here.

::: toolkit/toolkit-tiers.mk
@@ -136,2 @@
>    media/webrtc \
> -  media/mtransport/third_party \

Huh. Pilot error.

Comment 48

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

As mentioned on IRC, in the automated tests, use the PSM XPCOM component (NS_NSSCOMPONENT_CID) to initialize and shutdown NSS, to better simulate the browser environment that this code will be run in. Note that you will need to create a profile directory to use PSM. Look at my code in bug 767231 and/or in xpcom/test to see how to do this.

For the things that you expect to fix in follow-up bugs: Please file the bug, assigned to yourself, reference the bug in the review comments, and have it block the "Enable WebRTC by default" bug. Then, Randell can triage them later to determine which things really need to block.

::: media/mtransport/build/Makefile.in
@@ +1,2 @@
> +# ***** BEGIN LICENSE BLOCK *****
> +# Version: MPL 1.1/GPL 2.0/LGPL 2.1

For new files that aren't originally licensed by somebody else, use:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line

(Note also the mode line).

::: media/mtransport/dtlsidentity.cpp
@@ +17,5 @@
> +// Helper class to avoid having a crapload of if (!NULL) statements at
> +// the end to clean up. The way you use this is you instantiate the
> +// object as scoped_c_ptr<PtrType> obj(value, destructor);
> +// TODO([e-mail]): Move this to some generic location
> +template <class T> class scoped_c_ptr {

Yes, std::string, vector, etc. are OK. I understand that this code cannot use the Mozilla string API due to linkage issues.

@@ +42,5 @@
> +  void (*d_)(T *);
> +};
> +
> +// Auto-generate an identity based on name=name
> +mozilla::RefPtr<DtlsIdentity> DtlsIdentity::Generate(const std::string name) {

1. OK, let me know what part of the PeerConnection code is (or will be) handling this so I can look at it.
2. If we know that all of our existing code can use a random name without problem, then let's just do that because YAGNI. Otherwise, I have to review all the callers too.
3. already_AddRefed<T> is more about readability of the code than optimization.

@@ +54,5 @@
> +  }
> +
> +  PK11RSAGenParams rsaparams;
> +  rsaparams.keySizeInBits = 1024;
> +  rsaparams.pe = 0x010001;

The decimal value is easier to read. I don't care about the comment so much.

@@ +170,5 @@
> +
> +  if (algorithm == "sha-1") {
> +    ht = HASH_AlgSHA1;
> +  } else if (algorithm == "sha-224") {
> +    ht = HASH_AlgSHA224;

If we all (browsers implementing WebRTC) can agree to avoid SHA-1, it will save trouble down the road. People are already asking us to transition away from SHA-1 in other areas. If we're not sure about that agreement, please file a bug about it that blocks the "Enable WebRTC by default" bug.

@@ +228,5 @@
> +// Note that this tolerates some badly formatted data, in particular:
> +// (a) arbitrary runs of colons 
> +// (b) colons at the beginning or end.
> +nsresult DtlsIdentity::ParseFingerprint(const std::string fp,
> +                                        unsigned char *digest,

It is OK to avoid making this change, because this is already so pervasive and we can't use the Mozilla string API that would help make things less painful.

::: media/mtransport/transportlayerdtls.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: [e-mail]
> +

I don't understand the first sentence of your comment. My point is that it is likely that the people that work on Necko will be working on this code, so it is worth making it more like the Necko/PSM code so that it is easier to figure out what's going on. I understand now that it makes less sense to rename the file than I thought; ignore that request.

@@ +5,5 @@
> +// Original author: [e-mail]
> +
> +#include <queue>
> +
> +

I am not questioning the design decision. My point is that, as someone wholly new to this code, and with experience with the existing Mozilla networking code, it is helpful (for others) to explain here that we're adapting the the TransportLayer's push model into NSPR's pull model, and that the pull model is inappropriate for TransportLayer.

@@ +59,5 @@
> +  PRInt32 offset_;
> +};
> +
> +void NSPRHelper::PacketReceived(const void *data, PRInt32 len) {
> +  input_.push(new Packet());

When you say "everything important," do you mean that some things aren't expected to be done in a single-threaded fashion? If so, which things?

@@ +536,5 @@
> +  downward_->SignalPacketReceived.connect(this, &TransportLayerDtls::PacketReceived);
> +
> +  if (downward_->state() == OPEN) {
> +    Handshake();
> +  }

Never mind this comment.

@@ +644,5 @@
> +  }
> +
> +  // Now try a recv if we're open, since there might be data left
> +  if (state_ == OPEN) {
> +    unsigned char buf[2000];

I thought that somewhere in this feature there is (or was supposed to be) MTU dscovery logic of some kind?

::: media/mtransport/transportlayerdtls.h
@@ +26,5 @@
> +#include "transportlayer.h"
> +
> +struct Packet;
> +
> +class NSPRHelper {

NSPRHelper.

@@ +62,5 @@
> +
> +
> +  enum Role { CLIENT, SERVER};
> +  enum Verification { VERIFY_UNSET, VERIFY_ALLOW_ALL, VERIFY_DIGEST};
> +  const static int kMaxDigestLength = 64;

#include "hasht.h"
const static int kMaxDigestLength = HASH_LENGTH_MAX;

@@ +68,5 @@
> +  // DTLS-specific operations
> +  void SetRole(Role role) { role_ = role;}
> +  Role role() { return role_; }
> +
> +  void SetIdentity(mozilla::RefPtr<DtlsIdentity> identity) {

That looks like the start of a long and unfun conversation.

Actually, I am unsure why you pass the mozilla::RefPtr<DtlsIdentity> instead of just DtlsIdentity*, which is the normal convention in Gecko.

@@ +73,5 @@
> +    identity_ = identity;
> +  }
> +  nsresult SetVerificationAllowAll();
> +  nsresult SetVerificationDigest(const std::string digest_algorithm,
> +                                 const unsigned char *digest_value,

As above, just disregard this comment since we don't have the Mozilla String API available for this code.

@@ +82,5 @@
> +
> +  nsresult ExportKeyingMaterial(const std::string& label,
> +                                bool use_context,
> +                                const std::string& context,
> +                                unsigned char *out,

As above, just disregard this comment since we don't have the Mozilla String API available for this code.

@@ +106,5 @@
> +  // A single digest to check
> +  class VerificationDigest {
> +   public:
> +    VerificationDigest(std::string algorithm,
> +                       const unsigned char *value, size_t len) {

As above, just disregard this comment since we don't have the Mozilla String API available for this code.

@@ +153,5 @@
> +  Verification verification_mode_;
> +  std::vector<mozilla::RefPtr<VerificationDigest> > digests_;
> +
> +  PRFileDesc *pr_fd_;
> +  PRFileDesc *ssl_fd_;

Looks like we need to add ScopedPRFileDesc to ScopedNSSTypes.h. (We can move it to some non-NSS-specific file later if/when people complain about depending on NSS and/or PSM.)

Comment 49

[Eric Rescorla (:ekr)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

A few quick responses.

::: media/mtransport/transportlayerdtls.cpp
@@ +59,5 @@
> +  PRInt32 offset_;
> +};
> +
> +void NSPRHelper::PacketReceived(const void *data, PRInt32 len) {
> +  input_.push(new Packet());

Actually, everything.  It's just that some of the code *must* be on the STS thread whereas other code must just be externally mutexed.

@@ +743,5 @@
> +  }
> +
> +  return NS_OK;
> +}
> +

Currently all the callers.

@@ +775,5 @@
> +
> +  return stream->AuthCertificateHook(fd, checksig, isServer);
> +}
> +
> +SECStatus TransportLayerDtls::CheckDigest(mozilla::RefPtr<VerificationDigest> digest,

Respond here but you asked this in a bunch of places.

My view is that it's very bad form to intermix raw ptrs and smart ptrs, so once you have RefPtrized something, you should try to avoid accessing the raw ptr.

I thought that was a common view at Moz (and indeed that I had seen bsmedberg advocate it). Am I wrong?

Comment 50

[Eric Rescorla (:ekr)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

A few quick responses.

::: media/mtransport/transportlayerdtls.cpp
@@ +59,5 @@
> +  PRInt32 offset_;
> +};
> +
> +void NSPRHelper::PacketReceived(const void *data, PRInt32 len) {
> +  input_.push(new Packet());

Actually, everything.  It's just that some of the code *must* be on the STS thread whereas other code must just be externally mutexed.

@@ +743,5 @@
> +  }
> +
> +  return NS_OK;
> +}
> +

Currently all the callers.

@@ +775,5 @@
> +
> +  return stream->AuthCertificateHook(fd, checksig, isServer);
> +}
> +
> +SECStatus TransportLayerDtls::CheckDigest(mozilla::RefPtr<VerificationDigest> digest,

Respond here but you asked this in a bunch of places.

My view is that it's very bad form to intermix raw ptrs and smart ptrs, so once you have RefPtrized something, you should try to avoid accessing the raw ptr.

I thought that was a common view at Moz (and indeed that I had seen bsmedberg advocate it). Am I wrong?

Comment 51

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Eric Rescorla from comment #47)
> > +// TODO([e-mail]): Move this to some generic location
> 
> Yeah, this is a googlism. Would you like a mass change to TODO?

Sure.

> @@ +17,5 @@
> > +// Helper class to avoid having a crapload of if (!NULL) statements at
> > +// the end to clean up. The way you use this is you instantiate the
> > +// object as scoped_c_ptr<PtrType> obj(value, destructor);
> > +// TODO([e-mail]): Move this to some generic location
> > +template <class T> class scoped_c_ptr {
> 
> 1. We can't use nsACString. It causes linkage errors.

Is this due to the C++ unit test issue?

> @@ +54,5 @@
> > +  }
> > +
> > +  PK11RSAGenParams rsaparams;
> > +  rsaparams.keySizeInBits = 1024;
> > +  rsaparams.pe = 0x010001;
> 
> I believe I copied this from the NSS code. What are you thinking? A comment?

That would be fine. Unexplained magic constants == bad.

> ::: media/mtransport/m_cpp_utils.h
> @@ +8,5 @@
> > +#define m_cpp_utils_h__
> > +
> > +#include "mozilla/Attributes.h"
> > +
> > +#define DISALLOW_ASSIGNMENT(T) \
> 
> I believe not. We have MOZ_DELETE but when I asked on #developers there was
> no macro like this. I would be happy to contribute this macro to MFBT if
> it's not going to be painful.

mfbt/double-conversion/utils.h (imported from V8) defines among others:

ARRAY_SIZE(array)
DISALLOW_COPY_AND_ASSIGN(type)
DISALLOW_IMPLICIT_CONSTRUCTORS(type)
Min<T>(a,b), Max<T>(a,b)
Vector<T>
UNIMPLEMENTED
UNREACHABLE
(though I wish those calls used MOZ_ASSERT() before abort())

Moving pieces of those to a more general include in MFBT would be reasonable (perhaps not all).  Independent bug.


> ::: media/mtransport/standalone/Makefile.in
> @@ +48,5 @@
> > +MODULE = mtransport_s
> > +LIBRARY_NAME = mtransport_s
> > +FORCE_STATIC_LIB= 1
> > +ifeq (WINNT,$(OS_TARGET))
> > +VISIBILITY_FLAGS =
> 
> I am not sure. Ted?
> 
> @@ +74,5 @@
> > +	$(NULL)
> > +
> > +
> > +export:: $(MTRANSPORT_CPPSRCS)
> > +	$(INSTALL) $^ .
> 
> Right. this is part of the dual-compilation thing. Ted, are we planning to
> get rid of that?

You should ping ted directly because there a lot of long messages here...

> ::: media/mtransport/transportflow.cpp
> @@ +28,5 @@
> > +    old_layer->SignalStateChange.disconnect(this);
> > +    old_layer->SignalPacketReceived.disconnect(this);
> > +  }
> > +  layer->SignalStateChange.connect(this, &TransportFlow::StateChange);
> > +  layer->SignalPacketReceived.connect(this, &TransportFlow::PacketReceived);
> 
> Can you define "safe"?

Yes: my comment (not copied) said "and doesn't lose signals".  As in, is there a race condition between disconnecting from the old layer and connecting to the new one where a signal could be lost?  

From looking at the sigslot docs on sourceforge, this *is* a problem - if StateChange or PacketReceived is emitted between the .disconnect and the .connect, it's lost.

Please consider flipping the order, but then you'll need to make sure you handle double-notification (both old_layer and layer will get the signal).  Or make a slot-holder object that you can redirect:

  layer->mSignalProxy = old_layer->mSignalProxy;
  layer->DirectSignalsTo(layer);

(I'm assuming SignalProxy is roughly "nsRefPtr<SigslotProxy> mSignalProxy;"
and
SigslotProxy::DirectSignalsTo(Blah *blah) { mTarget = blah; }
and
SigslotProxy::StateChange(...) { mTarget->StateChange(...); }

> 
> ::: media/mtransport/transportlayerdtls.cpp

> 
> 1. The documentation here (if any) should go into TransportLayer.h and
> TransportFlow.h, since they all share the same semantics.

Agreed

> 2. WRT to the more general implementation question you are asking. we have
> to adapt four different I/O models: 
>     (a) NSPR
>     (b) nICEr [recall that DTLS is *not* sitting directly on top of UDP]
>     (c) SCTP
>     (d) SRTP
> 
> This sort of event driven model is a much better fit than a pull model like
> NSPRs, especially since we woudl then have to recast all this other stuff as
> NSPR I/O layers and because SRTP talks directly to nICEr as well as to the
> DTLS stack.

I agree with ekr here.

> 
> 3. I don't understand the question about buffering.
> 
> @@ +59,5 @@
> > +  PRInt32 offset_;
> > +};
> > +
> > +void NSPRHelper::PacketReceived(const void *data, PRInt32 len) {
> > +  input_.push(new Packet());
> 
> 1. On OOM, std::vector either throws std::bad_alloc or crashes, depending on
> whether -fno-exceptions is set.

Per bsmedberg, std::vector is ok.

> @@ +644,5 @@
> > +  }
> > +
> > +  // Now try a recv if we're open, since there might be data left
> > +  if (state_ == OPEN) {
> > +    unsigned char buf[2000];
> 
> The usual "more than enough room for IP" theory, since we don't know the
> interface MTU. I would be ok with making this larger but then as I think you
> suggested, it shouldn't be on the stack. Thoughts?

High-water-mark packet buffer used *only* if we've seen a too-large packet.  I.e.:
    unsigned char buf[15xx]; // big enough for one 10/100T ethernet packet
    unsigned char *bufptr = mBuffer ? mBuffer : &buf[0];
    size_t bufsize = mBuffer ? mBufferSize : sizeof(buf);
...
    if (too_big_for_buffer) {
       mBuffer = mBuffer ? realloc(mBuffer,...) : malloc(...);
       mBufferSize = ...;
    }

Or something like that.

> @@ +828,5 @@
> > +  PR_ASSERT(peer_cert_ == NULL);
> > +
> > +  switch (verification_mode_) {
> > +    case VERIFY_UNSET:
> > +      // Jump to error exit
> 
> I do not believe that there is a missing goto. This break jumps out of the
> switch and thus to a SECFailure return.
> 
> Do you think there is a logic error here, or just that the comment isn't
> clear.

Remove 'Jump to' - perhaps "clean up and return SECFailure"


> @@ +32,5 @@
> > +  NSPRHelper(TransportLayer *output) :
> > +      output_(output),
> > +      input_() {}
> > +
> > +  void PacketReceived(const void *data, PRInt32 len);
> 
> I believe Jesup is planning to take care of this with some script of his.

Ehsan's script or my tweak to it to cover all patch queues (not just primary), but yes.

> @@ +62,5 @@
> > +
> > +
> > +  enum Role { CLIENT, SERVER};
> > +  enum Verification { VERIFY_UNSET, VERIFY_ALLOW_ALL, VERIFY_DIGEST};
> > +  const static int kMaxDigestLength = 64;
> 
> It's the size of the largest known digest, SHA-512. Comment?

See bsmith's comment

> We can't use ns*String for linkage reasons.

Unit tests I assume
 
> ::: media/mtransport/transportlayerprsock.cpp
> @@ +87,5 @@
> > +  }
> > +
> > +  MLOG(PR_LOG_DEBUG, LAYER_INFO << "OnSocketReady(flags=" << outflags << ")");
> > +  
> > +  unsigned char buf[1600];
> 
> Yeah, this is just my Ethernet myopia.

See above for ideas
 
> As for unsigned char, I'm not sure what the advantage of uint8_t is here.

Minimal, but the point of uintNN_t is that it isolates us from knowing what sizes "C types" are.

Comment 52

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Brian Smith (:bsmith) from comment #48)

> For the things that you expect to fix in follow-up bugs: Please file the
> bug, assigned to yourself, reference the bug in the review comments, and
> have it block the "Enable WebRTC by default" bug. Then, Randell can triage
> them later to determine which things really need to block.

Bug 796463

> 
> ::: media/mtransport/build/Makefile.in
> @@ +1,2 @@
> > +# ***** BEGIN LICENSE BLOCK *****
> > +# Version: MPL 1.1/GPL 2.0/LGPL 2.1
> 
> For new files that aren't originally licensed by somebody else, use:
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Coding_Style#Mode_Line
> 
> (Note also the mode line).

Good point on the mode

> @@ +54,5 @@
> > +  }
> > +
> > +  PK11RSAGenParams rsaparams;
> > +  rsaparams.keySizeInBits = 1024;
> > +  rsaparams.pe = 0x010001;
> 
> The decimal value is easier to read. I don't care about the comment so much.

Ok.  I flagged it only because as a non-NSS guy looking at this, I had no idea what it was or why.

> ::: media/mtransport/transportlayerdtls.cpp
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +// Original author: [e-mail]
> > +
> 
> I don't understand the first sentence of your comment. My point is that it
> is likely that the people that work on Necko will be working on this code,
> so it is worth making it more like the Necko/PSM code so that it is easier
> to figure out what's going on. I understand now that it makes less sense to
> rename the file than I thought; ignore that request.

My position on that is that for code that has similar APIs and semantics (and use-patterns) to existing Necko code, there's an argument to make it mimic the naming/API conventions.  If using naming suggestive of other Necko interfaces might lead one astray because the actual usage pattern or invariants are sufficiently different, then a different naming helps avoid over-reliance on "knowing" what something does.

Comment 53

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

::: media/mtransport/transportlayerdtls.cpp
@@ +743,5 @@
> +  }
> +
> +  return NS_OK;
> +}
> +

I will review how this is used in the next pass.

@@ +752,5 @@
> +                                                  unsigned int outlen) {
> +  SECStatus rv = SSL_ExportKeyingMaterial(ssl_fd_,
> +                                          label.c_str(),
> +                                          label.size(),
> +                                          use_context ? PR_TRUE : PR_FALSE,

you can use implicit conversion from bool -> PRBool

@@ +775,5 @@
> +
> +  return stream->AuthCertificateHook(fd, checksig, isServer);
> +}
> +
> +SECStatus TransportLayerDtls::CheckDigest(mozilla::RefPtr<VerificationDigest> digest,

So many smart pointer types in Gecko now. I could only find a single use of RefPtr<T> as a parameter to a function. I found that, for return alues, instead of already_AddRefed<T>, we're supposed to use TemporaryRef<T>. It seems to me that the bad part of using RefPtr<T> as a parameter is that it will cause an extra AddRef() and Release()--not a big deal here, but not a good habit to get into.

See http://stackoverflow.com/questions/2502394/the-cost-of-passing-by-shared-ptr and http://channel9.msdn.com/Shows/Going+Deep/C-and-Beyond-2011-Scott-Andrei-and-Herb-Ask-Us-Anything. That's about shared_ptr, but it's the same thing, AFAICT.

(Passing AutoPtr by value as a parameter makes a lot more sense, and indeed that seems to be more common in some small parts of Gecko.)

Comment 54

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

::: media/mtransport/transportlayerdtls.h
@@ +32,5 @@
> +  NSPRHelper(TransportLayer *output) :
> +      output_(output),
> +      input_() {}
> +
> +  void PacketReceived(const void *data, PRInt32 len);

*Please* do those fix-ups script *at least* before this gets checked in, if not right away, to avoid future readers of the code having to navigate through a "fix naming and types" revision when using the blame tools.

::: media/mtransport/transportlayerprsock.cpp
@@ +87,5 @@
> +  }
> +
> +  MLOG(PR_LOG_DEBUG, LAYER_INFO << "OnSocketReady(flags=" << outflags << ")");
> +  
> +  unsigned char buf[1600];

Generally, we try to use 'char' types for text and uint8_t for non-text data.

Comment 55

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 664195 [details] [diff] [review]
Patch 3: build nICEr and nrappkit  r=ted

Corrying r+ forward from ted

Comment 56

[Eric Rescorla (:ekr)]

Attachment: Patch 3: build nICEr and nrappkit r=ted

Comment 57

[Patrick McManus [:mcmanus]]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

this rocks.

there are no showstoppers here from my perspective. There are a bunch of comments, mostly they can be addressed without further review or discussion.. if its just a piece of advice feel free to use your judgment on whether to reject it (please comment on them), implement it, file a followup, or discuss it. r=mcmanus assuming you go through the list.

I did look at nr_socket_prsock* in good detail and its interaction with necko.

nricemediastream*, nricectx* (but not anything psm or nss related), transportflow*, transportlayer.[cpp|h] and transportlayerice.* in some detail

a quick read of sockettransportservice_unittest, transportlayerprsock.cpp/h and transportlayerloopback* but didn't have much to say beyond the usual formatting fluff.

any other comments are just drive-bys.

let me know if there is something else in particular you'd like me to read.

::: media/mtransport/dtlsidentity.h
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

bitrotted license text

::: media/mtransport/nr_socket_prsock.cpp
@@ +111,5 @@
> +NS_IMPL_THREADSAFE_ISUPPORTS0(NrSocket);
> +
> +
> +// The nsASocket callbacks
> +void NrSocket::OnSocketReady(PRFileDesc *fd, PRInt16 outflags) {

silly brace nit 
void foo::bar()
{
}

for general whitespace cleanups (spaces after commas but not before parens, stray newlines, etc..) I recommend jst-review.pl.. it has turned me from an egregious whitespace mangler into just a semi-bad one.

@@ +307,5 @@
> +      ABORT(r);
> +  }
> +
> +  // Finally, register with the STS
> +  rv = stservice_->AttachSocket(fd_, this);

assert we're on the socket thread

@@ +319,5 @@
> +  return(_status);
> +}
> +
> +int NrSocket::sendto(const void *msg, size_t len,
> +                     int flags, nr_transport_addr *to) {

some notes about threading et al would be useful here.

I assume this is meant to be run on the socket thread and the STS is dispatching it.

@@ +331,5 @@
> +  if(fd_==NULL)
> +    ABORT(R_EOD);
> +
> +  // TODO([e-mail]): Convert flags?
> +  status = PR_SendTo(fd_, msg, len, flags, &naddr, PR_INTERVAL_NO_WAIT);

you might want to also setsockopt PR_SockOpt_Nonblocking .. from browsing some imlpementations I'm not 100% confident PR_INTERVAL_NO_WAIT is going to always boil down to the same thing. That is what you want, right?

@@ +373,5 @@
> +}
> +
> +// Close the socket so that the STS will detach and then kill it
> +void NrSocket::close() {
> +  mCondition = NS_BASE_STREAM_CLOSED;

assert socket thread if you're messing with state used by the socket transport service

@@ +398,5 @@
> +};
> +
> +
> +int nr_socket_local_create(nr_transport_addr *addr, nr_socket **sockp) {
> +  NrSocket *sock = new NrSocket();

nsRefPtr<nrSocket> sock = new NRSocket();

@@ +410,5 @@
> +  if (r)
> +    ABORT(r);
> +
> +  // Add a reference so that we can delete it in destroy()
> +  sock->AddRef();

sock.forget()

@@ +416,5 @@
> +  _status =0;
> +
> +abort:
> +  if (_status) {
> +    delete sock;

if this is refptr you don't need this

@@ +426,5 @@
> +static int nr_socket_local_destroy(void **objp) {
> +  if(!objp || !*objp)
> +    return 0;
> +
> +  NrSocket *sock = static_cast<NrSocket *>(*objp);

nsRefPtr<NrSocket> sock = dont_AddRef(static_cast<NrSocket *>(*objp));

@@ +430,5 @@
> +  NrSocket *sock = static_cast<NrSocket *>(*objp);
> +  *objp=0;
> +
> +  sock->close();  // Signal STS that we want not to listen
> +  sock->Release();  // Decrement the ref count

don't need the release if its a refptr

::: media/mtransport/nr_socket_prsock.h
@@ +37,5 @@
> +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +*/
> +
> +
> +// Implementation of nICEr/nr_socket that is tied to the Firefox STS

s/STS/SocketTransportService ( i was actually thinking strict transport security and was confused )
s/Firefox/Gecko

@@ +53,5 @@
> +#include "nsISocketTransportService.h"
> +#include "nsXPCOM.h"
> +
> +#include "m_cpp_utils.h"
> +

define this in a namespace.. at least mozilla.. maybe mozilla::media ?

@@ +65,5 @@
> +  virtual ~NrSocket() {
> +    PR_Close(fd_);
> +  }
> +
> +  // Implement nsASocket

these functions are going to be called on the socket thread. Do you have any concerns about racing against your other functions (or dtor)?

@@ +73,5 @@
> +  // nsISupports methods
> +  NS_DECL_ISUPPORTS
> +
> +  // Implementations of the async_event APIs
> +  int async_wait(int how, NR_async_cb cb, void *cb_arg,

There are a bunch of obvious naming things here that should be fixed up.. both methods async_wait ->AsyncWait() and members fd_ ->mFD

@@ +97,5 @@
> +  PRFileDesc *fd_;
> +  nr_transport_addr my_addr_;
> +  NR_async_cb cbs_[2];
> +  void *cb_args_[2];
> +  nsCOMPtr<nsISocketTransportService> stservice_;

this can just be a local stack variable, you don't need to have the reference around

::: media/mtransport/nricectx.cpp
@@ +80,5 @@
> +#include "nricemediastream.h"
> +
> +MLOG_INIT("mtransport");
> +
> +static bool initialized = false;

using namespace mozilla

@@ +235,5 @@
> +    nr_crypto_vtbl = &nr_ice_crypto_nss_vtbl;
> +    initialized = true;
> +    
> +    // Set the priorites for candidate type preferences
> +    NR_reg_set_uchar((char *)"ice.pref.type.srv_rflx",100);

definitely file a bug

@@ +311,5 @@
> +    return NULL;
> +  }
> +
> +  nsresult rv;
> +  ctx->sts_target_ = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);

it does seem like there ought to be a nicer way to do that.. but I guess not :(

::: media/mtransport/nricectx.h
@@ +69,5 @@
> +typedef struct nr_ice_cand_pair_ nr_ice_cand_pair;
> +
> +class NrIceMediaStream;
> +
> +class NrIceCtx : public mozilla::RefCounted<NrIceCtx> {

put this in namespace mozilla or mozilla::media and you won't need all the mozilla:: identifiers.

@@ +166,5 @@
> +  nr_ice_ctx *ctx_;
> +  nr_ice_peer_ctx *peer_;
> +  nr_ice_handler_vtbl* ice_handler_vtbl_;  // Must be pointer
> +  nr_ice_handler* ice_handler_;  // Must be pointer
> +  nsCOMPtr<nsIEventTarget> sts_target_; // The thread to run on

I was wondering where this would go :)

::: media/mtransport/nricemediastream.cpp
@@ +66,5 @@
> +#include "nricectx.h"
> +#include "nricemediastream.h"
> +
> +MLOG_INIT("mtransport");
> +

using namesapce mozilla

@@ +81,5 @@
> +                                  components, &stream->stream_);
> +  if (r) {
> +    MLOG(PR_LOG_ERROR, "Couldn't create ICE media stream for '"
> +         << name << "'");
> +    return NULL;

s/NULL/nullptr

::: media/mtransport/nricemediastream.h
@@ +31,5 @@
> +
> +typedef struct nr_ice_media_stream_ nr_ice_media_stream;
> +
> +class NrIceCtx;
> +

define in namespace mozilla or mozilla::media

::: media/mtransport/test/sockettransportservice_unittest.cpp
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: [e-mail]

I think this file is cool.

::: media/mtransport/transportflow.h
@@ +41,5 @@
> +  // Data received on the flow
> +  sigslot::signal3<TransportFlow*, const unsigned char *, size_t>
> +    SignalPacketReceived;
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(TransportFlow);

I didn't know about INLINE. nice!

::: media/mtransport/transportlayerdtls.cpp
@@ +48,5 @@
> +  ~Packet() {
> +    delete data_;
> +  }
> +  void Assign(const void *data, PRInt32 len) {
> +    data_ = new unsigned char[len];

I would consider a fallible path here depending on the range of len (which isn't clear to me).. oom does happen a lot with big allocations and we try and trap the big ones.

::: media/mtransport/transportlayerice.cpp
@@ +36,5 @@
> +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +*/
> +

I would ask gerv what he thinks of this copyright notice arrangement used throughout mtransport

@@ +77,5 @@
> +#ifdef ERROR
> +#undef ERROR
> +#endif
> +
> +MLOG_INIT("mtransport");

I think brian mentioned how we avoid global c++ ctors... but if he didn't I am doing it now :)

::: media/mtransport/transportlayerice.h
@@ +22,5 @@
> +
> +#include "nricemediastream.h"
> +#include "transportflow.h"
> +#include "transportlayer.h"
> +

put it into namespace mozilla or mozilla::media

::: media/mtransport/transportlayerloopback.h
@@ +26,5 @@
> +
> +class TransportLayerLoopback : public TransportLayer {
> + public:
> +  TransportLayerLoopback() :
> +      peer_(NULL),

the whole patch should be scrubbed of bitrotted NULL and PR?Int

@@ +78,5 @@
> +  class QueuedPacket {
> +   public:
> +    QueuedPacket() : data_(NULL), len_(0) {}
> +    ~QueuedPacket() {
> +      delete data_;

delete[] ?

Comment 58

[Eric Rescorla (:ekr)]

Attachment: Update of patch 4; WIP

Comment 59

[Eric Rescorla (:ekr)]

Attachment: Update of patch 4; WIP

Comment 60

[Eric Rescorla (:ekr)]

Attachment: Patch 4: Generic media transport subsystem with modules for ICE and DTLS

Comment 61

[Eric Rescorla (:ekr)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

::: media/mtransport/dtlsidentity.cpp
@@ +46,5 @@
> +mozilla::RefPtr<DtlsIdentity> DtlsIdentity::Generate(const std::string name) {
> +  SECStatus rv;
> +
> +  std::string subject_name_string = "CN=" + name;
> +  CERTName *subject_name = CERT_AsciiToName(

Can you add one to ScopedNSSTypes?

@@ +54,5 @@
> +  }
> +
> +  PK11RSAGenParams rsaparams;
> +  rsaparams.keySizeInBits = 1024;
> +  rsaparams.pe = 0x010001;

Done

@@ +60,5 @@
> +  scoped_c_ptr<SECKEYPrivateKey> private_key(SECKEY_DestroyPrivateKey);
> +  scoped_c_ptr<SECKEYPublicKey> public_key(SECKEY_DestroyPublicKey);
> +  SECKEYPublicKey *pubkey;
> +
> +  private_key = PK11_GenerateKeyPair(PK11_GetInternalSlot(),

Done.

@@ +79,5 @@
> +  if (!certreq.get()) {
> +    return NULL;
> +  }
> +
> +  PRTime notBefore = PR_Now() - (86400UL * PR_USEC_PER_SEC);

Added a comment.

@@ +89,5 @@
> +    return NULL;
> +  }
> +
> +  unsigned long serial;
> +  // Note: This serial in principle could collide, but it's unlikely

Done.

@@ +110,5 @@
> +  if (rv != SECSuccess)
> +    return NULL;
> +
> +  // Set version to X509v3.
> +  *(certificate.get()->version.data) = 2;

Done.

@@ +113,5 @@
> +  // Set version to X509v3.
> +  *(certificate.get()->version.data) = 2;
> +  certificate.get()->version.len = 1;
> +
> +  SECItem innerDER;

My understanding is that the data is allocated in the arena.

@@ +119,5 @@
> +  innerDER.data = NULL;
> +
> +  if (!SEC_ASN1EncodeItem(arena, &innerDER, certificate.get(),
> +                          SEC_ASN1_GET(CERT_CertificateTemplate)))
> +    return NULL;

Changed this.

@@ +121,5 @@
> +  if (!SEC_ASN1EncodeItem(arena, &innerDER, certificate.get(),
> +                          SEC_ASN1_GET(CERT_CertificateTemplate)))
> +    return NULL;
> +
> +  SECItem *signedCert 

My understanding is that the data is allocated in the arena.

@@ +141,5 @@
> +}
> +
> +
> +
> +DtlsIdentity::~DtlsIdentity() {

Still TODO

@@ +153,5 @@
> +nsresult DtlsIdentity::ComputeFingerprint(const std::string algorithm,
> +                                          unsigned char *digest,
> +                                          std::size_t size,
> +                                          std::size_t *digest_length) {
> +  PR_ASSERT(cert_);

This will be handled externally.

@@ +170,5 @@
> +
> +  if (algorithm == "sha-1") {
> +    ht = HASH_AlgSHA1;
> +  } else if (algorithm == "sha-224") {
> +    ht = HASH_AlgSHA224;

I filed a bug.

@@ +186,5 @@
> +  PR_ASSERT(ho);
> +  if (!ho)
> +    return NS_ERROR_INVALID_ARG;
> +
> +  PR_ASSERT(ho->length >= 20);  // Double check

I would prefer to be safe.

@@ +195,5 @@
> +  SECStatus rv = HASH_HashBuf(ho->type, digest,
> +                              cert->derCert.data,
> +                              cert->derCert.len);
> +
> +  // This should not happen

done

@@ +212,5 @@
> +  std::string str("");
> +  char group[3];
> +  
> +  for (std::size_t i=0; i < size; i++) {
> +    PR_snprintf(group, 3, "%.2x", digest[i]);

Done/

@@ +233,5 @@
> +                                        size_t size,
> +                                        size_t *length) {
> +  size_t offset = 0;
> +  bool top_half = true;
> +  unsigned char val = 0;

Done.

@@ +246,5 @@
> +    if (top_half && (fp[i] == ':')) {
> +      continue;
> +    } else if ((fp[i] >= '0') && (fp[i] <= '9')) {
> +      val |= fp[i] - '0';
> +    } else if ((fp[i] >= 'a') && (fp[i] <= 'f')) {

Done.

::: media/mtransport/dtlsidentity.h
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Done.

@@ +46,5 @@
> +
> +#include "mozilla/RefPtr.h"
> +#include "nsISupportsImpl.h"
> +
> +class DtlsIdentity : public mozilla::RefCounted<DtlsIdentity> {

Done.

::: media/mtransport/logging.h
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: [e-mail]

Let's do it in a followup.

@@ +28,5 @@
> +  static LogCtx mlog_ctx(n)
> +
> +#define MLOG(level, b) \
> +  do { if (mlog_ctx.module()) {                            \
> +          std::stringstream str;                           \

Approved by Jesup for now.

::: media/mtransport/nr_socket_prsock.cpp
@@ +183,5 @@
> +  PRNetAddr *naddr) 
> +  {
> +    int _status;
> +    
> +    memset(naddr, 0, sizeof(PRNetAddr));

Done.

@@ +213,5 @@
> +        memcpy(naddr->ipv6.ip._S6_un._S6_u8,
> +               &addr->u.addr6.sin6_addr.__u6_addr.__u6_addr8, 16);
> +#endif
> +#else
> +        // TODO([e-mail]): make IPv6 work

Done.

@@ +307,5 @@
> +      ABORT(r);
> +  }
> +
> +  // Finally, register with the STS
> +  rv = stservice_->AttachSocket(fd_, this);

Unfortunately gSocketThread is not available to link into my unit test harness..... Gr...

AttachSocket has its own assert. Is that enough?

@@ +319,5 @@
> +  return(_status);
> +}
> +
> +int NrSocket::sendto(const void *msg, size_t len,
> +                     int flags, nr_transport_addr *to) {

Done.

@@ +331,5 @@
> +  if(fd_==NULL)
> +    ABORT(R_EOD);
> +
> +  // TODO([e-mail]): Convert flags?
> +  status = PR_SendTo(fd_, msg, len, flags, &naddr, PR_INTERVAL_NO_WAIT);

Done.

@@ +373,5 @@
> +}
> +
> +// Close the socket so that the STS will detach and then kill it
> +void NrSocket::close() {
> +  mCondition = NS_BASE_STREAM_CLOSED;

My understnading was that this was safe to do on any thread, actually, because worst case the STS would pick it up the next time through.

@@ +398,5 @@
> +};
> +
> +
> +int nr_socket_local_create(nr_transport_addr *addr, nr_socket **sockp) {
> +  NrSocket *sock = new NrSocket();

I am normally a big fan of smart pointers, but in this case I think the explicit stuff is clearer.

::: media/mtransport/nr_socket_prsock.h
@@ +37,5 @@
> +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +*/
> +
> +
> +// Implementation of nICEr/nr_socket that is tied to the Firefox STS

Done.

@@ +65,5 @@
> +  virtual ~NrSocket() {
> +    PR_Close(fd_);
> +  }
> +
> +  // Implement nsASocket

I believe this is correct. You'll notice that this subclassed to nsASocketHandler. The result is that the dtor only fires when the STS has stopped paying attention.

@@ +73,5 @@
> +  // nsISupports methods
> +  NS_DECL_ISUPPORTS
> +
> +  // Implementations of the async_event APIs
> +  int async_wait(int how, NR_async_cb cb, void *cb_arg,

I think that in this case it's better to match the naming of the nr_socket APIs.

WRT the underscore_, Jesup and I agreed to defer the question of conversion from Google to Mozilla style.

@@ +95,5 @@
> +  void fire_callback(int how);
> +
> +  PRFileDesc *fd_;
> +  nr_transport_addr my_addr_;
> +  NR_async_cb cbs_[2];

Done.

::: media/mtransport/nricectx.cpp
@@ +188,5 @@
> +  // Streams which do not exist should never fail.
> +  PR_ASSERT(s);
> +
> +  ctx->SetState(ICE_CTX_FAILED);
> +  s -> SignalFailed(s);

Done

@@ +235,5 @@
> +    nr_crypto_vtbl = &nr_ice_crypto_nss_vtbl;
> +    initialized = true;
> +    
> +    // Set the priorites for candidate type preferences
> +    NR_reg_set_uchar((char *)"ice.pref.type.srv_rflx",100);

Reopened https://bugzilla.mozilla.org/show_bug.cgi?id=786313

@@ +268,5 @@
> +      NR_reg_set_uchar((char *)"ice.pref.interface.virbr0", 233);
> +      NR_reg_set_uchar((char *)"ice.pref.interface.wlan0", 232);
> +    }
> +    
> +    NR_reg_set_string((char *)"ice.stun.server.0.addr", (char *)"216.93.246.14");

https://bugzilla.mozilla.org/show_bug.cgi?id=786236

::: media/mtransport/objs.mk
@@ +45,5 @@
> +  transportlayerloopback.cpp \
> +  transportlayerprsock.cpp \
> +  $(NULL)
> +
> +MTRANSPORT_CPPSRCS = $(addprefix $(topsrcdir)/media/mtransport/, $(MTRANSPORT_LCPPSRCS))
\ No newline at end of file

\ No newline at end of file

\ No newline at end of file

\ No newline at end of file

\ No newline at end of file

You actually need both of these variables elsewhere.

::: media/mtransport/test/Makefile.in
@@ +85,5 @@
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/plugin \
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/event \
> + $(NULL)
> +
> +# SCTP DEFINES

Done.

@@ +146,5 @@
> +  nrappkit_unittest.cpp \
> +  sockettransportservice_unittest.cpp \
> +  transport_unittests.cpp \
> +  runnable_utils_unittest.cpp \
> +#  sctp_unittest.cpp \

It just depends on SCTP which hasn't landed on my copy of m-c.

::: media/mtransport/test/sctp_unittest.cpp
@@ +25,5 @@
> +#include "mtransport_test_utils.h"
> +#include "runnable_utils.h"
> +
> +// XXX FIX
> +#include "../../../netwerk/sctp/src/usrsctp.h"

Done.

::: media/mtransport/transportflow.cpp
@@ +28,5 @@
> +    old_layer->SignalStateChange.disconnect(this);
> +    old_layer->SignalPacketReceived.disconnect(this);
> +  }
> +  layer->SignalStateChange.connect(this, &TransportFlow::StateChange);
> +  layer->SignalPacketReceived.connect(this, &TransportFlow::PacketReceived);

Added a comment that this is not thread safe.

::: media/mtransport/transportlayer.cpp
@@ +45,5 @@
> +  }
> +}
> +
> +const std::string& TransportLayer::flow_id() { 
> +    static std::string empty;

Done.

::: media/mtransport/transportlayer.h
@@ +7,5 @@
> +#ifndef transportlayer_h__
> +#define transportlayer_h__
> +
> +// Pull in sigslot from libjingle. If we remove libjingle, we will
> +// need to either bring that in separately or refactor this code

Removed.

@@ +31,5 @@
> +  virtual const std::string id() { return name; } \
> +  static std::string ID() { return name; }
> +
> +// Abstract base class for network transport layers.
> +class TransportLayer : public sigslot::has_slots<> {

In the README.

::: media/mtransport/transportlayerdtls.cpp
@@ +5,5 @@
> +// Original author: [e-mail]
> +
> +#include <queue>
> +
> +

Comment added.

@@ +48,5 @@
> +  ~Packet() {
> +    delete data_;
> +  }
> +  void Assign(const void *data, PRInt32 len) {
> +    data_ = new unsigned char[len];

These can't practically be bigger than 16k or so, so I think this is OK fo rnow.

@@ +59,5 @@
> +  PRInt32 offset_;
> +};
> +
> +void NSPRHelper::PacketReceived(const void *data, PRInt32 len) {
> +  input_.push(new Packet());

I didn't want to use a constructor here because it made packet require some sort of assignment/copy construction and that seemed like more trouble than it was worth.

@@ +117,5 @@
> +}
> +
> +static PRInt32 TransportLayerAvailable(PRFileDesc *f) {
> +  UNIMPLEMENTED;
> +  return -1;

Done.

@@ +189,5 @@
> +// Note: this is always nonblocking and assumes a zero timeout.
> +// This function does not support peek.
> +static PRInt32 TransportLayerRecv(PRFileDesc *f, void *buf, PRInt32 amount,
> +                   PRIntn flags, PRIntervalTime to) {
> +  PR_ASSERT(flags == 0);

Done.

@@ +191,5 @@
> +static PRInt32 TransportLayerRecv(PRFileDesc *f, void *buf, PRInt32 amount,
> +                   PRIntn flags, PRIntervalTime to) {
> +  PR_ASSERT(flags == 0);
> +
> +  if (flags != 0) {

Done.

@@ +200,5 @@
> +  return TransportLayerRead(f, buf, amount);
> +}
> +
> +// Note: this is always nonblocking and assumes a zero timeout.
> +// This function does not support peek.

Done.

@@ +204,5 @@
> +// This function does not support peek.
> +static PRInt32 TransportLayerSend(PRFileDesc *f, const void *buf, PRInt32 amount,
> +                   PRIntn flags, PRIntervalTime to) {
> +  PRInt32 written = TransportLayerWrite(f, buf, amount);
> +

Done.

@@ +209,5 @@
> +  return written;
> +}
> +
> +static PRInt32 TransportLayerRecvfrom(PRFileDesc *f, void *buf, PRInt32 amount,
> +                       PRIntn flags, PRNetAddr *addr, PRIntervalTime to) {

File reindented.

@@ +242,5 @@
> +
> +static PRStatus TransportLayerGetpeername(PRFileDesc *f, PRNetAddr *addr) {
> +  // TODO([e-mail]): Modify to return unique names for each channel
> +  // somehow, as opposed to always the same static address. The current
> +  // implementation messes up the session cache, which is why it's off

I added a bug.https://bugzilla.mozilla.org/show_bug.cgi?id=797422

@@ +301,5 @@
> +  UNIMPLEMENTED;
> +  return -1;
> +}
> +
> +static const struct PRIOMethods nss_methods = {

Done.

@@ +340,5 @@
> +  TransportLayerReserved,
> +  TransportLayerReserved
> +};
> +
> +TransportLayerDtls::~TransportLayerDtls() {

I think it's fine/

@@ +354,5 @@
> +  // Get the transport service as an event target
> +  nsresult rv;
> +  target_ = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
> +
> +  if (!NS_SUCCEEDED(rv)) {

Done.

@@ +360,5 @@
> +    return rv;
> +  }
> +
> +  timer_ = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
> +  if (!NS_SUCCEEDED(rv)) {

Done.

@@ +393,5 @@
> +                                          const unsigned char *digest_value,
> +                                          size_t digest_len) {
> +  // Defensive programming
> +  if (verification_mode_ != VERIFY_UNSET &&
> +    verification_mode_ != VERIFY_DIGEST)

Done.

@@ +411,5 @@
> +  return NS_OK;
> +}
> +
> +// TODO([e-mail]): make sure this is called from STS. Otherwise
> +// we have thread safety issues

Filed bug: https://bugzilla.mozilla.org/show_bug.cgi?id=797423

@@ +419,5 @@
> +  if (!downward_) {
> +    MLOG(PR_LOG_ERROR, "DTLS layer with nothing below. This is useless");
> +    return false;
> +  }
> +  helper_ = new NSPRHelper(downward_);

Done.

@@ +487,5 @@
> +  }
> +
> +  if (mode_ != DGRAM) {
> +    // Disable SSLv3
> +    rv = SSL_OptionSet(ssl_fd, SSL_ENABLE_SSL3, PR_FALSE);

Done.

@@ +559,5 @@
> +      MLOG(PR_LOG_ERROR, LAYER_INFO << "State change of lower layer to INIT forbidden");
> +      SetState(ERROR);
> +      break;
> +    case CONNECTING:
> +      break;

It doesn't change our behavior. I added a log.

@@ +644,5 @@
> +  }
> +
> +  // Now try a recv if we're open, since there might be data left
> +  if (state_ == OPEN) {
> +    unsigned char buf[2000];

DTLS has some PMTU discovery but you can't depend on it for this.

Jesup: I have filed a bug.
https://bugzilla.mozilla.org/show_bug.cgi?id=797432

@@ +679,5 @@
> +  PRInt32 rv = PR_Send(ssl_fd_, data, len, 0, PR_INTERVAL_NO_WAIT);
> +
> +  if (rv > 0) {
> +    // We have data
> +    MLOG(PR_LOG_DEBUG, LAYER_INFO << "Wrote " << rv << " bytes to NSS");

Done.

@@ +710,5 @@
> +
> +  TransportLayerDtls *stream = reinterpret_cast<TransportLayerDtls *>(arg);
> +
> +  if (!stream->identity_) {
> +    MLOG(PR_LOG_ERROR, "No identity available");

Done.

@@ +721,5 @@
> +  }
> +
> +  *pRetKey = SECKEY_CopyPrivateKey(stream->identity_->privkey());
> +  if (!pRetKey) {
> +    CERT_DestroyCertificate(*pRetCert);

Done.

@@ +723,5 @@
> +  *pRetKey = SECKEY_CopyPrivateKey(stream->identity_->privkey());
> +  if (!pRetKey) {
> +    CERT_DestroyCertificate(*pRetCert);
> +    return SECFailure;
> +  }

Done.

@@ +743,5 @@
> +  }
> +
> +  return NS_OK;
> +}
> +

That would be very useful.

@@ +752,5 @@
> +                                                  unsigned int outlen) {
> +  SECStatus rv = SSL_ExportKeyingMaterial(ssl_fd_,
> +                                          label.c_str(),
> +                                          label.size(),
> +                                          use_context ? PR_TRUE : PR_FALSE,

done.

@@ +775,5 @@
> +
> +  return stream->AuthCertificateHook(fd, checksig, isServer);
> +}
> +
> +SECStatus TransportLayerDtls::CheckDigest(mozilla::RefPtr<VerificationDigest> digest,

Changed to const &

@@ +790,5 @@
> +                                       &computed_digest_len);
> +  if (!NS_SUCCEEDED(res)) {
> +    MLOG(PR_LOG_ERROR, "Could not compute peer fingerprint for digest " <<
> +         digest->algorithm_);
> +    // Go to end

Done.

@@ +814,5 @@
> +SECStatus TransportLayerDtls::AuthCertificateHook(PRFileDesc *fd,
> +                                                  PRBool checksig,
> +                                                  PRBool isServer) {
> +  CERTCertificate *peer_cert = NULL;
> +  peer_cert = SSL_PeerCertificate(fd);

Done.

@@ +817,5 @@
> +  CERTCertificate *peer_cert = NULL;
> +  peer_cert = SSL_PeerCertificate(fd);
> +
> +  // We are not set up to take this being called multiple
> +  // times. Change this if we ever add renegotiation.

Done.

@@ +819,5 @@
> +
> +  // We are not set up to take this being called multiple
> +  // times. Change this if we ever add renegotiation.
> +  PR_ASSERT(!auth_hook_called_);
> +  if (auth_hook_called_)

Done.

@@ +820,5 @@
> +  // We are not set up to take this being called multiple
> +  // times. Change this if we ever add renegotiation.
> +  PR_ASSERT(!auth_hook_called_);
> +  if (auth_hook_called_)
> +    return SECFailure;

Done.

@@ +828,5 @@
> +  PR_ASSERT(peer_cert_ == NULL);
> +
> +  switch (verification_mode_) {
> +    case VERIFY_UNSET:
> +      // Jump to error exit

Rewrote.

@@ +835,5 @@
> +      peer_cert_ = peer_cert;
> +      return SECSuccess;
> +
> +    case VERIFY_DIGEST: {
> +      CERTCertificate *peer_cert =

Done.

@@ +841,5 @@
> +
> +      PR_ASSERT(digests_.size() != 0);
> +      // Check all the provided digests
> +
> +      SECStatus res= SECFailure;

Done.

@@ +855,5 @@
> +        // Matches all digests, we are good to go
> +        peer_cert_ = peer_cert;
> +        return SECSuccess;
> +      }
> +    }

Done.

@@ +860,5 @@
> +  }
> +
> +  if (peer_cert) {
> +    CERT_DestroyCertificate(peer_cert);
> +  }

It's called in the checking fxns.

::: media/mtransport/transportlayerdtls.h
@@ +26,5 @@
> +#include "transportlayer.h"
> +
> +struct Packet;
> +
> +class NSPRHelper {

I don't think this is the right plan here... This really isn't like a SocketInfo. It's a bridge to an NSPR underlying socket. I've renamed it to TransportLayerNSPRAdapter.

The threading model documentation is in the README.

@@ +43,5 @@
> +  TransportLayer *output_;
> +  std::queue<Packet *> input_;
> +};
> +
> +class TransportLayerDtls : public TransportLayer {

1. Per Jesup, I am intending to defer this sort of style renaming.
2. We are going to handle the shutdown in PEerConnection.

@@ +68,5 @@
> +  // DTLS-specific operations
> +  void SetRole(Role role) { role_ = role;}
> +  Role role() { return role_; }
> +
> +  void SetIdentity(mozilla::RefPtr<DtlsIdentity> identity) {

Because I don't want the raw pointer to escape. Changed this to be a const &.

@@ +153,5 @@
> +  Verification verification_mode_;
> +  std::vector<mozilla::RefPtr<VerificationDigest> > digests_;
> +
> +  PRFileDesc *pr_fd_;
> +  PRFileDesc *ssl_fd_;

Currently we are PR_Close()ing ssl_fd_. My understanding was that this would transitively close pr_fd_. When we have ScopedPRFileDesc, we can obviously just convert ssl_fd_

@@ +155,5 @@
> +
> +  PRFileDesc *pr_fd_;
> +  PRFileDesc *ssl_fd_;
> +  mozilla::ScopedDeletePtr<NSPRHelper> helper_;
> +  CERTCertificate *peer_cert_;

Done.

::: media/mtransport/transportlayerice.cpp
@@ +77,5 @@
> +#ifdef ERROR
> +#undef ERROR
> +#endif
> +
> +MLOG_INIT("mtransport");

Done.

::: media/mtransport/transportlayerloopback.h
@@ +78,5 @@
> +  class QueuedPacket {
> +   public:
> +    QueuedPacket() : data_(NULL), len_(0) {}
> +    ~QueuedPacket() {
> +      delete data_;

Done.

Comment 62

[Eric Rescorla (:ekr)]

The uploaded patch should address nearly just the technical issues. The two exceptions are:

(1) Jesup had raised a question about reference management for nr_timer.cpp
(2) There are a few questions bsmith had raised about freeing stuff that either need explicit frees or scoped pointers.

There are also the following mostly-mechanical transformations that I need to make:

1. Trailing whitespace.
2. PR_ASSERT -> MOZ_ASSERT
3. MLOG_INIT -> MOZ_LOG_MODULE
   MLOG -> MOZ_LOG
4. NULL -> nullptr
5. Namespace mozilla

Finally, there are some things I would like to defer:

1. Add MOZ_OVERRIDE to overrides.
2. More documentation for TransportLayer and TransportLayerFlow (though I added some in README).

Comment 63

[(not currently active) Ted Mielczarek]

Comment on attachment 667584 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

::: media/mtransport/build/Makefile.in
@@ +40,5 @@
> +  $(NULL)
> +
> +CPPSRCS = \
> +	$(MTRANSPORT_LCPPSRCS) \
> +	$(NULL)

2-spaces, not tabs.

::: media/mtransport/objs.mk
@@ +34,5 @@
> +ifeq ($(OS_ARCH), WINNT)
> +LOCAL_INCLUDES +=  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/win32/include
> +endif
> +
> +MTRANSPORT_LCPPSRCS = \

Use := for immediate assignments.

::: media/mtransport/runnable_utils.py
@@ +1,1 @@
> +MAX_ARGS = 10

Stick a MPL2 license boilerplate in this file.

::: media/mtransport/standalone/Makefile.in
@@ +35,5 @@
> +  ../transportlayerloopback.h \
> +  ../transportlayerprsock.h \
> +  $(NULL)
> +
> +CPPSRCS = \

:=

@@ +37,5 @@
> +  $(NULL)
> +
> +CPPSRCS = \
> +	$(MTRANSPORT_LCPPSRCS) \
> +	$(NULL)

2-spaces

::: media/mtransport/test/Makefile.in
@@ +29,5 @@
> +  -I$(topsrcdir)/media/webrtc/trunk/testing/gtest/include/ \
> +  -I$(topsrcdir)/media/mtransport/ \
> +  -I$(topsrcdir)/netwerk/sctp/src/ \
> +
> + $(NULL)

What's with the extra blank line in here?

@@ +67,5 @@
> +DEFINES += -D__Userspace_os_$(OS_TARGET)=1
> +endif
> +endif
> +endif
> +endif

Not a big fan of the hugely-nested if/else here. If we don't think the fallback is going to work then maybe you could just write this as
ifeq (...)
endif
ifeq (...)
endif

@@ +81,5 @@
> +LOCAL_INCLUDES +=  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/linux/include
> +endif
> +
> +ifneq ($(OS_TARGET),WINNT)
> +CPP_UNIT_TESTS = \

:=

Comment 64

[Eric Rescorla (:ekr)]

Comment on attachment 660321 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

::: media/mtransport/nr_timer.cpp
@@ +50,5 @@
> +                       int l, void **handle) {
> +  nsresult rv;
> + 
> +  // TODO([e-mail]): See if this is too expensive and if so cache
> +  nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);

It looks like it is standard practice to just grab a timer this way not to recycle. See, for instance, nsDocShell::SendPing()

@@ +59,5 @@
> +
> +  timer->InitWithCallback(new nrappkitTimerCallback(cb, arg), timeout,
> +                           nsITimer::TYPE_ONE_SHOT);
> +  
> +  timer->AddRef();

It's right. Added a comment.

@@ +74,5 @@
> +
> +int NR_async_timer_cancel(void *handle) {
> +  nsITimer *timer = static_cast<nsITimer *>(handle);
> +  
> +  timer->Cancel();

Added a release.

Comment 65

[Eric Rescorla (:ekr)]

https://tbpl.mozilla.org/?tree=Try&rev=4f99b3a2c0e9

Comment 66

[Eric Rescorla (:ekr)]

Attachment: Patch 1: imported patch nrappkit.patch

Comment 67

[Eric Rescorla (:ekr)]

Attachment: Add sigslot

Comment 68

[Eric Rescorla (:ekr)]

Attachment: Patch 4: Generic media transport subsystem with modules for ICE and DTLS

Comment 69

[Eric Rescorla (:ekr)]

Comment on attachment 668080 [details] [diff] [review]
Patch 1: imported patch nrappkit.patch

This includes some small  cleanup of platform-dependent code.

Comment 70

[Eric Rescorla (:ekr)]

Comment on attachment 668080 [details] [diff] [review]
Patch 1: imported patch nrappkit.patch

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

Jesup, these are just some trivial changes...

Comment 71

[Eric Rescorla (:ekr)]

Comment on attachment 668083 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

This includes the mechanical changes to whitespace, PRtypes, namespaces, etc. I also updated the code to use scoped pointers in dtlsidentity.cpp

Comment 72

[Eric Rescorla (:ekr)]

Note: patch 4 compiles on Mac, compiles but does not link on Linux (pending Webrtc.org uplift) and has some unknown compile error on Win. I will debug the Win problem this afternoon/eve unless Jesup gets to it first.

Comment 73

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 668083 [details] [diff] [review]
Patch 4: Generic media transport subsystem with modules for ICE and DTLS

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

Fix the nullptr/$(NULL) makefile thing and r=jesup

I think you do want to export sigslot.h or we'll have to do a separate patch for that.

::: media/mtransport/build/Makefile.in
@@ +24,5 @@
> +
> +EXPORTS_NAMESPACES = mtransport
> +
> +EXPORTS_mtransport = \
> +  ../dtlsidentity.h \

I think you want to export sigslot.h as we are in alder

@@ +36,5 @@
> +  ../transportlayerloopback.h \
> +  ../transportlayerprsock.h \
> +  ../m_cpp_utils.h \
> +  ../runnable_utils.h \
> +  $(nullptr)

$(NULL)

Dangers of scripts...

@@ +40,5 @@
> +  $(nullptr)
> +
> +CPPSRCS = \
> +	$(MTRANSPORT_LCPPSRCS) \
> +	$(nullptr)

Ditto

::: media/mtransport/dtlsidentity.cpp
@@ +20,5 @@
> +namespace mozilla {
> +
> +MOZ_MTLOG_MODULE("mtransport");
> +
> +// Helper class to avoid having a pile of if (!nullptr) statements at

NULL, but not important

::: media/mtransport/nr_timer.cpp
@@ +77,5 @@
> +  // We need an AddRef here to keep the timer alive, per the spec.
> +  timer->AddRef();
> +
> +  if (handle)
> +    *handle = timer.get();

is .get() needed here?  I think not, though not a big deal

::: media/mtransport/nricemediastream.h
@@ +43,5 @@
> +class NrIceMediaStream {
> + public:
> +  static RefPtr<NrIceMediaStream> Create(NrIceCtx *ctx,
> +                                           const std::string& name,
> +                                           int components);

indentation due to renaming

::: media/mtransport/objs.mk
@@ +20,5 @@
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/registry \
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/stats \
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/plugin \
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/event \
> + $(nullptr)

$(NULL)

@@ +25,5 @@
> +
> +ifeq ($(OS_ARCH), Darwin)
> +LOCAL_INCLUDES += \
> +  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/darwin/include \
> +  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/generic/include

These should end with $(NULL)

@@ +32,5 @@
> +
> +ifeq ($(OS_ARCH), Linux)
> +LOCAL_INCLUDES += \
> +  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/linux/include \
> +  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/generic/include

These should end with $(NULL)

@@ +39,5 @@
> +
> +ifeq ($(OS_ARCH), WINNT)
> +LOCAL_INCLUDES += \
> +  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/win32/include \
> +  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/generic/include

These should end with $(NULL)

@@ +56,5 @@
> +  transportlayerdtls.cpp \
> +  transportlayerlog.cpp \
> +  transportlayerloopback.cpp \
> +  transportlayerprsock.cpp \
> +  $(nullptr)

$(NULL)

::: media/mtransport/standalone/Makefile.in
@@ +33,5 @@
> +  ../transportlayerice.h \
> +  ../transportlayerlog.h \
> +  ../transportlayerloopback.h \
> +  ../transportlayerprsock.h \
> +  $(nullptr)

$(NULL)

@@ +37,5 @@
> +  $(nullptr)
> +
> +CPPSRCS = \
> +	$(MTRANSPORT_LCPPSRCS) \
> +	$(nullptr)

$(NULL)

@@ +44,5 @@
> +# Make a copy into the local directory for dual compilation
> +export:: $(MTRANSPORT_CPPSRCS)
> +	$(INSTALL) $^ .
> +
> +

Did you mean to remove the stun stuff?  (and -DNOMINMAX)?  Quite possibly, but checking, since it's in Alder

::: media/mtransport/test/Makefile.in
@@ +20,5 @@
> +  $(DEPTH)/media/mtransport/standalone/$(LIB_PREFIX)mtransport_s.$(LIB_SUFFIX) \
> +  $(DEPTH)/media/mtransport/third_party/nICEr/nicer_nicer/$(LIB_PREFIX)nicer.$(LIB_SUFFIX) \
> +  $(DEPTH)/media/mtransport/third_party/nrappkit/nrappkit_nrappkit/$(LIB_PREFIX)nrappkit.$(LIB_SUFFIX) \
> +  $(DEPTH)/media/webrtc/trunk/testing/gtest_gtest/$(LIB_PREFIX)gtest.$(LIB_SUFFIX) \
> +  $(nullptr)

$(NULL)

@@ +31,5 @@
> +  -I. \
> +  -I$(topsrcdir)/media/webrtc/trunk/testing/gtest/include/ \
> +  -I$(topsrcdir)/media/mtransport/ \
> +  -I$(topsrcdir)/netwerk/sctp/src/ \
> +

this blank line shouldn't be here

@@ +32,5 @@
> +  -I$(topsrcdir)/media/webrtc/trunk/testing/gtest/include/ \
> +  -I$(topsrcdir)/media/mtransport/ \
> +  -I$(topsrcdir)/netwerk/sctp/src/ \
> +
> + $(nullptr)

$(NULL) and indent

@@ +50,5 @@
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/registry \
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/stats \
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/plugin \
> + -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/event \
> + $(nullptr)

$(NULL)
indent 2 please (whole block)

@@ +76,5 @@
> +ifeq ($(OS_ARCH), Darwin)
> +LOCAL_INCLUDES +=  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/darwin/include
> +DEFINES += \
> +  -DGTEST_USE_OWN_TR1_TUPLE=1 \
> +  $(nullptr)

$(NULL)

@@ +90,5 @@
> +  nrappkit_unittest.cpp \
> +  sockettransportservice_unittest.cpp \
> +  transport_unittests.cpp \
> +  runnable_utils_unittest.cpp \
> +  $(nullptr)

$(NULL)

::: media/mtransport/test/ice_unittest.cpp
@@ +63,5 @@
> +
> +    mozilla::RefPtr<NrIceMediaStream> stream =
> +        ice_ctx_->CreateStream(static_cast<char *>(name), components);
> +
> +    ASSERT_TRUE(stream != nullptr);

ASSERT_TRUE(stream)  (minor style point)

::: media/mtransport/test/transport_unittests.cpp
@@ +36,5 @@
> +#define GTEST_HAS_RTTI 0
> +#include "gtest/gtest.h"
> +#include "gtest_utils.h"
> +
> +  using namespace mozilla;

left-justify

::: media/mtransport/transportlayerdtls.cpp
@@ +68,5 @@
> +// because this is only useful for NSPR.
> +struct Packet {
> +  Packet() : data_(nullptr), len_(0), offset_(0) {}
> +
> +  void Assign(const void *data, int32_t len) {

If we're using NSPR signatures, should this be PRInt32, etc and be an exception?  Perhaps not, and in reality it doesn't matter.

@@ +74,5 @@
> +    memcpy(data_, data, len);
> +    len_ = len;
> +  }
> +
> +  ScopedDeleteArray<uint8_t> data_;

Didn't realize you could have them in structures.  Handy

@@ +158,5 @@
> +  return -1;
> +}
> +
> +static int64_t TransportLayerSeek64(PRFileDesc *f, int64_t offset,
> +                                       PRSeekWhence how) {

indents messed up by auto changes

Comment 74

[Randell Jesup [:jesup] (needinfo me)]

Attachment: interdiffs for patch 4 against alder (effectively against the previous patch)

So it's easier to see what changed... (a few things might have been changed before teh last patch; this is against Alder.)

Comment 75

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Interdiffs for media/mtransport/*dtls* (to make bsmith's life easier)

Comment 76

[Eric Rescorla (:ekr)]

Attachment: Patch 4: Generic media transport subsystem with modules for ICE and DTLS

Comment 77

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Review comments as patch

Here are the changes I think need to be made, in patch form.

Comment 78

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 668666 [details] [diff] [review]
Review comments as patch

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

::: media/mtransport/dtlsidentity.cpp
@@ +21,5 @@
>    if (cert_)
>      CERT_DestroyCertificate(cert_);
>  }
>  
> +TemporaryRef<DtlsIdentity> DtlsIdentity::Generate() {

TemporaryRef for return values.

@@ +23,5 @@
>  }
>  
> +TemporaryRef<DtlsIdentity> DtlsIdentity::Generate() {
> +
> +  ScopedPK11SlotInfo slot(PK11_GetInternalSlot());

I combined the two Generate() functions together so they could share the slot variable. I made these functions use GenerateRandomOnSlot() to avoid locking issues noted by wtc.

@@ +64,3 @@
>    if (private_key == nullptr)
>      return nullptr;
> +  public_key = pubkey;

replaced X.reset(Y) with X = Y;

@@ +81,5 @@
>    // This is a sort of arbitrary range designed to be valid
>    // now with some slack in case the other side expects
>    // some before expiry.
> +  //
> +  // Note: explicit casts necessary to avoid 

fixed this trailing whitespace already.

@@ +82,5 @@
>    // now with some slack in case the other side expects
>    // some before expiry.
> +  //
> +  // Note: explicit casts necessary to avoid 
> +  //       warning C4307: '*' : integral constant overflow

seems like we were overflowing at least on Windows.

@@ +144,5 @@
>      return nullptr;
>    }
>    certificate->derCert = *signedCert;
>  
> +  return new DtlsIdentity(private_key.forget(), certificate.forget());

Use implicit conversion to smart pointer as described in the RefPtr.h examples, to make things easier to read.

::: media/mtransport/nricectx.cpp
@@ +96,2 @@
>  
> +  SECStatus rv = PK11_GenerateRandomOnSlot(slot, buf, len);

Same lock contention issue.

::: media/mtransport/nricemediastream.cpp
@@ +165,5 @@
>           << name_ << "'");
>      return;
>    }
>  
> +  for (int i=0; i<attrct; i++) {

signed/unsigned comparison warnings.

::: media/mtransport/third_party/nICEr/nicer.gyp
@@ +21,5 @@
>  	      '../nrappkit/src/share',
>  	      '../nrappkit/src/stats',
>  	      '../nrappkit/src/util',
>  	      '../nrappkit/src/util/libekr',
> + 	      '../nrappkit/src/port/generic/include',

needed to build on windows.

::: media/mtransport/transportlayerdtls.cpp
@@ +28,5 @@
>  namespace mozilla {
>  
>  MOZ_MTLOG_MODULE("mtransport");
>  
> +static PRDescIdentity transport_layer_identity = PR_INVALID_IO_LAYER;

The old name made me confused about whether we were talking about the built-in NSPR layer or our new layer.

@@ +200,5 @@
>  // This function does not support peek.
>  static int32_t TransportLayerRecv(PRFileDesc *f, void *buf, int32_t amount,
>                                    int32_t flags, PRIntervalTime to) {
>    MOZ_ASSERT(flags == 0);
> +  if (flags != 0) {

Removed checks of "to" and misleading comment.

@@ +349,5 @@
>  };
>  
>  TransportLayerDtls::~TransportLayerDtls() {
> +  if (timer_) {
> +    timer_->Cancel();

timer_ may be null here.

@@ +442,3 @@
>    }
> +
> +  ScopedPRFileDesc pr_fd(PR_CreateIOLayerStub(transport_layer_identity,

pr_fd_ did not need to be a member function.

@@ +450,2 @@
>  
> +  ScopedPRFileDesc ssl_fd;

ssl_fd previous leaked.

@@ +561,5 @@
>      MOZ_MTLOG(PR_LOG_ERROR, "Couldn't set certificate validation hook");
>      return false;
>    }
>  
> +  ssl_fd_ = ssl_fd.forget();

ssl_fd_ should be set before we call SSL_ResetHandshake, I think.

Comment 79

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 668666 [details] [diff] [review]
Review comments as patch

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

::: media/mtransport/transportlayerdtls.cpp
@@ +436,5 @@
>      MOZ_MTLOG(PR_LOG_ERROR, "Can't start DTLS without specifying a verification mode");
>      return false;
>    }
>  
> +  if (transport_layer_identity == PR_INVALID_IO_LAYER) {

Also, notice that there was a significant typo in the previous code (an extraneous "!").

Comment 80

[Eric Rescorla (:ekr)]

Comment on attachment 668666 [details] [diff] [review]
Review comments as patch

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

lgtm with two comments below.

Also nicer.gyp is part of patch 2, not patch 4.

::: media/mtransport/transportlayerdtls.cpp
@@ +561,5 @@
>      MOZ_MTLOG(PR_LOG_ERROR, "Couldn't set certificate validation hook");
>      return false;
>    }
>  
> +  ssl_fd_ = ssl_fd.forget();

This seems OK, but then let's reset ssl_fd_ to nullptr if it fails.

::: media/mtransport/transportlayerdtls.h
@@ +54,1 @@
>        ssl_fd_(nullptr),

If you're going to remove these initializers, remove this one as well.

Comment 81

[Eric Rescorla (:ekr)]

Attachment: Fix bsmith linkage errors

Comment 82

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Review comments as patch [v2]

Comment 83

[Eric Rescorla (:ekr)]

Attachment: Fix bsmith linkage errors

Comment 84

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 668843 [details] [diff] [review]
Fix bsmith linkage errors

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

Parts of this will conflict with the patch I just posted because I also fixed those issues.

::: media/mtransport/transportlayerdtls.cpp
@@ +568,5 @@
>    if (rv != SECSuccess) {
>      MOZ_MTLOG(PR_LOG_ERROR, "Couldn't reset handshake");
>      return false;
>    }
> +  ssl_fd_ = ssl_fd.forget();

add a blank line before this line.

@@ +921,5 @@
>          }
>        }
>        break;
> +    default:
> +      MOZ_CRASH();  // Can't happen

This is better than what I did.

@@ +926,2 @@
>    }
> +    

Extra whitespace here.

Comment 85

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8fac69129b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/da39ec3b9778
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5246d1cd364
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbce2911566d

Comment 86

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cbce2911566d
https://hg.mozilla.org/mozilla-central/rev/b5246d1cd364
https://hg.mozilla.org/mozilla-central/rev/da39ec3b9778
https://hg.mozilla.org/mozilla-central/rev/a8fac69129b0