740122 - fix MSVC "possible loss of data" warnings in netwerk/

Status: NEW

(Core :: Networking, defect, P5)

Description

[Nathan Froyd [:froydnj]]

There are a fair number of these warnings (C4244); I think most of them are fairly harmless, but there are a couple that may point to actual problems.

Once these are all flushed out, maybe we can turn on /we4244 for netwerk?

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

Here's a patch with the warnings fixed in rather a mindless way.

Comment 2

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

Comment on attachment 610265 [details] [diff] [review]
patch

Thanks for the patch.

Some but not all of these already have r+d patches in bugzilla that I haven't checked in yet (Search for assigned:[e-mail]).

I can review the patch in a few days, after I check in the fixes for the other bugs.

Comment 3

[Jason Duell]

Comment on attachment 610265 [details] [diff] [review]
patch

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

I'll let Brian take this then.

Comment 4

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

Comment on attachment 610265 [details] [diff] [review]
patch

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

::: netwerk/base/src/nsStreamTransportService.cpp
@@ +218,5 @@
>          }
>      }
>  
>      // limit amount read
> +    PRUint32 max = PRUint32(mLimit - mOffset);

Why not this?:

    PRUint64 max = mLimit - mOffset;

@@ +417,5 @@
>          }
>      }
>  
>      // limit amount written
> +    PRUint32 max = PRUint32(mLimit - mOffset);

Ditto.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4154,5 @@
>          *_retval = 0;                                          \
>          return NS_OK;                                          \
>      }                                                          \
>      *_retval = mChannelCreationTime +                          \
> +        PRTime((stamp - mChannelCreationTimestamp).ToMicroseconds());   \

Fixed in bug 736887.

@@ +4603,5 @@
>          //
>          nsresult rv =  mListener->OnDataAvailable(this,
>                                                    mListenerContext,
>                                                    input,
> +                                                  (PRUint32) mLogicalOffset,

Fixed in bug 736903. (Note that this was a bug and that the cast is not correct.)

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1040,5 @@
>          // reset to detect if next message illegally starts with continuation
>          mFragmentOpcode = kContinuation;
>        } else {
>          opcode = kContinuation;
> +        mFragmentAccumulator += PRUint32(payloadLength);

We know payloadLength fits in a 32-bit integer because above we check that it is less than the 32-bit integer mMaxMessageSize. Instead of doing all the casting below, we should rename payload to payloadLength64, and then after that check, set

   const PRUint32 payloadLength = PRUint32(payloadLength64);

This will avoid most, but not all, of the need for casts, AFAICT.

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +334,5 @@
>  
>  NS_IMETHODIMP
>  nsPartChannel::GetContentLength(PRInt32 *aContentLength)
>  {
> +    *aContentLength = PRInt32(mContentLength); // XXX truncates 64-bit value

Should nsIChannel.contentLength be clamped to 2^31, or is it OK to truncate it like this? OnDataAvailable is specified as clamping the value to 2^31 and it seems like a good idea to be consistent.

@@ +843,5 @@
>  
>      rv = mPartChannel->SetContentType(mContentType);
>      if (NS_FAILED(rv)) return rv;
>  
> +    rv = mPartChannel->SetContentLength(PRInt32(mContentLength)); // XXX Truncates 64-bit!

This is wrong. We can define a new SetContentLength64(PRInt64) function on nsPartChannel, which avoids the truncation. And...

@@ +905,5 @@
>          if (aLen == 0)
>              return NS_OK;
>      }
>  
> +    PRUint32 offset = PRUint32(mTotalSent);

We cannot truncate the offset to 32 bits. Instead, we have to clamp it, as was done in bug 736903. I recommend changing the type of offset to PRInt64, and then changing the signature of nsPartChannel::SendOnDataAvailable to take a 64-bit offset, and then have nsPartChannel::SendOnDataAvailable do the clamping.

Comment 5

[Makoto Kato [:m_kato]]

ContentLength will change by bug 536324.

Comment 6

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258