1006207 - Remove redundant code from transport layer

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Martin Thomson [:mt:]]

This was originally intended to be a little more generic than it currently is, and there is some vestigal code in here for handling STREAM transports in TransportLayerDtls.  But it's not actually used in practice.  TransportLayerDtls sets mode_ to DGRAM and leaves it there.

Comment 1

[Martin Thomson [:mt:]]

Byron, is there anything in the ICE TCP patches that would change this?

Comment 2

[Byron Campen [:bwc]]

Nope, nor do I have anything in progress that touches this.

Comment 3

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1006207-Removing-unused-mode_-code.patch

All good, all the unit tests run and pass.  I'm going to run this on try to see if there are weird compiler corner cases that are triggered by the absence of code, but I'm feeling bullish on this.

Comment 4

[Martin Thomson [:mt:]]

Maybe I'm being too parsimonious, but I think that this is enough: https://tbpl.mozilla.org/?tree=Try&rev=93eb39e68f6f

Comment 5

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1006207-Removing-unused-mode_-code.patch

So yeah, reindent was necessary.  Missed that.

Comment 6

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1006207-Removing-unused-mode_-code.patch

Comment 7

[Martin Thomson [:mt:]]

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

Comment 8

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1006207-Removing-unused-mode_-code.patch

Rebased to inbound, which had conflicting changes.

Comment 9

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-1006207-Removing-offset-from-Packet-class-forcin.patch

Comment 10

[Eric Rescorla (:ekr)]

Comment on attachment 8466526 [details] [diff] [review]
0001-Bug-1006207-Removing-unused-mode_-code.patch

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

::: media/mtransport/transportlayerdtls.cpp
@@ +453,4 @@
>      return false;
>    pr_fd->secret = reinterpret_cast<PRFilePrivate *>(nspr_io_adapter_.get());
>  
> +  ScopedPRFileDesc ssl_fd(DTLS_ImportFD(nullptr, pr_fd));

This code uses = rather than initialization syntax.

@@ +644,4 @@
>      int32_t err = PR_GetError();
>      switch(err) {
>        case SSL_ERROR_RX_MALFORMED_HANDSHAKE:
> +        MOZ_MTLOG(ML_ERROR, LAYER_INFO << "Malformed DTLS message; ignoring");

Please leave a comment here about how you would handle stream, just for the future.

@@ +652,5 @@
> +        rv = DTLS_GetHandshakeTimeout(ssl_fd_, &timeout);
> +        if (rv == SECSuccess) {
> +          uint32_t timeout_ms = PR_IntervalToMilliseconds(timeout);
> +
> +          MOZ_MTLOG(ML_DEBUG, LAYER_INFO << "Setting DTLS timeout to " <<

Fix indent. Suggest break before <<

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 8466528 [details] [diff] [review]
0002-Bug-1006207-Removing-offset-from-Packet-class-forcin.patch

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

This seems like it may need one more pass

::: media/mtransport/transportlayerdtls.cpp
@@ +77,4 @@
>    input_.back()->Assign(data, len);
>  }
>  
> +int32_t TransportLayerNSPRAdapter::Read(void *data, int32_t buflen) {

Let's rename this to Recv.

@@ +85,5 @@
>  
>    Packet* front = input_.front();
> +  if (buflen < front->len_) {
> +    PR_SetError(PR_BUFFER_OVERFLOW_ERROR, 0);
> +    return -1;

Note that this is basically not recoverable. Should we drop this
packet? Certainly should add an assert.

@@ +90,3 @@
>    }
>  
> +  int32_t count = front->len_;

Why create a temporary here and why is it signed?

Comment 12

[Martin Thomson [:mt:]]

(In reply to Eric Rescorla (:ekr) from comment #10)
> Comment on attachment 8466526 [details] [diff] [review]
> >  
> > +  ScopedPRFileDesc ssl_fd(DTLS_ImportFD(nullptr, pr_fd));
> 
> This code uses = rather than initialization syntax.

Really?  Because this is a direct copy from a few lines above where pr_fd is initialized as so:

  ScopedPRFileDesc pr_fd(PR_CreateIOLayerStub(transport_layer_identity,
                                              &TransportLayerMethods));
> @@ +644,4 @@
> >      int32_t err = PR_GetError();
> >      switch(err) {
> >        case SSL_ERROR_RX_MALFORMED_HANDSHAKE:
> > +        MOZ_MTLOG(ML_ERROR, LAYER_INFO << "Malformed DTLS message; ignoring");
> 
> Please leave a comment here about how you would handle stream, just for the
> future.

I don't think that I'd drive a streamed protocol through here in future, but sure.

(In reply to Eric Rescorla (:ekr) from comment #11)

I changed the name and used MOZ_ASSERT().

> @@ +90,3 @@
> >    }
> >  
> > +  int32_t count = front->len_;
> 
> Why create a temporary here and why is it signed?

Because I need to return this value and front is deleted before the method returns.

Comment 13

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1006207-Removing-unused-mode_-code.patch

r=ekr

Comment 14

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-1006207-Removing-offset-from-Packet-class-forcin.patch

https://tbpl.mozilla.org/?tree=Try&rev=9ebe243bba40

Comment 15

[Eric Rescorla (:ekr)]

Comment on attachment 8467379 [details] [diff] [review]
0002-Bug-1006207-Removing-offset-from-Packet-class-forcin.patch

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

lgtm

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/302936c0978c
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd811aaf5a90

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/302936c0978c
https://hg.mozilla.org/mozilla-central/rev/fd811aaf5a90