Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side. r=mayhemer draft
authorShih-Chiang Chien <schien@mozilla.com>
Thu, 02 Feb 2017 15:20:04 +0800
changeset 479800 da54f269b4b186ad784b2351f39829922f08db89
parent 479141 20a8536b0bfac74389d3a57bd8dd957d98779ce1
child 479801 f1a882bac48f03b8fadda430f84027c4c3ace010
push id44361
push userschien@mozilla.com
push dateTue, 07 Feb 2017 08:57:19 +0000
reviewersmayhemer
bugs1325915
milestone54.0a1
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side. r=mayhemer MozReview-Commit-ID: 41qlyHKMSFm
netwerk/protocol/http/HttpChannelChild.cpp
netwerk/protocol/http/HttpChannelChild.h
netwerk/protocol/http/HttpChannelParent.cpp
netwerk/protocol/http/HttpChannelParent.h
netwerk/protocol/http/PHttpChannel.ipdl
netwerk/protocol/http/nsHttpChannel.cpp
netwerk/protocol/http/nsHttpChannel.h
--- a/netwerk/protocol/http/HttpChannelChild.cpp
+++ b/netwerk/protocol/http/HttpChannelChild.cpp
@@ -586,62 +586,53 @@ HttpChannelChild::DoOnStartRequest(nsIRe
 }
 
 class TransportAndDataEvent : public ChannelEvent
 {
  public:
   TransportAndDataEvent(HttpChannelChild* child,
                         const nsresult& channelStatus,
                         const nsresult& transportStatus,
-                        const uint64_t& progress,
-                        const uint64_t& progressMax,
                         const nsCString& data,
                         const uint64_t& offset,
                         const uint32_t& count)
   : mChild(child)
   , mChannelStatus(channelStatus)
   , mTransportStatus(transportStatus)
-  , mProgress(progress)
-  , mProgressMax(progressMax)
   , mData(data)
   , mOffset(offset)
   , mCount(count) {}
 
   void Run()
   {
-    mChild->OnTransportAndData(mChannelStatus, mTransportStatus, mProgress,
-                               mProgressMax, mOffset, mCount, mData);
+    mChild->OnTransportAndData(mChannelStatus, mTransportStatus,
+                               mOffset, mCount, mData);
   }
  private:
   HttpChannelChild* mChild;
   nsresult mChannelStatus;
   nsresult mTransportStatus;
-  uint64_t mProgress;
-  uint64_t mProgressMax;
   nsCString mData;
   uint64_t mOffset;
   uint32_t mCount;
 };
 
 mozilla::ipc::IPCResult
 HttpChannelChild::RecvOnTransportAndData(const nsresult& channelStatus,
                                          const nsresult& transportStatus,
-                                         const uint64_t& progress,
-                                         const uint64_t& progressMax,
                                          const uint64_t& offset,
                                          const uint32_t& count,
                                          const nsCString& data)
 {
   LOG(("HttpChannelChild::RecvOnTransportAndData [this=%p]\n", this));
   MOZ_RELEASE_ASSERT(!mFlushedForDiversion,
                      "Should not be receiving any more callbacks from parent!");
 
   mEventQ->RunOrEnqueue(new TransportAndDataEvent(this, channelStatus,
-                                                  transportStatus, progress,
-                                                  progressMax, data, offset,
+                                                  transportStatus, data, offset,
                                                   count),
                         mDivertingToParent);
   return IPC_OK();
 }
 
 class MaybeDivertOnDataHttpEvent : public ChannelEvent
 {
  public:
@@ -676,18 +667,16 @@ HttpChannelChild::MaybeDivertOnData(cons
   if (mDivertingToParent) {
     SendDivertOnDataAvailable(data, offset, count);
   }
 }
 
 void
 HttpChannelChild::OnTransportAndData(const nsresult& channelStatus,
                                      const nsresult& transportStatus,
-                                     const uint64_t progress,
-                                     const uint64_t& progressMax,
                                      const uint64_t& offset,
                                      const uint32_t& count,
                                      const nsCString& data)
 {
   LOG(("HttpChannelChild::OnTransportAndData [this=%p]\n", this));
 
   if (!mCanceled && NS_SUCCEEDED(mStatus)) {
     mStatus = channelStatus;
@@ -712,16 +701,19 @@ HttpChannelChild::OnTransportAndData(con
       MakeUnique<MaybeDivertOnDataHttpEvent>(this, data, offset, count));
   }
 
   // Hold queue lock throughout all three calls, else we might process a later
   // necko msg in between them.
   AutoEventEnqueuer ensureSerialDispatch(mEventQ);
 
   DoOnStatus(this, transportStatus);
+
+  const int64_t progressMax = mResponseHead->ContentLength();
+  const int64_t progress = offset + count;
   DoOnProgress(this, progress, progressMax);
 
   // OnDataAvailable
   //
   // NOTE: the OnDataAvailable contract requires the client to read all the data
   // in the inputstream.  This code relies on that ('data' will go away after
   // this function).  Apparently the previous, non-e10s behavior was to actually
   // support only reading part of the data, allowing later calls to read the
--- a/netwerk/protocol/http/HttpChannelChild.h
+++ b/netwerk/protocol/http/HttpChannelChild.h
@@ -121,18 +121,16 @@ protected:
                                              const nsCString& securityInfoSerialization,
                                              const NetAddr& selfAddr,
                                              const NetAddr& peerAddr,
                                              const int16_t& redirectCount,
                                              const uint32_t& cacheKey,
                                              const nsCString& altDataType) override;
   mozilla::ipc::IPCResult RecvOnTransportAndData(const nsresult& channelStatus,
                                                  const nsresult& status,
-                                                 const uint64_t& progress,
-                                                 const uint64_t& progressMax,
                                                  const uint64_t& offset,
                                                  const uint32_t& count,
                                                  const nsCString& data) override;
   mozilla::ipc::IPCResult RecvOnStopRequest(const nsresult& statusCode, const ResourceTimingStruct& timing) override;
   mozilla::ipc::IPCResult RecvOnProgress(const int64_t& progress, const int64_t& progressMax) override;
   mozilla::ipc::IPCResult RecvOnStatus(const nsresult& status) override;
   mozilla::ipc::IPCResult RecvFailedAsyncOpen(const nsresult& status) override;
   mozilla::ipc::IPCResult RecvRedirect1Begin(const uint32_t& registrarId,
@@ -304,18 +302,16 @@ private:
                       const NetAddr& peerAddr,
                       const uint32_t& cacheKey,
                       const nsCString& altDataType);
   void MaybeDivertOnData(const nsCString& data,
                          const uint64_t& offset,
                          const uint32_t& count);
   void OnTransportAndData(const nsresult& channelStatus,
                           const nsresult& status,
-                          const uint64_t progress,
-                          const uint64_t& progressMax,
                           const uint64_t& offset,
                           const uint32_t& count,
                           const nsCString& data);
   void OnStopRequest(const nsresult& channelStatus, const ResourceTimingStruct& timing);
   void MaybeDivertOnStop(const nsresult& aChannelStatus);
   void OnProgress(const int64_t& progress, const int64_t& progressMax);
   void OnStatus(const nsresult& status);
   void FailedAsyncOpen(const nsresult& status);
--- a/netwerk/protocol/http/HttpChannelParent.cpp
+++ b/netwerk/protocol/http/HttpChannelParent.cpp
@@ -47,19 +47,17 @@ using namespace mozilla::ipc;
 
 namespace mozilla {
 namespace net {
 
 HttpChannelParent::HttpChannelParent(const PBrowserOrId& iframeEmbedding,
                                      nsILoadContext* aLoadContext,
                                      PBOverrideStatus aOverrideStatus)
   : mIPCClosed(false)
-  , mStoredStatus(NS_OK)
-  , mStoredProgress(0)
-  , mStoredProgressMax(0)
+  , mIgnoreProgress(false)
   , mSentRedirect1Begin(false)
   , mSentRedirect1BeginFailed(false)
   , mReceivedRedirect2Verify(false)
   , mPBOverride(aOverrideStatus)
   , mLoadContext(aLoadContext)
   , mStatus(NS_OK)
   , mPendingDiversion(false)
   , mDivertingFromChild(false)
@@ -1252,37 +1250,36 @@ HttpChannelParent::OnDataAvailable(nsIRe
        this, aRequest));
 
   MOZ_RELEASE_ASSERT(!mDivertingFromChild,
     "Cannot call OnDataAvailable if diverting is set!");
 
   nsresult channelStatus = NS_OK;
   mChannel->GetStatus(&channelStatus);
 
+  nsresult transportStatus =
+    (mChannel->IsReadingFromCache()) ? NS_NET_STATUS_READING
+                                     : NS_NET_STATUS_RECEIVING_FROM;
+
   static uint32_t const kCopyChunkSize = 128 * 1024;
   uint32_t toRead = std::min<uint32_t>(aCount, kCopyChunkSize);
 
   nsCString data;
   if (!data.SetCapacity(toRead, fallible)) {
     LOG(("  out of memory!"));
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   while (aCount) {
     nsresult rv = NS_ReadInputStreamToString(aInputStream, data, toRead);
     if (NS_FAILED(rv)) {
       return rv;
     }
 
-    // OnDataAvailable is always preceded by OnStatus/OnProgress calls that set
-    // mStoredStatus/mStoredProgress(Max) to appropriate values, unless
-    // LOAD_BACKGROUND set.  In that case, they'll have garbage values, but
-    // child doesn't use them.
-    if (mIPCClosed || !SendOnTransportAndData(channelStatus, mStoredStatus,
-                                              mStoredProgress, mStoredProgressMax,
+    if (mIPCClosed || !SendOnTransportAndData(channelStatus, transportStatus,
                                               aOffset, toRead, data)) {
       return NS_ERROR_UNEXPECTED;
     }
 
     aOffset += toRead;
     aCount -= toRead;
     toRead = std::min<uint32_t>(aCount, kCopyChunkSize);
   }
@@ -1295,50 +1292,53 @@ HttpChannelParent::OnDataAvailable(nsIRe
 //-----------------------------------------------------------------------------
 
 NS_IMETHODIMP
 HttpChannelParent::OnProgress(nsIRequest *aRequest,
                               nsISupports *aContext,
                               int64_t aProgress,
                               int64_t aProgressMax)
 {
-  // OnStatus has always just set mStoredStatus. If it indicates this precedes
-  // OnDataAvailable, store and ODA will send to child.
-  if (mStoredStatus == NS_NET_STATUS_RECEIVING_FROM ||
-      mStoredStatus == NS_NET_STATUS_READING)
-  {
-    mStoredProgress = aProgress;
-    mStoredProgressMax = aProgressMax;
-  } else {
-    // Send OnProgress events to the child for data upload progress notifications
-    // (i.e. status == NS_NET_STATUS_SENDING_TO) or if the channel has
-    // LOAD_BACKGROUND set.
-    if (mIPCClosed || !SendOnProgress(aProgress, aProgressMax))
-      return NS_ERROR_UNEXPECTED;
+  // If it indicates this precedes OnDataAvailable, child can derive the value in ODA.
+  if (mIgnoreProgress) {
+    mIgnoreProgress = false;
+    return NS_OK;
+  }
+
+  // Send OnProgress events to the child for data upload progress notifications
+  // (i.e. status == NS_NET_STATUS_SENDING_TO) or if the channel has
+  // LOAD_BACKGROUND set.
+  if (mIPCClosed || !SendOnProgress(aProgress, aProgressMax)) {
+    return NS_ERROR_UNEXPECTED;
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 HttpChannelParent::OnStatus(nsIRequest *aRequest,
                             nsISupports *aContext,
                             nsresult aStatus,
                             const char16_t *aStatusArg)
 {
-  // If this precedes OnDataAvailable, store and ODA will send to child.
+  // If this precedes OnDataAvailable, transportStatus will be derived in ODA.
   if (aStatus == NS_NET_STATUS_RECEIVING_FROM ||
-      aStatus == NS_NET_STATUS_READING)
-  {
-    mStoredStatus = aStatus;
+      aStatus == NS_NET_STATUS_READING) {
+    // The transport status and progress generated by ODA will be coalesced
+    // into one IPC message. Therefore, we can ignore the next OnProgress event
+    // since it is generated by ODA as well.
+    mIgnoreProgress = true;
     return NS_OK;
   }
+
   // Otherwise, send to child now
-  if (mIPCClosed || !SendOnStatus(aStatus))
+  if (mIPCClosed || !SendOnStatus(aStatus)) {
     return NS_ERROR_UNEXPECTED;
+  }
+
   return NS_OK;
 }
 
 //-----------------------------------------------------------------------------
 // HttpChannelParent::nsIParentChannel
 //-----------------------------------------------------------------------------
 
 NS_IMETHODIMP
--- a/netwerk/protocol/http/HttpChannelParent.h
+++ b/netwerk/protocol/http/HttpChannelParent.h
@@ -216,21 +216,20 @@ private:
   nsCOMPtr<nsIAssociatedContentSecurity>  mAssociatedContentSecurity;
   bool mIPCClosed;                // PHttpChannel actor has been Closed()
 
   nsCOMPtr<nsIChannel> mRedirectChannel;
   nsCOMPtr<nsIAsyncVerifyRedirectCallback> mRedirectCallback;
 
   nsAutoPtr<class nsHttpChannel::OfflineCacheEntryAsForeignMarker> mOfflineForeignMarker;
 
-  // state for combining OnStatus/OnProgress with OnDataAvailable
-  // into one IPDL call to child.
-  nsresult mStoredStatus;
-  int64_t mStoredProgress;
-  int64_t mStoredProgressMax;
+  // OnStatus is always called before OnProgress.
+  // Set true in OnStatus if next OnProgress can be ignored
+  // since the information can be recontructed from ODA.
+  bool mIgnoreProgress              : 1;
 
   bool mSentRedirect1Begin          : 1;
   bool mSentRedirect1BeginFailed    : 1;
   bool mReceivedRedirect2Verify     : 1;
 
   PBOverrideStatus mPBOverride;
 
   nsCOMPtr<nsILoadContext> mLoadContext;
--- a/netwerk/protocol/http/PHttpChannel.ipdl
+++ b/netwerk/protocol/http/PHttpChannel.ipdl
@@ -108,18 +108,16 @@ child:
                        int16_t             redirectCount,
                        uint32_t            cacheKey,
                        nsCString           altDataType);
 
   // Combines a single OnDataAvailable and its associated OnProgress &
   // OnStatus calls into one IPDL message
   async OnTransportAndData(nsresult  channelStatus,
                            nsresult  transportStatus,
-                           uint64_t  progress,
-                           uint64_t  progressMax,
                            uint64_t  offset,
                            uint32_t  count,
                            nsCString data);
 
   async OnStopRequest(nsresult channelStatus, ResourceTimingStruct timing);
 
   async OnProgress(int64_t progress, int64_t progressMax);
 
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -272,16 +272,17 @@ nsHttpChannel::nsHttpChannel()
     , mIsPartialRequest(0)
     , mHasAutoRedirectVetoNotifier(0)
     , mPinCacheContent(0)
     , mIsCorsPreflightDone(0)
     , mStronglyFramed(false)
     , mPushedStream(nullptr)
     , mLocalBlocklist(false)
     , mWarningReporter(nullptr)
+    , mIsReadingFromCache(false)
     , mDidReval(false)
 {
     LOG(("Creating nsHttpChannel [this=%p]\n", this));
     mChannelCreationTime = PR_Now();
     mChannelCreationTimestamp = TimeStamp::Now();
 }
 
 nsHttpChannel::~nsHttpChannel()
@@ -6938,16 +6939,18 @@ nsHttpChannel::OnDataAvailable(nsIReques
     MOZ_ASSERT(!(mCachedContentIsPartial && (request == mTransactionPump)),
                "transaction pump not suspended");
 
     if (mAuthRetryPending || (request == mTransactionPump && mTransactionReplaced)) {
         uint32_t n;
         return input->ReadSegments(NS_DiscardSegment, nullptr, count, &n);
     }
 
+    mIsReadingFromCache = (request == mCachePump);
+
     if (mListener) {
         //
         // synthesize transport progress event.  we do this here since we want
         // to delay OnProgress events until we start streaming data.  this is
         // crucially important since it impacts the lock icon (see bug 240053).
         //
         nsresult transportStatus;
         if (request == mCachePump)
--- a/netwerk/protocol/http/nsHttpChannel.h
+++ b/netwerk/protocol/http/nsHttpChannel.h
@@ -262,16 +262,20 @@ public: /* internal necko use only */
       uint32_t mKeep : 2;
     };
 
     void MarkIntercepted();
     NS_IMETHOD GetResponseSynthesized(bool* aSynthesized) override;
     bool AwaitingCacheCallbacks();
     void SetCouldBeSynthesized();
 
+    // Return true if the latest ODA is invoked by mCachePump.
+    // Should only be called on the same thread as ODA.
+    bool IsReadingFromCache() const { return mIsReadingFromCache; }
+
 private: // used for alternate service validation
     RefPtr<TransactionObserver> mTransactionObserver;
 public:
     void SetConnectionInfo(nsHttpConnectionInfo *); // clones the argument
     void SetTransactionObserver(TransactionObserver *arg) { mTransactionObserver = arg; }
     TransactionObserver *GetTransactionObserver() { return mTransactionObserver; }
 
 protected:
@@ -592,16 +596,20 @@ private:
     void PopRedirectAsyncFunc(nsContinueRedirectionFunc func);
 
     nsCString mUsername;
 
     // If non-null, warnings should be reported to this object.
     HttpChannelSecurityWarningReporter* mWarningReporter;
 
     RefPtr<ADivertableParentChannel> mParentChannel;
+
+    // True if the channel is reading from cache.
+    Atomic<bool> mIsReadingFromCache;
+
 protected:
     virtual void DoNotifyListenerCleanup() override;
 
 private: // cache telemetry
     bool mDidReval;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(nsHttpChannel, NS_HTTPCHANNEL_IID)