bug 1187239 - ontransportstatus SENDING_TO should not use request stream re-entrantly r=hurley r=bz
authorPatrick McManus <mcmanus@ducksong.com>
Tue, 15 Sep 2015 18:12:37 -0400
changeset 263297 2a56db89602545da44a7dd7396a683ddf1d695f1
parent 263296 c167178109febd1b9c8eac5656c710ee09c4c58d
child 263298 9244da13f5e871e49c066a796f9cf28239a8200e
push id65276
push usermcmanus@ducksong.com
push dateFri, 18 Sep 2015 19:58:20 +0000
treeherdermozilla-inbound@2a56db896025 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershurley, bz
bugs1187239
milestone43.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1187239 - ontransportstatus SENDING_TO should not use request stream re-entrantly r=hurley r=bz
dom/base/nsXMLHttpRequest.cpp
netwerk/base/nsTransportUtils.cpp
netwerk/base/nsTransportUtils.h
netwerk/protocol/file/nsFileChannel.cpp
netwerk/protocol/http/nsHttpTransaction.cpp
netwerk/protocol/http/nsHttpTransaction.h
netwerk/test/unit/test_post.js
--- a/dom/base/nsXMLHttpRequest.cpp
+++ b/dom/base/nsXMLHttpRequest.cpp
@@ -3493,17 +3493,17 @@ nsXMLHttpRequest::OnProgress(nsIRequest 
     if (lengthComputable) {
       int64_t headerSize = aProgressMax - mUploadTotal;
       loaded -= headerSize;
     }
     mUploadLengthComputable = lengthComputable;
     mUploadTransferred = loaded;
     mProgressSinceLastProgressEvent = true;
 
-    MaybeDispatchProgressEvents(false);
+    MaybeDispatchProgressEvents((mUploadTransferred == mUploadTotal));
   } else {
     mLoadLengthComputable = lengthComputable;
     mLoadTotal = lengthComputable ? aProgressMax : 0;
     mLoadTransferred = aProgress;
     // Don't dispatch progress events here. OnDataAvailable will take care
     // of that.
   }
 
--- a/netwerk/base/nsTransportUtils.cpp
+++ b/netwerk/base/nsTransportUtils.cpp
@@ -18,23 +18,21 @@ class nsTransportStatusEvent;
 
 class nsTransportEventSinkProxy : public nsITransportEventSink
 {
 public:
     NS_DECL_THREADSAFE_ISUPPORTS
     NS_DECL_NSITRANSPORTEVENTSINK
 
     nsTransportEventSinkProxy(nsITransportEventSink *sink,
-                              nsIEventTarget *target,
-                              bool coalesceAll)
+                              nsIEventTarget *target)
         : mSink(sink)
         , mTarget(target)
         , mLock("nsTransportEventSinkProxy.mLock")
         , mLastEvent(nullptr)
-        , mCoalesceAll(coalesceAll)
     {
         NS_ADDREF(mSink);
     }
 
 private:
     virtual ~nsTransportEventSinkProxy()
     {
         // our reference to mSink could be the last, so be sure to release
@@ -42,17 +40,16 @@ private:
         NS_ProxyRelease(mTarget, mSink);
     }
 
 public:
     nsITransportEventSink           *mSink;
     nsCOMPtr<nsIEventTarget>         mTarget;
     Mutex                            mLock;
     nsTransportStatusEvent          *mLastEvent;
