Closed Bug 996238 Opened 10 years ago Closed 9 years ago

ALPN identifiers for WebRTC

Categories

(Core :: WebRTC: Networking, defect)

Product:
Core
Component:
WebRTC: Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: mt, Assigned: mt)

References

(
URL
)

Details

Attachments

(5 files, 7 obsolete files)

MozReview Request: Bug 996238 - Force handshake failure if ALPN fails, r=ekr
39 bytes, text/x-review-board-request
mt
: review+
Details
MozReview Request: Bug 996238 - Adding test for asymmetric media isolation across PC, r=ekr
39 bytes, text/x-review-board-request
mt
: review+
Details
MozReview Request: Bug 996238 - Move more of the test code to promises, r=jib
39 bytes, text/x-review-board-request
mt
: review+
Details
MozReview Request: Bug 996238 - Reformat gtest_utils.h, r=ekr
39 bytes, text/x-review-board-request
mt
: review+
Details
MozReview Request: Bug 996238 - ALPN support for WebRTC, r=ekr
39 bytes, text/x-review-board-request
mt
: review+
Details 
Use ALPN as a basis for stream isolation in WebRTC.  See URL for proposed names.
Depends on: 996250
Depends on: 966066
I'll put this up on reviewboard for you if I can somehow manage to get it working.  Let me get a few more review requests out there first.
Assignee: nobody → martin.thomson
Status: NEW → ASSIGNED
Attachment #8412885 - Flags: review?(ekr)
Comment on attachment 8412885 [details] [diff] [review]
0001-Bug-996238-ALPN-identifiers-for-WebRTC.patch

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

Based on your comments, this seems like it's not ready for review. Please re-r? if I misread.
Attachment #8412885 - Flags: review?(ekr)
Comment on attachment 8412885 [details] [diff] [review]
0001-Bug-996238-ALPN-identifiers-for-WebRTC.patch

Context error in previous comment, just ignore that.  I think that this is OK, though it might need a rebase before proceeding.
Attachment #8412885 - Flags: review?(ekr)
Comment on attachment 8412885 [details] [diff] [review]
0001-Bug-996238-ALPN-identifiers-for-WebRTC.patch

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

This feels like it needs another pass.

::: media/mtransport/test/gtest_utils.h
@@ +79,5 @@
>    do { \
> +    bool res; \
> +    WAIT_(expression, timeout, res); \
> +    EXPECT_TRUE(res); \
> +  } while(0)

LEt's revert these formatting changes.

