Last Comment Bug 784912 - nsIStreamListener.onDataAvailable should handle 64-bit offset
: nsIStreamListener.onDataAvailable should handle 64-bit offset
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
Depends on: 789178
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-22 20:29 PDT by Makoto Kato [:m_kato]
Modified: 2012-09-10 21:02 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
for m-c (118.54 KB, patch)
2012-08-26 18:10 PDT, Makoto Kato [:m_kato]
michal.novotny: review+
Details | Diff | Splinter Review
for c-c (17.07 KB, patch)
2012-08-26 18:14 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
for c-c (18.62 KB, patch)
2012-08-28 22:44 PDT, Makoto Kato [:m_kato]
standard8: review-
Details | Diff | Splinter Review
for c-c fix v2 (32.90 KB, patch)
2012-08-30 03:51 PDT, Makoto Kato [:m_kato]
standard8: review+
Details | Diff | Splinter Review
comm-central Bustage fix. (1.12 KB, patch)
2012-09-06 01:45 PDT, Philip Chee
philip.chee: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2012-08-22 20:29:08 PDT
nsIStreamListener is unfrozen interface now.

The offset param of onDataAvailable is still 32-bit, so we should use 64-bit offset instead of.
Comment 1 Josh Aas 2012-08-23 01:50:51 PDT
Are you planning to work on this Makoto? If not I may be able to do it.
Comment 2 Makoto Kato [:m_kato] 2012-08-23 01:58:41 PDT
(In reply to Josh Aas (Mozilla Corporation) from comment #1)
> Are you planning to work on this Makoto? If not I may be able to do it.

I already test this now.
https://tbpl.mozilla.org/?tree=Try&pusher=m_kato@ga2.so-net.ne.jp

Also, we need a patch for comm-central.
Comment 3 Makoto Kato [:m_kato] 2012-08-23 01:59:20 PDT
aah, try server url is https://tbpl.mozilla.org/?tree=Try&rev=324dba6644b0
Comment 4 Makoto Kato [:m_kato] 2012-08-26 18:10:15 PDT
Created attachment 655479 [details] [diff] [review]
for m-c
Comment 5 Makoto Kato [:m_kato] 2012-08-26 18:13:46 PDT
Comment on attachment 655479 [details] [diff] [review]
for m-c

Should I separate patch (network and other)?
Comment 6 Makoto Kato [:m_kato] 2012-08-26 18:14:21 PDT
Created attachment 655481 [details] [diff] [review]
for c-c
Comment 7 Christian :Biesinger (don't email me, ping me on IRC) 2012-08-27 10:13:01 PDT
Comment on attachment 655479 [details] [diff] [review]
for m-c

any specific reason to keep count a 32-bit integer, while you're touching this anyway?
Comment 8 Michal Novotny (:michal) 2012-08-28 16:55:47 PDT
Comment on attachment 655479 [details] [diff] [review]
for m-c

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

The patch looks good, r+ with few nits fixed.

::: content/base/src/nsDOMFileReader.cpp
@@ +299,5 @@
>    if (mDataFormat == FILE_AS_BINARY) {
>      //Continuously update our binary string as data comes in
>      NS_ASSERTION(mResult.Length() == aOffset,
>                   "unexpected mResult length");
>      uint32_t oldLen = mResult.Length();

I think here is a bug that actually has nothing to do with your patch. If (oldLen + aCount) > 0xFFFFFFFF then it overflows when passed to GetMutableData() since the argument is size_type which is uint32_t, so the string mResult is shrinked. ReadFuncBinaryString() then writes outside the buffer.

::: modules/libjar/zipwriter/src/nsDeflateConverter.cpp
@@ +101,5 @@
>  }
>  
>  /* void onDataAvailable (in nsIRequest aRequest, in nsISupports aContext,
>   *                       in nsIInputStream aInputStream,
>   *                       in unsigned long aOffset, in unsigned long aCount); */

in unsigned long long aOffset

::: modules/libjar/zipwriter/src/nsZipDataStream.cpp
@@ +60,5 @@
>  }
>  
>  /* void onDataAvailable (in nsIRequest aRequest, in nsISupports aContext,
>   *                       in nsIInputStream aInputStream,
>   *                       in unsigned long aOffset, in unsigned long aCount); */

in unsigned long long aOffset

::: netwerk/streamconv/test/TestStreamConv.cpp
@@ +58,5 @@
>      EndListener() {};
>  
>      // nsIStreamListener method
>      NS_IMETHOD OnDataAvailable(nsIRequest* request, nsISupports *ctxt, nsIInputStream *inStr, 
> +                               uint64_t sourceOffset, uint32_t count)

Pass offset instead of saturated(offset) in SendData() below.
Comment 9 Makoto Kato [:m_kato] 2012-08-28 22:44:38 PDT
Created attachment 656337 [details] [diff] [review]
for c-c
Comment 10 Mark Banner (:standard8) 2012-08-29 10:53:12 PDT
Comment on attachment 656337 [details] [diff] [review]
for c-c

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

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +349,5 @@
>  }
>  
>  // Whenever data arrives from the connection, core netlib notifices the protocol by calling
>  // OnDataAvailable. We then read and process the incoming data from the input stream.
> +NS_IMETHODIMP nsMsgProtocol::OnDataAvailable(nsIRequest *request, nsISupports *ctxt, nsIInputStream *inStr, uint64_t sourceOffset, uint32_t count)