-    bool                             mCoalesceAll;
 };
 
 class nsTransportStatusEvent : public nsRunnable
 {
 public:
     nsTransportStatusEvent(nsTransportEventSinkProxy *proxy,
                            nsITransport *transport,
                            nsresult status,
@@ -100,17 +97,17 @@ nsTransportEventSinkProxy::OnTransportSt
                                              int64_t progressMax)
 {
     nsresult rv = NS_OK;
     nsRefPtr<nsTransportStatusEvent> event;
     {
         MutexAutoLock lock(mLock);
 
         // try to coalesce events! ;-)
-        if (mLastEvent && (mCoalesceAll || mLastEvent->mStatus == status)) {
+        if (mLastEvent && (mLastEvent->mStatus == status)) {
             mLastEvent->mStatus = status;
             mLastEvent->mProgress = progress;
             mLastEvent->mProgressMax = progressMax;
         }
         else {
             event = new nsTransportStatusEvent(this, transport, status,
                                                progress, progressMax);
             if (!event)
@@ -130,17 +127,16 @@ nsTransportEventSinkProxy::OnTransportSt
     return rv;
 }
 
 //-----------------------------------------------------------------------------
 
 nsresult
 net_NewTransportEventSinkProxy(nsITransportEventSink **result,
                                nsITransportEventSink *sink,
-                               nsIEventTarget *target,
-                               bool coalesceAll)
+                               nsIEventTarget *target)
 {
-    *result = new nsTransportEventSinkProxy(sink, target, coalesceAll);
+    *result = new nsTransportEventSinkProxy(sink, target);
     if (!*result)
         return NS_ERROR_OUT_OF_MEMORY;
     NS_ADDREF(*result);
     return NS_OK;
 }
--- a/netwerk/base/nsTransportUtils.h
+++ b/netwerk/base/nsTransportUtils.h
@@ -9,21 +9,18 @@
 
 /**
  * This function returns a proxy object for a transport event sink instance.
  * The transport event sink will be called on the thread indicated by the
  * given event target.  Like events are automatically coalesced.  This means
  * that for example if the status value is the same from event to event, and
  * the previous event has not yet been delivered, then only one event will
  * be delivered.  The progress reported will be that from the second event.
- * If aCoalesceAllEvents is true, then any undelivered event will be replaced
- * with the next event if it arrives early enough.  This option should be used
- * cautiously since it can cause states to be effectively skipped.  Coalescing
- * events can help prevent a backlog of unprocessed transport events in the
- * case that the target thread is overworked.
+
+ * Coalescing events can help prevent a backlog of unprocessed transport
+ * events in the case that the target thread is overworked.
  */
 nsresult
 net_NewTransportEventSinkProxy(nsITransportEventSink **aResult,
                                nsITransportEventSink *aSink,
-                               nsIEventTarget *aTarget,
-                               bool aCoalesceAllEvents = false);
+                               nsIEventTarget *aTarget);
 
 #endif // nsTransportUtils_h__
--- a/netwerk/protocol/file/nsFileChannel.cpp
+++ b/netwerk/protocol/file/nsFileChannel.cpp
@@ -141,18 +141,18 @@ nsFileCopyEvent::Dispatch(nsIRunnable *c
                           nsIEventTarget *target)
 {
   // Use the supplied event target for all asynchronous operations.
 
   mCallback = callback;
   mCallbackTarget = target;
 
   // Build a coalescing proxy for progress events
-  nsresult rv = net_NewTransportEventSinkProxy(getter_AddRefs(mSink), sink,
-                                               target, true);
+  nsresult rv = net_NewTransportEventSinkProxy(getter_AddRefs(mSink), sink, target);
+
   if (NS_FAILED(rv))
     return rv;
 
   // Dispatch ourselves to I/O thread pool...
   nsCOMPtr<nsIEventTarget> pool =
       do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID, &rv);
   if (NS_FAILED(rv))
     return rv;
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -125,16 +125,17 @@ nsHttpTransaction::nsHttpTransaction()
     , mHttpResponseMatched(false)
     , mPreserveStream(false)
     , mDispatchedAsBlocking(false)
     , mResponseTimeoutEnabled(true)
     , mForceRestart(false)
     , mReuseOnRestart(false)
     , mContentDecoding(false)
     , mContentDecodingCheck(false)
+    , mDeferredSendProgress(false)
     , mReportedStart(false)
     , mReportedResponseHeader(false)
     , mForTakeResponseHead(nullptr)
     , mResponseHeadTaken(false)
     , mSubmittedRatePacing(false)
     , mPassedRatePacing(false)
     , mSynchronousRatePaceRequest(false)
     , mCountRecv(0)
@@ -268,23 +269,21 @@ nsHttpTransaction::Init(uint32_t caps,
         rv = httpChannelInternal->GetResponseTimeoutEnabled(
             &mResponseTimeoutEnabled);
         if (NS_WARN_IF(NS_FAILED(rv))) {
             return rv;
         }
         httpChannelInternal->GetInitialRwin(&mInitialRwin);
     }
 
-    // create transport event sink proxy. it coalesces all events if and only
-    // if the activity observer is not active. when the observer is active
-    // we need not to coalesce any events to get all expected notifications
-    // of the transaction state, necessary for correct debugging and logging.
+    // create transport event sink proxy. it coalesces consecutive
+    // events of the same status type.
     rv = net_NewTransportEventSinkProxy(getter_AddRefs(mTransportSink),
-                                        eventsink, target,
-                                        !activityDistributorActive);
+                                        eventsink, target);
+
     if (NS_FAILED(rv)) return rv;
 
     mConnInfo = cinfo;
     mCallbacks = callbacks;
     mConsumerTarget = target;
     mCaps = caps;
 
     if (requestHead->IsHead()) {
@@ -342,19 +341,27 @@ nsHttpTransaction::Init(uint32_t caps,
     // a non-owning reference to the request header data, so we MUST keep
     // mReqHeaderBuf around).
     nsCOMPtr<nsIInputStream> headers;
     rv = NS_NewByteInputStream(getter_AddRefs(headers),
                                mReqHeaderBuf.get(),
                                mReqHeaderBuf.Length());
     if (NS_FAILED(rv)) return rv;
 
-    if (requestBody) {
-        mHasRequestBody = true;
+    mHasRequestBody = !!requestBody;
+    if (mHasRequestBody) {
+        // some non standard methods set a 0 byte content-length for
+        // clarity, we can avoid doing the mulitplexed request stream for them
+        uint64_t size;
+        if (NS_SUCCEEDED(requestBody->Available(&size)) && !size) {
+            mHasRequestBody = false;
+        }
+    }
 
+    if (mHasRequestBody) {
         // wrap the headers and request body in a multiplexed input stream.
         nsCOMPtr<nsIMultiplexInputStream> multi =
             do_CreateInstance(kMultiplexInputStream, &rv);
         if (NS_FAILED(rv)) return rv;
 
         rv = multi->AppendStream(headers);
         if (NS_FAILED(rv)) return rv;
 
@@ -580,16 +587,25 @@ nsHttpTransaction::OnTransportStatus(nsI
     if (status == NS_NET_STATUS_SENDING_TO) {
         // suppress progress when only writing request headers
         if (!mHasRequestBody) {
             LOG(("nsHttpTransaction::OnTransportStatus %p "
                  "SENDING_TO without request body\n", this));
             return;
         }
 
+        if (mReader) {
+            // A mRequestStream method is on the stack - wait.
+            LOG(("nsHttpTransaction::OnSocketStatus [this=%p] "
+                 "Skipping Re-Entrant NS_NET_STATUS_SENDING_TO\n", this));
+            // its ok to coalesce several of these into one deferred event
+            mDeferredSendProgress = true;
+            return;
+        }
+
         nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mRequestStream);
         if (!seekable) {
             LOG(("nsHttpTransaction::OnTransportStatus %p "
                  "SENDING_TO without seekable request stream\n", this));
             progress = 0;
         } else {
             int64_t prog = 0;
             seekable->Tell(&prog);
@@ -675,21 +691,28 @@ nsHttpTransaction::ReadSegments(nsAHttpS
         return mStatus;
     }
 
     if (!mConnected) {
         mConnected = true;
         mConnection->GetSecurityInfo(getter_AddRefs(mSecurityInfo));
     }
 
+    mDeferredSendProgress = false;
     mReader = reader;
-
     nsresult rv = mRequestStream->ReadSegments(ReadRequestSegment, this, count, countRead);
+    mReader = nullptr;
 
-    mReader = nullptr;
+    if (mDeferredSendProgress && mConnection && mConnection->Transport()) {
+        // to avoid using mRequestStream concurrently, OnTransportStatus()
+        // did not report upload status off the ReadSegments() stack from nsSocketTransport
+        // do it now.
+        OnTransportStatus(mConnection->Transport(), NS_NET_STATUS_SENDING_TO, 0);
+    }
+    mDeferredSendProgress = false;
 
     if (mForceRestart) {
         // The forceRestart condition was dealt with on the stack, but it did not
         // clear the flag because nsPipe in the readsegment stack clears out
         // return codes, so we need to use the flag here as a cue to return ERETARGETED
         if (NS_SUCCEEDED(rv)) {
             rv = NS_BINDING_RETARGETED;
         }
--- a/netwerk/protocol/http/nsHttpTransaction.h
+++ b/netwerk/protocol/http/nsHttpTransaction.h
@@ -295,16 +295,17 @@ private:
     bool                            mHttpResponseMatched;
     bool                            mPreserveStream;
     bool                            mDispatchedAsBlocking;
     bool                            mResponseTimeoutEnabled;
     bool                            mForceRestart;
     bool                            mReuseOnRestart;
     bool                            mContentDecoding;
     bool                            mContentDecodingCheck;
+    bool                            mDeferredSendProgress;
 
     // mClosed           := transaction has been explicitly closed
     // mTransactionDone  := transaction ran to completion or was interrupted
     // mResponseComplete := transaction ran to completion
 
     // For Restart-In-Progress Functionality
     bool                            mReportedStart;
     bool                            mReportedResponseHeader;
--- a/netwerk/test/unit/test_post.js
+++ b/netwerk/test/unit/test_post.js
@@ -20,16 +20,41 @@ var teststring1 = "--" + BOUNDARY + "\r\
                 + "0123456789\r\n"
                 + "--" + BOUNDARY + "\r\n"
                 + "Content-Disposition: form-data; name=\"files\"; filename=\"" + testfile.leafName + "\"\r\n"
                 + "Content-Type: application/octet-stream\r\n"
                 + "Content-Length: " + testfile.fileSize + "\r\n\r\n";
 var teststring2 = "--" + BOUNDARY + "--\r\n";
 
 const BUFFERSIZE = 4096;
+var correctOnProgress = false;
+
+var listenerCallback = {
+  QueryInterface: function (iid) {
+    if (iid.equals(Ci.nsISupports) ||
+	iid.equals(Ci.nsIProgressEventSink))
+      return this;
+    throw Cr.NS_ERROR_NO_INTERFACE;
+  },
+
+  getInterface: function (iid) {
+    if (iid.equals(Ci.nsIProgressEventSink))
+      return this;
+    throw Cr.NS_ERROR_NO_INTERFACE;
+  },
+
+  onProgress: function (request, context, progress, progressMax) {
+    // this works because the response is 0 bytes and does not trigger onprogress
+    if (progress === progressMax) {
+      correctOnProgress = true;
+    }
+  },
+
+    onStatus: function (request, context, status, statusArg) { },
+};
 
 function run_test() {
   var sstream1 = Cc["@mozilla.org/io/string-input-stream;1"].
                    createInstance(Ci.nsIStringInputStream);
   sstream1.data = teststring1;
 
   var fstream = Cc["@mozilla.org/network/file-input-stream;1"].
                   createInstance(Ci.nsIFileInputStream);
@@ -58,19 +83,18 @@ function run_test() {
   httpserver.registerPathHandler(testpath, serverHandler);
   httpserver.start(-1);
 
   var channel = setupChannel(testpath);
 
   channel.QueryInterface(Ci.nsIUploadChannel)
          .setUploadStream(mime, "", mime.available());
   channel.requestMethod = "POST";
-
+  channel.notificationCallbacks = listenerCallback;
   channel.asyncOpen(new ChannelListener(checkRequest, channel), null);
-
   do_test_pending();
 }
 
 function setupChannel(path) {
   var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
   return chan = ios.newChannel2(URL + path,
                                "",
                                null,
@@ -94,10 +118,11 @@ function serverHandler(metadata, respons
 
   do_check_eq(teststring1 +
 	      read_stream(testfile_stream, testfile_stream.available()) +
 	      teststring2,
 	      data);
 }
 
 function checkRequest(request, data, context) {
+  do_check_true(correctOnProgress);
   httpserver.stop(do_test_finished);
 }