::: media/mtransport/test/transport_unittests.cpp
@@ +457,5 @@
>      p2_->SetDtlsAllowAll();
>    }
>  
> +  void SetAlpn(std::string first = "a", std::string second = "a",
> +               int withDefaults = 3) {

The default args seem more confusing than helpfull here and below, especially since my intuition would be that SetAlpn("X") would set both, not set "X", "a"

::: media/mtransport/transportlayerdtls.cpp
@@ +387,5 @@
> +// Set the permitted ALPN identifiers.
> +// The default is here to allow for peers that don't want to negotiate ALPN
> +// in that case, the default string will be reported from GetNegotiatedAlpn().
> +// Setting the default to the empty string causes the transport layer to fail
> +// if ALPN is not negotiated.

This seems like a not-great semantic (helpful comment, though).

What would you think about having an alpnRequired flag?

@@ +397,5 @@
> +  // that is one octet of size followed by that many bytes of identifier
> +  size_t sz = 0;
> +  bool foundDefault = false;
> +  for (auto i = alpnAllowed.begin(); i != alpnAllowed.end(); ++i) {
> +    if (i->empty() || i->length() + sz > MAX_ALPN_LENGTH) {

Parens to make precedence clear here.

@@ +399,5 @@
> +  bool foundDefault = false;
> +  for (auto i = alpnAllowed.begin(); i != alpnAllowed.end(); ++i) {
> +    if (i->empty() || i->length() + sz > MAX_ALPN_LENGTH) {
> +      MOZ_MTLOG(ML_ERROR, "Invalid input to SetAlpn");
> +      return NS_ERROR_INVALID_ARG;

Suggest an assert here.

@@ +402,5 @@
> +      MOZ_MTLOG(ML_ERROR, "Invalid input to SetAlpn");
> +      return NS_ERROR_INVALID_ARG;
> +    }
> +    foundDefault = foundDefault || (*i == alpnDefault);
> +    sz += i->length() + 1;

I would suggest that we move the error check for the size being
too large to after the increment. That would be clearer.

@@ +614,4 @@
>      return false;
>    }
>  
> +  if (!alpnAllowed_.empty()) {

Should we be enabling ALPN if alpnAllowed() is empty?

@@ +617,5 @@
> +  if (!alpnAllowed_.empty()) {
> +    unsigned char buf[MAX_ALPN_LENGTH];
> +    size_t i = 0;
> +    for (auto tag = alpnAllowed_.begin();
> +         i < MAX_ALPN_LENGTH && tag != alpnAllowed_.end(); ++tag) {

Isn't the length being <= MAX_ALPN_LENGTH supposed to be an invariant?

If so, let's not make it a loop condition. Suggest either an assert or
if you are feeling paranoid a run-time check.

Actually, it seems like wouldn't it be easier to just lay out the buffer
on the first pass through?

@@ +618,5 @@
> +    unsigned char buf[MAX_ALPN_LENGTH];
> +    size_t i = 0;
> +    for (auto tag = alpnAllowed_.begin();
> +         i < MAX_ALPN_LENGTH && tag != alpnAllowed_.end(); ++tag) {
> +      buf[i++] = tag->length();

Should we should be checking somewhere that each individual length fits in a
byte.

I realize that this is an invariant because we check that MAX_ALPN_LENGTH < 255,
but that's not a protocol invariant and if we were to change the API to
allow longer lengths for the total field, then you would need an independent
check that each element is < 255.

@@ +619,5 @@
> +    size_t i = 0;
> +    for (auto tag = alpnAllowed_.begin();
> +         i < MAX_ALPN_LENGTH && tag != alpnAllowed_.end(); ++tag) {
> +      buf[i++] = tag->length();
> +      size_t incr = std::min(tag->length(), MAX_ALPN_LENGTH - i);

Truncation seems bad here. If you don't trust your logic above, let's have this be an error.

@@ +709,5 @@
>        return;
>      }
> +    if (!CheckAlpn()) {
> +      TL_SET_STATE(TS_ERROR);
> +      PR_Close(ssl_fd_);  // not an error at the TLS layer

Can you explain this comment?

@@ +767,5 @@
> +                                  &chosenAlpnLen, 255);
> +
> +  // NO_OVERLAP shouldn't happen, but it's good to double check sometimes
> +  if (rv != SECSuccess || alpnState == SSL_NEXT_PROTO_NO_OVERLAP) {
> +    MOZ_MTLOG(ML_ERROR, LAYER_INFO << "ALPN error");

Let's make these separate errors and log them separately because
SECFailure here seems like it's basically an internal error.

@@ +771,5 @@
> +    MOZ_MTLOG(ML_ERROR, LAYER_INFO << "ALPN error");
> +    return false;
> +  }
> +  if (alpnState == SSL_NEXT_PROTO_NO_SUPPORT) {
> +    MOZ_MTLOG(ML_NOTICE, LAYER_INFO << "ALPN required but not negotiated");

Why does this say "required"? It seems like if alpnDefault_ is non-empty
then this is cool.

@@ +783,5 @@
> +  MOZ_MTLOG(ML_NOTICE, LAYER_INFO << "Selected ALPN string: " << chosen);
> +  if (alpnAllowed_.find(chosen) == alpnAllowed_.end()) {
> +    MOZ_MTLOG(ML_ERROR, LAYER_INFO << "Bad ALPN string: " << chosen);
> +    for (auto i = alpnAllowed_.begin(); i != alpnAllowed_.end(); ++i) {
> +      MOZ_MTLOG(ML_INFO, LAYER_INFO << "Permitted ALPN: " << *i);

Shouldn't NSS be throwing an error here? Is this an invariant?

::: media/mtransport/transportlayerdtls.h
@@ +69,5 @@
>    void SetIdentity(const RefPtr<DtlsIdentity>& identity) {
>      identity_ = identity;
>    }
> +  nsresult SetAlpn(const std::set<std::string>& allowedAlpn,
> +                   const std::string& alpnDefault = "");

Let's get rid of the default arg. you don't seem to use it.
Attachment #8412885 - Flags: review?(ekr) → review-
Most of these are pretty easy to fix up.

(In reply to Eric Rescorla (:ekr) from comment #4)
> LEt's revert these formatting changes.

Expect a second patch.
 
> ::: media/mtransport/transportlayerdtls.cpp
> @@ +387,5 @@
> > +// Set the permitted ALPN identifiers.
> > +// The default is here to allow for peers that don't want to negotiate ALPN
> > +// in that case, the default string will be reported from GetNegotiatedAlpn().
> > +// Setting the default to the empty string causes the transport layer to fail
> > +// if ALPN is not negotiated.
> 
> This seems like a not-great semantic (helpful comment, though).
> 
> What would you think about having an alpnRequired flag?

Seemed redundant.  No default means that something is required in most contexts.

> @@ +399,5 @@
> > +  bool foundDefault = false;
> > +  for (auto i = alpnAllowed.begin(); i != alpnAllowed.end(); ++i) {
> > +    if (i->empty() || i->length() + sz > MAX_ALPN_LENGTH) {
> > +      MOZ_MTLOG(ML_ERROR, "Invalid input to SetAlpn");
> > +      return NS_ERROR_INVALID_ARG;
> 
> Suggest an assert here.

I'm deleting this bit in favour of assertions in the bit where the extension is built.

> @@ +614,4 @@
> >      return false;
> >    }
> >  
> > +  if (!alpnAllowed_.empty()) {
> 
> Should we be enabling ALPN if alpnAllowed() is empty?

You can enable ALPN, but if you don't set a value for the extension, then it doesn't get sent (or accepted).  Worth a comment?

> @@ +617,5 @@
> > +  if (!alpnAllowed_.empty()) {
> > +    unsigned char buf[MAX_ALPN_LENGTH];
> > +    size_t i = 0;
> > +    for (auto tag = alpnAllowed_.begin();
> > +         i < MAX_ALPN_LENGTH && tag != alpnAllowed_.end(); ++tag) {
> 
> Isn't the length being <= MAX_ALPN_LENGTH supposed to be an invariant?
> 
> If so, let's not make it a loop condition. Suggest either an assert or
> if you are feeling paranoid a run-time check.
> 
> Actually, it seems like wouldn't it be easier to just lay out the buffer
> on the first pass through?

I considered that, but the allocations suck.

> @@ +618,5 @@
> > +    unsigned char buf[MAX_ALPN_LENGTH];
> > +    size_t i = 0;
> > +    for (auto tag = alpnAllowed_.begin();
> > +         i < MAX_ALPN_LENGTH && tag != alpnAllowed_.end(); ++tag) {
> > +      buf[i++] = tag->length();
> 
> Should we should be checking somewhere that each individual length fits in a
> byte.

Will do, see below.
 
> @@ +619,5 @@
> > +    size_t i = 0;
> > +    for (auto tag = alpnAllowed_.begin();
> > +         i < MAX_ALPN_LENGTH && tag != alpnAllowed_.end(); ++tag) {
> > +      buf[i++] = tag->length();
> > +      size_t incr = std::min(tag->length(), MAX_ALPN_LENGTH - i);
> 
> Truncation seems bad here. If you don't trust your logic above, let's have
> this be an error.

So what I'm going to do is assert here and not check on SetAlpn.  That is not as good as I would have liked (because it defers errors a little), but it is easier and ensures that the ALPN info is accessible later.

> @@ +771,5 @@
> > +    MOZ_MTLOG(ML_ERROR, LAYER_INFO << "ALPN error");
> > +    return false;
> > +  }
> > +  if (alpnState == SSL_NEXT_PROTO_NO_SUPPORT) {
> > +    MOZ_MTLOG(ML_NOTICE, LAYER_INFO << "ALPN required but not negotiated");
> 
> Why does this say "required"? It seems like if alpnDefault_ is non-empty
> then this is cool.

Yeah, I'll make the logging properly conditional.

> @@ +783,5 @@
> > +  MOZ_MTLOG(ML_NOTICE, LAYER_INFO << "Selected ALPN string: " << chosen);
> > +  if (alpnAllowed_.find(chosen) == alpnAllowed_.end()) {
> > +    MOZ_MTLOG(ML_ERROR, LAYER_INFO << "Bad ALPN string: " << chosen);
> > +    for (auto i = alpnAllowed_.begin(); i != alpnAllowed_.end(); ++i) {
> > +      MOZ_MTLOG(ML_INFO, LAYER_INFO << "Permitted ALPN: " << *i);
> 
> Shouldn't NSS be throwing an error here? Is this an invariant?

That's not the concern here, the concern is that we're not the server and the server picks some random crap.  NSS doesn't do that sanity check, and that's a carry over from the olden days of NPN, where the NPN string the client chooses doesn't have to be on the server's list.

I see no real problem with NSS externalizing this.  It permits some flexibility.
Since you asked, let's clear this up now.  I don't know if you want another bug number, but I consider this to be collateral.
Attachment #8486827 - Flags: review?(ekr)
I think that this should address everything in the first round.
Attachment #8412885 - Attachment is obsolete: true
Attachment #8486828 - Flags: review?(ekr)
Comment on attachment 8486827 [details] [diff] [review]
0001-Bug-996238-Reformat-gtest_utils.h.patch

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

lgtm
Attachment #8486827 - Flags: review?(ekr) → review+
Comment on attachment 8486828 [details] [diff] [review]
0002-Bug-996238-ALPN-support-for-WebRTC.patch

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

Still two issues I want to make sure we resolve.

::: media/mtransport/test/transport_unittests.cpp
@@ +510,5 @@
> +    nsresult res = dtls_->SetAlpn(alpn, withDefault ? str : "");
> +    ASSERT_EQ(NS_OK, res);
> +  }
> +
> +  std::string GetAlpn() {

const?

::: media/mtransport/transportlayerdtls.cpp
@@ +594,5 @@
>    }
>  
> +  if (!alpnAllowed_.empty()) {
> +    unsigned char buf[MAX_ALPN_LENGTH];
> +    size_t i = 0;

I feel like "offset" would be a better name.

@@ +598,5 @@
> +    size_t i = 0;
> +    for (auto tag = alpnAllowed_.begin();
> +         tag != alpnAllowed_.end(); ++tag) {
> +      MOZ_ASSERT(tag->length() <= 0xff, "ALPN tag too long");
> +      MOZ_ASSERT(i + 1 + tag->length() < MAX_ALPN_LENGTH, "ALPN too long");

suggest sizeof(buf).

IMPORTANT: I didn't mean to suggest we didn't need a runtime check here. I think
we do. I just think we only need one.

@@ +817,4 @@
>    if (rv == SECSuccess) {
>      MOZ_MTLOG(ML_NOTICE,
>                LAYER_INFO << "****** SSL handshake completed ******");
> +

What happened here?

@@ +826,5 @@
> +    if (!CheckAlpn()) {
> +      TL_SET_STATE(TS_ERROR);
> +      // Close the socket here, because failure to negotiate the correct
> +      // ALPN label doesn't result in a fatal error with the DTLS socket.
> +      PR_Close(ssl_fd_);

IMPORTANT: Do we need to set ssl_fd_ = nullptr?

@@ +888,5 @@
> +    MOZ_MTLOG(ML_NOTICE, LAYER_INFO << "ALPN not negotiated, "
> +              << (alpnDefault_.empty() ? "failing" : "selecting default"));
> +    alpn_ = alpnDefault_;
> +    return !alpn_.empty();
> +  }

Suggest some test so you catch any other states. Maybe a switch?

@@ +891,5 @@
> +    return !alpn_.empty();
> +  }
> +
> +  // NSS won't null terminate the ALPN string for us
> +  // TODO handle non-unicode strings

Please file a bug #.

Also, I note that all the strings you are using appear to be ASCII.

::: media/mtransport/transportlayerdtls.h
@@ +69,5 @@
>      identity_ = identity;
>    }
> +  nsresult SetAlpn(const std::set<std::string>& allowedAlpn,
> +                   const std::string& alpnDefault);
> +  std::string GetNegotiatedAlpn() const { return alpn_; }

Maybe const std::string& ?

@@ +161,5 @@
> +  // what ALPN identifiers are permitted
> +  std::set<std::string> alpnAllowed_;
> +  // what ALPN identifier is used if ALPN is not supported
> +  // the empty string indicates that ALPN is required
> +  std::string alpnDefault_;

Underscores not camel case in this code, please.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2696,4 @@
>        return nullptr;
>      }
>  
> +    std::set<std::string> alpn;

This seems like it could do with a comment to the effect that we
are always willing to do isolated but sometimes we require it.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +634,3 @@
>    dtlsLayer->SignalStateChange.disconnect(this);
>  
> +  bool privacyRequested = (dtlsLayer->GetNegotiatedAlpn() == "c-webrtc");

Is there a test for this part of the system?
Attachment #8486828 - Flags: review?(ekr) → review-
(In reply to Eric Rescorla (:ekr) from comment #10)
> > +      MOZ_ASSERT(tag->length() <= 0xff, "ALPN tag too long");
> > +      MOZ_ASSERT(i + 1 + tag->length() < MAX_ALPN_LENGTH, "ALPN too long");
> 
> IMPORTANT: I didn't mean to suggest we didn't need a runtime check here. I
> think
> we do. I just think we only need one.

Really?  This is purely a programming screwup.  I can make it a MOZ_RELEASE_ASSERT if you feel that's appropriate, but that doesn't seem necessary.

> > +      PR_Close(ssl_fd_);
> 
> IMPORTANT: Do we need to set ssl_fd_ = nullptr?

Not really, no.  We don't do that anywhere else, and nor do we do anything from the error state - at least through the usual code paths.  No harm in it though.
 
> > +  // TODO handle non-unicode strings
> 
> Please file a bug #.
> 
> Also, I note that all the strings you are using appear to be ASCII.

Yeah, I think that I'll remove the TODO and make it a comment instead.  If we need to negotiate non-Unicode, then we'll need to use something other than strings, which is a major inconvenience here and I'd consider it to be unlikely.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> @@ +634,3 @@
> >    dtlsLayer->SignalStateChange.disconnect(this);
> >  
> > +  bool privacyRequested = (dtlsLayer->GetNegotiatedAlpn() == "c-webrtc");
> 
> Is there a test for this part of the system?

Good point.  I think that I originally wanted to rely on the existing test, but it turns out that we don't have an asymmetric setup test.  Which I will add.  In a follow-on patch.
(In reply to Martin Thomson [:mt] from comment #11)
> (In reply to Eric Rescorla (:ekr) from comment #10)
> > > +      MOZ_ASSERT(tag->length() <= 0xff, "ALPN tag too long");
> > > +      MOZ_ASSERT(i + 1 + tag->length() < MAX_ALPN_LENGTH, "ALPN too long");
> > 
> > IMPORTANT: I didn't mean to suggest we didn't need a runtime check here. I
> > think
> > we do. I just think we only need one.
> 
> Really?  This is purely a programming screwup.  I can make it a
> MOZ_RELEASE_ASSERT if you feel that's appropriate, but that doesn't seem
> necessary.

It seems like this API is pretty brittle, so I'd prefer a runtime
check. My thought was that you would have something that asserted in
debug and returned error in release.


> > > +      PR_Close(ssl_fd_);
> > 
> > IMPORTANT: Do we need to set ssl_fd_ = nullptr?
> 
> Not really, no.  We don't do that anywhere else, and nor do we do anything
> from the error state - at least through the usual code paths.  No harm in it
> though.

Doesn't this lead to double-close?


> > > +  // TODO handle non-unicode strings
> > 
> > Please file a bug #.
> > 
> > Also, I note that all the strings you are using appear to be ASCII.
> 
> Yeah, I think that I'll remove the TODO and make it a comment instead.  If
> we need to negotiate non-Unicode, then we'll need to use something other
> than strings, which is a major inconvenience here and I'd consider it to be
> unlikely.
> 
> > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> > @@ +634,3 @@
> > >    dtlsLayer->SignalStateChange.disconnect(this);
> > >  
> > > +  bool privacyRequested = (dtlsLayer->GetNegotiatedAlpn() == "c-webrtc");
> > 
> > Is there a test for this part of the system?
> 
> Good point.  I think that I originally wanted to rely on the existing test,
> but it turns out that we don't have an asymmetric setup test.  Which I will
> add.  In a follow-on patch.
Take 3.
Attachment #8486828 - Attachment is obsolete: true
Attachment #8487398 - Flags: review?(ekr)
Adding a guard against misuse in GetSrtpCiphers and ExportKeyingMaterial.  This doesn't cause problems in practice, because they are only called on newly opened flows (the code is directly after an upcall where the state becomes TS_OPEN), but this code doesn't know that and it's better to enforce that.
Attachment #8487398 - Attachment is obsolete: true
Attachment #8487398 - Flags: review?(ekr)
Attachment #8487429 - Flags: review?(ekr)
Adding a test case.  The diff might look a little odd here because git thinks that the factored out js file is a rename of the existing html file.  Basically, this is exactly the same test that I've been using, but with no peerIdentity isolation on the "remote" party.  Because the "local" party has isolation, the session should be created with the confidential ALPN token and media from the remote party should be isolated.
Attachment #8487430 - Flags: review?(ekr)
Attached file MozReview Request: bz://996238/mt (obsolete) — 

    
    

    
  
/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC

Pull down these commits:

hg pull review -r 7c45d360c7d16eb492edfb7656d36d3aae034657

    
  

        Attachment #8542235 -
        Flags: review?(ekr)

    
    

    
  
/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC

Pull down these commits:

hg pull review -r 7c45d360c7d16eb492edfb7656d36d3aae034657

    
  

        Attachment #8487430 -
        Attachment is obsolete: true

        Attachment #8487430 -
        Flags: review?(ekr)

    
  

        Attachment #8487429 -
        Attachment is obsolete: true

        Attachment #8487429 -
        Flags: review?(ekr)

    
  

        Attachment #8486827 -
        Attachment is obsolete: true

    
    

    
  
I'll try to get to this next week.

    
    

    
  
https://reviewboard.mozilla.org/r/1755/#review1251

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +  char chosenAlpn[255];

Use MAX_ALPN_LENGTH

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +      MOZ_ASSERT(tag->length() <= 0xff, "ALPN tag too long");

Redundant assert

    
    

    
  
https://reviewboard.mozilla.org/r/1759/#review1249

::: media/mtransport/transportlayerdtls.h
(Diff revision 1)
> +  // what ALPN identifiers are permitted

Nit: periods after comments.

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +  const std::set<std::string>& alpnAllowed,

convention here is underscore, not camelcase

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +  rv = SSL_OptionSet(ssl_fd, SSL_ENABLE_ALPN, PR_TRUE);

Should we be enabling ALPN if nobody has provided an ALPN set?

Would it make more sense to put this next to the code below?

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +      MOZ_ASSERT(tag->length() <= 0xff, "ALPN tag too long");

These asserts seem weird, since the second one is more restrictive than the first. I guess the idea is that the first one enforces a protocol invariant and the second one an implementation invariant?

It's also worth noting that this API is not safe if the caller puts in overlong ALPN values. Is there a reason not to have a runtime check here? The second one would be enough.

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +      // ALPN label doesn't result in a fatal error with the DTLS socket.

Nit: Suggested rewording "because failure to negotiate the correct ALPN label doesn't result in a failure at the NSS layer."

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +// after this returns successfully, alpn_ will be set to the negotiated protocol

Maybe some upper-case letters and periods would help with these comments.

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +      return;

Do we need to close the socket? Won't it just be closed when the TLDtls is destroyed?

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +  char chosenAlpn[255];

MAX_ALPN_LENGTH?

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +                                  &chosenAlpnLen, 255);

sizeof(chosenAlpn)

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +    MOZ_MTLOG(ML_ERROR, LAYER_INFO << "ALPN error");

Can this ever happen except by a programming error?

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +  case SSL_NEXT_PROTO_SELECTED:

Shouldn't these cases be indented by 2?

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +  case SSL_NEXT_PROTO_NO_OVERLAP:

When does this happen? A comment might be useful

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +  if (alpn_allowed_.find(chosen) == alpn_allowed_.end()) {

How does this happen? A comment wold be welcome.

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +      MOZ_MTLOG(ML_INFO, LAYER_INFO << "Permitted ALPN: " << *i);

Would it be worth concatenating these into a string so they appear on the same log line?

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +    return NS_ERROR_NOT_AVAILABLE;

Should we have an assert here?

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +    return NS_ERROR_NOT_AVAILABLE;

Should we have an assert here?

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
(Diff revision 1)
> +  // if we aren't confidential

Period after comment.

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
(Diff revision 1)
> +  rv = dtls->SetAlpn(alpn, alpnDefault);

If I read this code correctly, there are three permitted responses from the negotiation.

  c-webrtc
  webrtc (if offered)
  ""
  
And if you get back "", that's an error, right, since if the other side refused ALPN negotiation you get "webrtc".

Is that right?

::: media/mtransport/test/transport_unittests.cpp
(Diff revision 1)
> +  void SetAlpn(std::string str, bool withDefault, bool withExtra = false) {

This seems only to be called from TransportTest::Alpn. Would probably bs simpler to just have std::string extra and then if extra.empty()

::: media/mtransport/test/transport_unittests.cpp
(Diff revision 1)
> +  // that ALPN wasn't negotiated; the client sees a close

Period after comment.

::: media/mtransport/test/transport_unittests.cpp
(Diff revision 1)
> +  // a problem and just sees the close

Period after comment. Also start sentences with uppercase here and above.

        Attachment #8542235 -
        Flags: review?(ekr)

    
    

    
  
https://reviewboard.mozilla.org/r/1761/#review1257

lgtm with nits and a comment about the test framework. If you can't remove all that boilerplate, please file a bug about the tests.

::: dom/media/tests/mochitest/identity/identityPcTest.js
(Diff revision 1)
> +  test.setMediaConstraints([ options.localAudio || {

What is localAudio? I don't see it in dxr.

::: dom/media/tests/mochitest/identity/identityPcTest.js
(Diff revision 1)
> +      function checkIdentity(pc, pfx, idp, name) {

Nit: pfx would be clearer as "prefix"

::: dom/media/tests/mochitest/identity/identityPcTest.js
(Diff revision 1)
> +      checkIdentity(test.pcRemote._pc, "remote: ", "test1.example.com", "someone");

Since you reuse these would it make sense to have them be variables?

::: dom/media/tests/mochitest/identity/identityPcTest.js
(Diff revision 1)
> +      function done() {

Wow, is this really the best we have in our infrastructure? see: http://underscorejs.org/#after

::: dom/media/tests/mochitest/identity/test_peerConnection_asymmetricIsolation.html
(Diff revision 1)
> +  <script type="application/javascript" src="identityPcTest.js"></script>

Wow, almost none of these seem to be used directly in this code. Is there no way to avoid having all the cut-and-paste? If the answr is no, let's file a bug

::: dom/media/tests/mochitest/identity/test_peerConnection_asymmetricIsolation.html
(Diff revision 1)
> +function theTest() {

Do we have a negative test for what happens when the other side refuses this?

    
    

    
  
https://reviewboard.mozilla.org/r/1759/#review1259

::: media/mtransport/transportlayerdtls.h
(Diff revision 1)
> +  // what ALPN identifiers are permitted

I don't understand this.  It's not a sentence, but I'll make faux sentences.

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 1)
> +  rv = SSL_OptionSet(ssl_fd, SSL_ENABLE_ALPN, PR_TRUE);

I don't know, it seemed like it wasn't a big deal to leave all the options in the one place.  But I take your point.  I might factor the ALPN setup out while I'm at it.

    
    

    
  
https://reviewboard.mozilla.org/r/1759/#review1261

> These asserts seem weird, since the second one is more restrictive than the first. I guess the idea is that the first one enforces a protocol invariant and the second one an implementation invariant?
> 
> It's also worth noting that this API is not safe if the caller puts in overlong ALPN values. Is there a reason not to have a runtime check here? The second one would be enough.

I was thinking that the first assert can be dropped (since the second is tighter).  As for runtime checks, I've switched to a runtime check.

> Can this ever happen except by a programming error?

(Wow replying to reviews on RB royally sucks.  You can't reply from the diff page and the page where you can reply shows a *single* line of context, which is laughably useless.)

Anyhow, my read of this error is that it only happens if someone screws up badly, or you are in a seriously bad state (i.e., malloc fails).  I guess that the former is worth logging, and the latter is so screwed up that the logging probably won't go anywhere.

Are you suggesting that I not log here?

> Would it be worth concatenating these into a string so they appear on the same log line?

I guess that we're already deep into the whole streams thing anyway.  Sure.

> Should we have an assert here?

I'd rather report an error here than choke and die.  If this did something important and we could require that the caller check state before calling in, sure, but I don't see any value in requiring that the caller check first - in this case.

> Should we have an assert here?

Here, I was just being nice.  I've made this assert on debug, fail at runtime, is that OK?

> If I read this code correctly, there are three permitted responses from the negotiation.
> 
>   c-webrtc
>   webrtc (if offered)
>   ""
>   
> And if you get back "", that's an error, right, since if the other side refused ALPN negotiation you get "webrtc".
> 
> Is that right?

If you set a default, then you will get the default (a default of "" is not setting it).  If you don't set a default (i.e., the confidential case), then the connection will fail.  The value of tldtls->GetAlpn() will be "" at that point, but the real indication that you didn't get ALPN is that the connection wasn't established.  You could use the empty string as an indicator that ALPN failed, but that value could be there for a number of other reasons (maybe ICE failed, for instance).

    
  

        Attachment #8542235 -
        Flags: review?(jib)

        Attachment #8542235 -
        Flags: review?(ekr)

    
    

    
  
/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC, r=ekr
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC, r=ekr
/r/1991 - Bug 996238 - Move more of the test code to promises, r=jib

Pull down these commits:

hg pull review -r 5672adb27723a8d615611cc1210e2e6e80d6a06c

    
    

    
  
:jib, I've moved some more of the code to promises.  I had hoped to use MediaElementChecker in the media isolation tests, but that didn't work out particularly well, so I kept the old logic and just polished it up a little.

    
    

    
  
https://reviewboard.mozilla.org/r/1759/#review1267

This looks pretty good. I want to make another pass through on Monday since this is important code, but I don't see anything that currently needs big changes

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
(Diff revisions 1 - 2)
>    // Only allow non-confidential (which is an allowed default),

Nit: you don't need the comma.

    
    

    
  
https://reviewboard.mozilla.org/r/1759/#review1265

> (Wow replying to reviews on RB royally sucks.  You can't reply from the diff page and the page where you can reply shows a *single* line of context, which is laughably useless.)
> 
> Anyhow, my read of this error is that it only happens if someone screws up badly, or you are in a seriously bad state (i.e., malloc fails).  I guess that the former is worth logging, and the latter is so screwed up that the logging probably won't go anywhere.
> 
> Are you suggesting that I not log here?

No, I think logging is fine here, I'm just trying to get my head around what could happen. If this is an internal error, maybe we should udate the logging message. Up to you.

> Here, I was just being nice.  I've made this assert on debug, fail at runtime, is that OK?

SG

> I'd rather report an error here than choke and die.  If this did something important and we could require that the caller check state before calling in, sure, but I don't see any value in requiring that the caller check first - in this case.

OK.

> If you set a default, then you will get the default (a default of "" is not setting it).  If you don't set a default (i.e., the confidential case), then the connection will fail.  The value of tldtls->GetAlpn() will be "" at that point, but the real indication that you didn't get ALPN is that the connection wasn't established.  You could use the empty string as an indicator that ALPN failed, but that value could be there for a number of other reasons (maybe ICE failed, for instance).

It looks to me like we always set a default but we set it to be "" if we are doing c-webrtc. Is that wrong?

    
    

    
  
(In reply to Eric Rescorla (:ekr) from comment #30)
> It looks to me like we always set a default but we set it to be "" if we are
> doing c-webrtc. Is that wrong?

Yep, that's right.  An empty value is probably valid, but here it means "no default".

    
    

    
  
https://reviewboard.mozilla.org/r/1991/#review1281

I think I would like to see it again.

::: dom/media/tests/mochitest/blacksilence.js
(Diff revision 1)
> +    // and it gets droped (and GC'd) when the test is done.

dropped

::: dom/media/tests/mochitest/blacksilence.js
(Diff revision 1)
> +  function wait(time) {
> +    return new Promise(r => setTimeout(r, time));
> +  }

Why not: var wait = ms => new Promise(r => setTimeout(r, ms));

::: dom/media/tests/mochitest/blacksilence.js
(Diff revision 1)
> +    checks.reduce((prev, next) => prev.then(next), Promise.resolve());

I think you need:

checks.reduce((prev, next, i) => prev.then(() => next(i)), Promise.resolve());

or there's no increment. Otherwise nicely done.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
>    checkMediaFlowPresent : function PCW_checkMediaFlowPresent(onSuccess) {

onSuccess is unused

::: dom/media/tests/mochitest/templates.js
(Diff revision 1)
> -      test.pcLocal.checkMediaFlowPresent(function () {
> +      test.pcLocal.checkMediaFlowPresent().then(() => test.next());
> -        test.next();
> -      });
>      }
>    ],
>    [
>      'PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT',
>      function (test) {
> -      test.pcRemote.checkMediaFlowPresent(function () {
> +      test.pcRemote.checkMediaFlowPresent().then(() => test.next());

.catch()

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
>    waitForMediaFlow : function MEC_WaitForMediaFlow(onSuccess) {

onSuccess Evanesco!

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -    if(self.canPlayThroughFired && self.timeUpdateFired && self.timePassed) {
> +    if(this.canPlayThroughFired && this.timeUpdateFired && this.timePassed) {

space after if, since you touched it.

::: dom/media/tests/mochitest/blacksilence.js
(Diff revision 1)
> -    function disconnect() {
> +      .then(() => {
> -      source.disconnect();
> +        source.disconnect();
> -      analyser.disconnect();
> +        analyser.disconnect();

The old code called these even in the cancel case, while the new code doesn't AFAICT. What happens if you don't disconnect? Is this step even necessary? If it is, then a catch and re-throw is needed to execute them in the failure case.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -        var mediaChecker = self.mediaCheckers[index];
> -        mediaChecker.waitForMediaFlow(function() {
> -          _checkMediaFlowPresent(index + 1, onSuccess);
> +        var mediaChecker = this.mediaCheckers[index];
> +        return mediaChecker.waitForMediaFlow()
> +          .then(() => _checkMediaFlowPresent(index + 1));

I wonder if these checks could be done in parallel with Promise.all?

::: dom/media/tests/mochitest/templates.js
(Diff revision 1)
> -      test.pcLocal.checkMediaFlowPresent(function () {
> +      test.pcLocal.checkMediaFlowPresent().then(() => test.next());
> -        test.next();
> -      });
>      }
>    ],
>    [
>      'PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT',
>      function (test) {
> -      test.pcRemote.checkMediaFlowPresent(function () {
> +      test.pcRemote.checkMediaFlowPresent().then(() => test.next());

All of these need .catch() to not bury errors unfortunately (or return promise up someday...)

::: dom/media/tests/mochitest/test_getUserMedia_peerIdentity.html
(Diff revision 1)
> -    navigator.mediaDevices.getUserMedia(config).then(function(stream) {
> -      var oneDone = false;
> -      function checkDone() {
> -        if (oneDone) {
> -          done();
> +    return navigator.mediaDevices.getUserMedia(config).then(function(stream) {
> +      return Promise.all([
> +        audioIsSilence(withConstraint, stream),
> +        videoIsBlack(withConstraint, stream)
> +      ]);

No => here? would lose the {}...

::: dom/media/tests/mochitest/test_getUserMedia_peerIdentity.html
(Diff revision 1)
> -  // without constraint
> -  testPeerIdentityConstraint(false, function() {
> -    // with the constraint
> -    testPeerIdentityConstraint(true, SimpleTest.finish.bind(SimpleTest));
> +  // both without and with the constraint
> +  testPeerIdentityConstraint(false)
> +    .then(() => testPeerIdentityConstraint(true))
> +    .then(() => SimpleTest.finish());

Need a .catch at the end to handle errors, and maybe one before calling SimpleTest.finish() as well to not halt subsequent tests on most errors?

::: dom/media/tests/mochitest/blacksilence.js
(Diff revision 1)
> +    for (var i = 0; i < 10; ++i) {
> +      checks.push(num => {

I dislike function-creation inside for-loops, not just because of performance - I think this creates the same function 10 times - but because it invites logic bugs around closures, like thinking i holds a different value in each created function - a code maintenance issue.

I would create the function outside the loop, which would let you s/num/i/ as well without confusion.

    
    

    
  
https://reviewboard.mozilla.org/r/1991/#review1317

Good.  I need to make sure that the tests pass on try too...  I'll reflag when it's working.

> The old code called these even in the cancel case, while the new code doesn't AFAICT. What happens if you don't disconnect? Is this step even necessary? If it is, then a catch and re-throw is needed to execute them in the failure case.

The test can't be cancelled now.  If it fails, we will leave things connected and let the page/frame reload take care of cleanup.  As for errors, the same applies.  I decided to leave that to the enclosing test case to handle.

> I think you need:
> 
> checks.reduce((prev, next, i) => prev.then(() => next(i)), Promise.resolve());
> 
> or there's no increment. Otherwise nicely done.

I've done something a little different, because I think that the Array.reduce was a little contrived once I refactored it.

> I dislike function-creation inside for-loops, not just because of performance - I think this creates the same function 10 times - but because it invites logic bugs around closures, like thinking i holds a different value in each created function - a code maintenance issue.
> 
> I would create the function outside the loop, which would let you s/num/i/ as well without confusion.

Totally agree.  I do want multiple closures, but this isn't going to do that right.  Shame on me for hitting a classic JavaScript trap.

> I wonder if these checks could be done in parallel with Promise.all?

I had exactly the same thought just now.  It's much less code that way and probably faster too.

    
    

    
  
Comment on attachment 8542235 [details]
MozReview Request: bz://996238/mt

(In reply to Martin Thomson [:mt] from comment #34)
> I've done something a little different, because I think that the
> Array.reduce was a little contrived once I refactored it.

Yeah but it was so cool-looking I didn't want to say anything. ;-) e.g. http://jsfiddle.net/jib1/js28u7sb

        Attachment #8542235 -
        Flags: review?(jib)

    
  

        Attachment #8542235 -
        Flags: review?(jib)

    
    

    
  
/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC, r=ekr
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC, r=ekr
/r/1991 - Bug 996238 - Move more of the test code to promises, r=jib

Pull down these commits:

hg pull review -r d36a73b366d844853b786acf2ea0f1357c753cb5

    
    

    
  
https://reviewboard.mozilla.org/r/1761/#review1263

> What is localAudio? I don't see it in dxr.

I just realized that I'm not using it.  No point having it then I guess.

> Wow, almost none of these seem to be used directly in this code. Is there no way to avoid having all the cut-and-paste? If the answr is no, let's file a bug

I don't know what you imagine a fix would look like here.  It's not unusual to import the things that you use and the things that those things use.  Are you looking for something that manages transitive dependencies, like require.js?  That's not a bad idea, but I think that I'll let you carry the sword on that one.

> Do we have a negative test for what happens when the other side refuses this?

In transport_unittests.cpp, but not at this level.  We don't have anything because doing that would require us to break Firefox somehow.  I can imagine exposing ChromeOnly properties that would let us frob that, but it's a lot of plumbing for marginal utility given that the lower level tests are in place.  When we have a setup that tests against all sorts of real peers (like Chrome), then perhaps this is worth revisiting.

> Wow, is this really the best we have in our infrastructure? see: http://underscorejs.org/#after

Now you're really asking for it...  I'll fix it.  See the next commit if you are interested.

    
    

    
  
/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC, r=ekr
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC, r=ekr
/r/1991 - Bug 996238 - Move more of the test code to promises, r=jib

Pull down these commits:

hg pull review -r d36a73b366d844853b786acf2ea0f1357c753cb5

    
    

    
  
In case it's not clear from RB: I have:
/r/1759 for ekr
/r/1991 for jib

...outstanding.  ekr, 1991 is a largely mechanical transformation to address the concern you had around test infrastructure.  I've it working on try, I need to setup a run with more complete test before this can be landed.

    
    

    
  
/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC, r=ekr
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC, r=ekr
/r/1991 - Bug 996238 - Move more of the test code to promises, r=jib

Pull down these commits:

hg pull review -r d36a73b366d844853b786acf2ea0f1357c753cb5

    
    

    
  
/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC, r=ekr
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC, r=ekr
/r/1991 - Bug 996238 - Move more of the test code to promises, r=jib

Pull down these commits:

hg pull review -r d36a73b366d844853b786acf2ea0f1357c753cb5

    
    

    
  
https://reviewboard.mozilla.org/r/1757/#review1515

Inheriting r=ekr from bugzilla.

    
    

    
  
https://reviewboard.mozilla.org/r/1991/#review1519

::: dom/media/tests/mochitest/blacksilence.js
(Diff revision 2)
> +          if (done) {
> +            return Promise.resolve();
> +          }

You could just return; here (it gets promoted), or even flip the if() and just let it fall off the function, to make this even smaller.

You could also merge resolve and done, though that may be less readable, but now we're code-golfing I suppose.

::: dom/media/tests/mochitest/blacksilence.js
(Diff revision 2)
> +      chain = chain.then(waitAndCheck(i));

Maybe: chain = chain.then(waitAndCheck(200 << i));

and s/counter/ms/ ?

Also, I miss the comment from your last patch about why all this is needed ("exponential backoff for b2g").

        Attachment #8542235 -
        Flags: review?(jib) → review+

    
    

    
  
https://reviewboard.mozilla.org/r/1759/#review1593

LGTM with nits.

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 3)
> +

Extra whitespace.

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 3)
> +  SECStatus rv = SSL_OptionSet(ssl_fd, SSL_ENABLE_NPN, PR_FALSE);

Is there a reason not to unconditionally turn off NPN?

::: media/mtransport/transportlayerdtls.cpp
(Diff revision 3)
> +  unsigned char buf[MAX_ALPN_LENGTH];

Can you add a comment that if this value becomes > 255, then you need to check each individual tag as well?

    
    

    
  
https://reviewboard.mozilla.org/r/1759/#review1595

> Can you add a comment that if this value becomes > 255, then you need to check each individual tag as well?

Maybe even a static assert.

    
    

    
  
https://reviewboard.mozilla.org/r/1759/#review1285

> It looks to me like we always set a default but we set it to be "" if we are doing c-webrtc. Is that wrong?

Well, setting a default of "" is equivalent to not setting a default.  See line 899 of transportlayerdtls.cpp (damn, I can't link to a specific line, reviewboard is getting worse and worse).

    
    

    
  
OK, now the bad news on this one...  This is blocked by bug 753136; it won't land without a version of NSS that actually fails the handshake properly.  I'll see if wtc has some cycles to get this unblocked.
Depends on: 753136

    
    

    
  
https://reviewboard.mozilla.org/r/1759/#review1609

> Do we need to close the socket? Won't it just be closed when the TLDtls is destroyed?

Turns out that the tests show that this is necessary.  Apparently NSS soldiers on if you don't tell it to close the socket.

    
    

    
  
Belay that last comment.  I did notice that closing helped with one of the test cases; however, the hard dependency on 753136 was the real problem.

    
  

        Attachment #8542235 -
        Flags: review+

    
    

    
  
/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC, r=ekr
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC, r=ekr
/r/1991 - Bug 996238 - Move more of the test code to promises, r=jib
/r/2473 - Bug 996238 - Force handshake failure if ALPN fails, r=ekr

Pull down these commits:

hg pull review -r 67f977edf99445df0f0ff532e43efa2c1d73f017

    
  

        Attachment #8542235 -
        Attachment is obsolete: true

        Attachment #8542235 -
        Flags: review?(ekr)

    
    

    
  
Comment on attachment 8542235 [details]
MozReview Request: bz://996238/mt

/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC, r=ekr
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC, r=ekr
/r/1991 - Bug 996238 - Move more of the test code to promises, r=jib

Pull down these commits:

hg pull -r 2624ff62e795febf82190bd50f336c1fd7289238 https://reviewboard-hg.mozilla.org/gecko/

        Attachment #8542235 -
        Attachment is obsolete: false

        Attachment #8542235 -
        Flags: review?(ekr)

    
    

    
  
Comment on attachment 8542235 [details]
MozReview Request: bz://996238/mt

Applying the r+ that RB didn't apply.

        Attachment #8542235 -
        Flags: review?(ekr) → review+

    
  
Keywords: checkin-needed

    
    

    
  
sorry had to back this out since this might have caused a CPP Test Bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=8351531&repo=mozilla-inbound
Flags: needinfo?(martin.thomson)

    
    

    
  
Comment on attachment 8542235 [details]
MozReview Request: bz://996238/mt

/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC, r=ekr
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC, r=ekr
/r/1991 - Bug 996238 - Move more of the test code to promises, r=jib

Pull down these commits:

hg pull -r 18566f8281925ee2ff17e69762c6514e98c2fcce https://reviewboard-hg.mozilla.org/gecko/

        Attachment #8542235 -
        Flags: review+ → review?(ekr)

    
    

    
  
Comment on attachment 8542235 [details]
MozReview Request: bz://996238/mt

/r/1757 - Bug 996238 - Reformat gtest_utils.h, r=ekr
/r/1759 - Bug 996238 - ALPN support for WebRTC, r=ekr
/r/1761 - Bug 996238 - Adding test for asymmetric media isolation across PC, r=ekr
/r/1991 - Bug 996238 - Move more of the test code to promises, r=jib

Pull down these commits:

hg pull -r 2b95d021b6272baab9a2a2db51880f3e332a6d30 https://reviewboard-hg.mozilla.org/gecko/

    
    

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

I'm going to assume the SM failures are infra-flakiness here.  I can't retrigger.
Flags: needinfo?(martin.thomson)
Keywords: checkin-needed

    
    

    
  
Comment on attachment 8542235 [details]
MozReview Request: bz://996238/mt

MT says this is a RB glitch, so removing r?

        Attachment #8542235 -
        Flags: review?(ekr)

    
    

    
  
Comment on attachment 8542235 [details]
MozReview Request: bz://996238/mt

Should have been +

        Attachment #8542235 -
        Flags: review+

    
    

    
  

        Attachment #8542235 -
        Attachment is obsolete: true

        Attachment #8618118 -
        Flags: review+

        Attachment #8618119 -
        Flags: review+

        Attachment #8618120 -
        Flags: review+

        Attachment #8618121 -
        Flags: review+

        Attachment #8618122 -
        Flags: review+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: