Closed
Bug 996238
Opened 11 years ago
Closed 10 years ago
ALPN identifiers for WebRTC
Categories
(Core :: WebRTC: Networking, defect)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: mt, Assigned: mt)
References
()
Details
Attachments
(5 files, 7 obsolete files)
Use ALPN as a basis for stream isolation in WebRTC. See URL for proposed names.
Assignee | ||
Comment 1•11 years ago
|
||
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.
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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-
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
New patches coming: https://tbpl.mozilla.org/?tree=Try&rev=3978520bc17f
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
I think that this should address everything in the first round.
Attachment #8412885 -
Attachment is obsolete: true
Attachment #8486828 -
Flags: review?(ekr)
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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-
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
Take 3.
Attachment #8486828 -
Attachment is obsolete: true
Attachment #8487398 -
Flags: review?(ekr)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
Assignee | ||
Comment 17•10 years ago
|
||
Assignee | ||
Comment 18•10 years ago
|
||
/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
Assignee | ||
Updated•10 years ago
|
Attachment #8542235 -
Flags: review?(ekr)
Assignee | ||
Comment 19•10 years ago
|
||
/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
Assignee | ||
Updated•10 years ago
|
Attachment #8487430 -
Attachment is obsolete: true
Attachment #8487430 -
Flags: review?(ekr)
Assignee | ||
Updated•10 years ago
|
Attachment #8487429 -
Attachment is obsolete: true
Attachment #8487429 -
Flags: review?(ekr)
Assignee | ||
Updated•10 years ago
|
Attachment #8486827 -
Attachment is obsolete: true
Comment 20•10 years ago
|
||
I'll try to get to this next week.
Assignee | ||
Comment 21•10 years ago
|
||
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
Comment 22•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8542235 -
Flags: review?(ekr)
Comment 23•10 years ago
|
||
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?
Assignee | ||
Comment 24•10 years ago
|
||
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.
Assignee | ||
Comment 25•10 years ago
|
||
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).
Assignee | ||
Updated•10 years ago
|
Attachment #8542235 -
Flags: review?(jib)
Attachment #8542235 -
Flags: review?(ekr)
Assignee | ||
Comment 26•10 years ago
|
||
/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
Assignee | ||
Comment 27•10 years ago
|
||
: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.
Assignee | ||
Comment 28•10 years ago
|
||
Comment 29•10 years ago
|
||
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.
Comment 30•10 years ago
|
||
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?
Comment 31•10 years ago
|
||
Assignee | ||
Comment 32•10 years ago
|
||
(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".
Comment 33•10 years ago
|
||
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.
Assignee | ||
Comment 34•10 years ago
|
||
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 35•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8542235 -
Flags: review?(jib)
Assignee | ||
Comment 36•10 years ago
|
||
/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
Assignee | ||
Comment 37•10 years ago
|
||
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.
Assignee | ||
Comment 38•10 years ago
|
||
/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
Assignee | ||
Comment 39•10 years ago
|
||
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.
Assignee | ||
Comment 40•10 years ago
|
||
A tiny slice of working-on-try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b6f1570a2fa
Assignee | ||
Comment 41•10 years ago
|
||
Assignee | ||
Comment 42•10 years ago
|
||
/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
Assignee | ||
Comment 43•10 years ago
|
||
/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
Assignee | ||
Comment 44•10 years ago
|
||
Assignee | ||
Comment 45•10 years ago
|
||
https://reviewboard.mozilla.org/r/1757/#review1515
Inheriting r=ekr from bugzilla.
Comment 46•10 years ago
|
||
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").
Updated•10 years ago
|
Attachment #8542235 -
Flags: review?(jib) → review+
Comment 47•10 years ago
|
||
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?
Comment 48•10 years ago
|
||
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.
Assignee | ||
Comment 49•10 years ago
|
||
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).
Assignee | ||
Comment 50•10 years ago
|
||
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
Assignee | ||
Comment 51•10 years ago
|
||
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.
Assignee | ||
Comment 52•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8542235 -
Flags: review+
Assignee | ||
Comment 53•10 years ago
|
||
/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
Assignee | ||
Comment 54•10 years ago
|
||
Rebased, retried:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72418b1bcf78
Assignee | ||
Updated•10 years ago
|
Attachment #8542235 -
Attachment is obsolete: true
Attachment #8542235 -
Flags: review?(ekr)
Assignee | ||
Comment 55•10 years ago
|
||
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)
Assignee | ||
Comment 56•10 years ago
|
||
Comment on attachment 8542235 [details]
MozReview Request: bz://996238/mt
Applying the r+ that RB didn't apply.
Attachment #8542235 -
Flags: review?(ekr) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 57•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fce22efb76e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ee10cfc0489
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a21cd6530b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/445c2952117c
Flags: in-testsuite+
Keywords: checkin-needed
Comment 58•10 years ago
|
||
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)
Assignee | ||
Comment 59•10 years ago
|
||
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)
Assignee | ||
Comment 60•10 years ago
|
||
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/
Assignee | ||
Comment 61•10 years ago
|
||
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 62•10 years ago
|
||
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)
Assignee | ||
Comment 63•10 years ago
|
||
Comment on attachment 8542235 [details]
MozReview Request: bz://996238/mt
Should have been +
Attachment #8542235 -
Flags: review+
Comment 64•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3348852ed0cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/033ff1073b76
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c6bfc178fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d57d943b918
Keywords: checkin-needed
Comment 65•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3348852ed0cd
https://hg.mozilla.org/mozilla-central/rev/033ff1073b76
https://hg.mozilla.org/mozilla-central/rev/f8c6bfc178fb
https://hg.mozilla.org/mozilla-central/rev/6d57d943b918
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 66•9 years ago
|
||
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+
Assignee | ||
Comment 67•9 years ago
|
||
Assignee | ||
Comment 68•9 years ago
|
||
Assignee | ||
Comment 69•9 years ago
|
||
Assignee | ||
Comment 70•9 years ago
|
||
Assignee | ||
Comment 71•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•