For some reason, the build isn't failing or warning, but I think you should also update ProcessProtocolState & friends to take a uint64_t sourceOffset argument as well. Otherwise we'll be collapsing a 64 down into a 32 bit.

::: mailnews/compose/src/nsURLFetcher.cpp
@@ +474,5 @@
>  
>  /** nsIStreamListener methods **/
>  
>  /* void onDataAvailable (in nsIRequest request, in nsISupports ctxt, in nsIInputStream inStr, in unsigned long sourceOffset, in unsigned long count); */
> +NS_IMETHODIMP nsURLFetcherStreamConsumer::OnDataAvailable(nsIRequest *aRequest, nsISupports *ctxt, nsIInputStream *inStr, uint64_t sourceOffset, uint32_t count)

nit: either update the comment, or preferably, just drop it.

::: mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
@@ +1076,5 @@
>      return NS_OK;
>  }
>  
>  /* void onDataAvailable (in nsIRequest aRequest, in nsISupports aContext, in nsIInputStream aInputStream, in unsigned long aOffset, in unsigned long aCount); */
> +NS_IMETHODIMP TokenStreamListener::OnDataAvailable(nsIRequest *aRequest, nsISupports *aContext, nsIInputStream *aInputStream, uint64_t aOffset, uint32_t aCount)

nit: either update the comment, or preferably, just drop it.
Comment 11 Makoto Kato [:m_kato] 2012-08-30 03:51:26 PDT
Created attachment 656808 [details] [diff] [review]
for c-c fix v2
Comment 12 Mark Banner (:standard8) 2012-08-31 03:45:13 PDT
Comment on attachment 656808 [details] [diff] [review]
for c-c fix v2

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

Looks good, thanks.
Comment 15 Philip Chee 2012-09-06 01:38:55 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1346906361.1346913493.32336.gz#err0
e:/builds/slave/comm-cen-trunk-w32/build/suite/feeds/src/nsFeedSniffer.cpp(342) : error C2511: 'nsresult nsFeedSniffer::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,uint32_t,uint32_t)' : overloaded member function not found in 'nsFeedSniffer'
        e:\builds\slave\comm-cen-trunk-w32\build\suite\feeds\src\nsFeedSniffer.h(18) : see declaration of 'nsFeedSniffer'
Comment 16 Philip Chee 2012-09-06 01:45:58 PDT
Created attachment 658817 [details] [diff] [review]
comm-central Bustage fix.

I checked in a bustage fix r=me.
Pushed:
http://hg.mozilla.org/comm-central/rev/8ad14cba8288

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