bug 1187239 - ontransportstatus SENDING_TO should not use request stream re-entrantly r=hurley r=bz

dom/base/nsXMLHttpRequest.cpp

@@ -3498,7 +3498,7 @@ nsXMLHttpRequest::OnProgress(nsIRequest *aRequest, nsISupports *aContext, int64_
     mUploadTransferred = loaded;
     mProgressSinceLastProgressEvent = true;
 
-    MaybeDispatchProgressEvents(false);
+    MaybeDispatchProgressEvents((mUploadTransferred == mUploadTotal));
   } else {
     mLoadLengthComputable = lengthComputable;
     mLoadTotal = lengthComputable ? aProgressMax : 0;

netwerk/base/nsTransportUtils.cpp

@@ -23,13 +23,11 @@ public:
     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);
     }
@@ -47,7 +45,6 @@ public:
     nsCOMPtr<nsIEventTarget>         mTarget;
     Mutex                            mLock;
     nsTransportStatusEvent          *mLastEvent;
-    bool                             mCoalesceAll;
 };
 
 class nsTransportStatusEvent : public nsRunnable
@@ -105,7 +102,7 @@ nsTransportEventSinkProxy::OnTransportStatus(nsITransport *transport,
         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;
@@ -135,10 +132,9 @@ nsTransportEventSinkProxy::OnTransportStatus(nsITransport *transport,
 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);

netwerk/base/nsTransportUtils.h

@@ -14,16 +14,13 @@
  * 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__

netwerk/protocol/file/nsFileChannel.cpp

@@ -146,8 +146,8 @@ nsFileCopyEvent::Dispatch(nsIRunnable *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;
 

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -130,6 +130,7 @@ nsHttpTransaction::nsHttpTransaction()
     , mReuseOnRestart(false)
     , mContentDecoding(false)
     , mContentDecodingCheck(false)
+    , mDeferredSendProgress(false)
     , mReportedStart(false)
     , mReportedResponseHeader(false)
     , mForTakeResponseHead(nullptr)
@@ -273,13 +274,11 @@ nsHttpTransaction::Init(uint32_t caps,
         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;
@@ -347,9 +346,17 @@ nsHttpTransaction::Init(uint32_t caps,
                                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);
@@ -585,6 +592,15 @@ nsHttpTransaction::OnTransportStatus(nsITransport* transport,
             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 "
@@ -680,12 +696,19 @@ nsHttpTransaction::ReadSegments(nsAHttpSegmentReader *reader,
         mConnection->GetSecurityInfo(getter_AddRefs(mSecurityInfo));
     }
 
+    mDeferredSendProgress = false;
     mReader = reader;
-
     nsresult rv = mRequestStream->ReadSegments(ReadRequestSegment, this, count, countRead);
-
     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

netwerk/protocol/http/nsHttpTransaction.h

@@ -300,6 +300,7 @@ private:
     bool                            mReuseOnRestart;
     bool                            mContentDecoding;
     bool                            mContentDecodingCheck;
+    bool                            mDeferredSendProgress;
 
     // mClosed           := transaction has been explicitly closed
     // mTransactionDone  := transaction ran to completion or was interrupted

netwerk/test/unit/test_post.js

@@ -25,6 +25,31 @@ var teststring1 = "--" + BOUNDARY + "\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"].
@@ -63,9 +88,8 @@ function run_test() {
   channel.QueryInterface(Ci.nsIUploadChannel)
          .setUploadStream(mime, "", mime.available());
   channel.requestMethod = "POST";
-
+  channel.notificationCallbacks = listenerCallback;
   channel.asyncOpen(new ChannelListener(checkRequest, channel), null);
-
   do_test_pending();
 }
 
@@ -99,5 +123,6 @@ function serverHandler(metadata, response) {
 }
 
 function checkRequest(request, data, context) {
+  do_check_true(correctOnProgress);
   httpserver.stop(do_test_finished);
 }