Bug 1267474 - only revalidate strongly framed responses 1/3 r=mayhemer

--HG--
extra : rebase_source : 9e00df6a46684032e62da6e2e607a7f65ff63d87

netwerk/protocol/http/Http2Session.cpp

@@ -3314,6 +3314,7 @@ Http2Session::SetNeedsCleanup()
 
   // This will result in Close() being called
   MOZ_ASSERT(!mNeedsCleanup, "mNeedsCleanup unexpectedly set");
+  mInputFrameDataStream->SetResponseIsComplete();
   mNeedsCleanup = mInputFrameDataStream;
   ResetDownstreamState();
 }

netwerk/protocol/http/Http2Stream.cpp

@@ -1082,6 +1082,15 @@ Http2Stream::Close(nsresult reason)
   mTransaction->Close(reason);
 }
 
+void
+Http2Stream::SetResponseIsComplete()
+{
+  nsHttpTransaction *trans = mTransaction->QueryHttpTransaction();
+  if (trans) {
+    trans->SetResponseIsComplete();
+  }
+}
+
 void
 Http2Stream::SetAllHeadersReceived()
 {

netwerk/protocol/http/Http2Stream.h

@@ -80,6 +80,7 @@ public:
   }
 
   void Close(nsresult reason);
+  void SetResponseIsComplete();
 
   void SetRecvdFin(bool aStatus);
   bool RecvdFin() { return mRecvdFin; }

netwerk/protocol/http/SpdySession31.cpp

@@ -2635,6 +2635,7 @@ SpdySession31::SetNeedsCleanup()
 
   // This will result in Close() being called
   MOZ_ASSERT(!mNeedsCleanup, "mNeedsCleanup unexpectedly set");
+  mInputFrameDataStream->SetResponseIsComplete();
   mNeedsCleanup = mInputFrameDataStream;
   ResetDownstreamState();
 }

netwerk/protocol/http/SpdyStream31.cpp

@@ -1476,6 +1476,15 @@ SpdyStream31::Close(nsresult reason)
   mTransaction->Close(reason);
 }
 
+void
+SpdyStream31::SetResponseIsComplete()
+{
+  nsHttpTransaction *trans = mTransaction->QueryHttpTransaction();
+  if (trans) {
+    trans->SetResponseIsComplete();
+  }
+}
+
 void
 SpdyStream31::UpdateRemoteWindow(int32_t delta)
 {

netwerk/protocol/http/SpdyStream31.h

@@ -49,6 +49,7 @@ public:
   }
 
   void Close(nsresult reason);
+  void SetResponseIsComplete();
 
   void SetRecvdFin(bool aStatus) { mRecvdFin = aStatus ? 1 : 0; }
   bool RecvdFin() { return mRecvdFin; }

netwerk/protocol/http/nsHttpChannel.cpp

@@ -260,6 +260,7 @@ nsHttpChannel::nsHttpChannel()
     , mPinCacheContent(0)
     , mIsPackagedAppResource(0)
     , mIsCorsPreflightDone(0)
+    , mStronglyFramed(false)
     , mPushedStream(nullptr)
     , mLocalBlocklist(false)
     , mWarningReporter(nullptr)
@@ -3558,6 +3559,12 @@ nsHttpChannel::OnCacheEntryCheck(nsICacheEntry* entry, nsIApplicationCache* appC
     mCachedContentIsValid = !doValidation;
 
     if (doValidation) {
+        nsXPIDLCString buf;
+        rv = entry->GetMetaDataElement("strongly-framed", getter_Copies(buf));
+        // describe this in terms of explicitly weakly framed so as to be backwards
+        // compatible with old cache contents which dont have strongly-framed makers
+        bool weaklyFramed = NS_SUCCEEDED(rv) && buf.EqualsLiteral("0");
+
         //
         // now, we are definitely going to issue a HTTP request to the server.
         // make it conditional if possible.
@@ -3569,10 +3576,12 @@ nsHttpChannel::OnCacheEntryCheck(nsICacheEntry* entry, nsIApplicationCache* appC
         // the request method MUST be either GET or HEAD (see bug 175641) and
         // the cached response code must be < 400
         //
+        // the cached content must not be weakly framed
+        //
         // do not override conditional headers when consumer has defined its own
         if (!mCachedResponseHead->NoStore() &&
             (mRequestHead.IsGet() || mRequestHead.IsHead()) &&
-            !mCustomConditionalRequest &&
+            !mCustomConditionalRequest && !weaklyFramed &&
             (mCachedResponseHead->Status() < 400)) {
 
             if (mConcurentCacheAccess) {
@@ -4357,6 +4366,9 @@ nsHttpChannel::InitCacheEntry()
     rv = UpdateExpirationTime();
     if (NS_FAILED(rv)) return rv;
 
+    // mark this weakly framed until a response body is seen
+    mCacheEntry->SetMetaDataElement("strongly-framed", "0");
+
     rv = AddCacheEntryHeaders(mCacheEntry);
     if (NS_FAILED(rv)) return rv;
 
@@ -4561,6 +4573,12 @@ nsHttpChannel::FinalizeCacheEntry()
 {
     LOG(("nsHttpChannel::FinalizeCacheEntry [this=%p]\n", this));
 
+    // Don't update this meta-data on 304
+    if (mStronglyFramed && !mCachedContentIsValid && mCacheEntry) {
+        LOG(("nsHttpChannel::FinalizeCacheEntry [this=%p] Is Strongly Framed\n", this));
+        mCacheEntry->SetMetaDataElement("strongly-framed", "1");
+    }
+
     if (mResponseHead && mResponseHeadersModified) {
         // Set the expiration time for this cache entry
         nsresult rv = UpdateExpirationTime();
@@ -6068,6 +6086,9 @@ nsHttpChannel::OnStopRequest(nsIRequest *request, nsISupports *ctxt, nsresult st
     if (mTransaction) {
         // determine if we should call DoAuthRetry
         bool authRetry = mAuthRetryPending && NS_SUCCEEDED(status);
+        mStronglyFramed = mTransaction->ResponseIsComplete();
+        LOG(("nsHttpChannel %p has a strongly framed transaction: %d",
+             this, mStronglyFramed));
 
         //
         // grab references to connection in case we need to retry an

netwerk/protocol/http/nsHttpChannel.h

@@ -539,6 +539,11 @@ private:
     // True if CORS preflight has been performed
     uint32_t                          mIsCorsPreflightDone : 1;
 
+    // if the http transaction was performed (i.e. not cached) and
+    // the result in OnStopRequest was known to be correctly delimited
+    // by chunking, content-length, or h2 end-stream framing
+    uint32_t                          mStronglyFramed : 1;
+
     nsCOMPtr<nsIChannel>              mPreflightChannel;
 
     nsTArray<nsContinueRedirectionFunc> mRedirectFuncStack;

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -101,15 +101,15 @@ nsHttpTransaction::nsHttpTransaction()
     , mCaps(0)
     , mClassification(CLASS_GENERAL)
     , mPipelinePosition(0)
-    , mCapsToClear(0)
     , mHttpVersion(NS_HTTP_VERSION_UNKNOWN)
     , mHttpResponseCode(0)
+    , mCapsToClear(0)
+    , mResponseIsComplete(false)
     , mClosed(false)
     , mConnected(false)
     , mHaveStatusLine(false)
     , mHaveAllHeaders(false)
     , mTransactionDone(false)
-    , mResponseIsComplete(false)
     , mDidContentStart(false)
     , mNoContent(false)
     , mSentData(false)

netwerk/protocol/http/nsHttpTransaction.h

@@ -105,8 +105,9 @@ public:
     // nsHttpTransaction::Connection should only be used on the socket thread
     already_AddRefed<nsAHttpConnection> GetConnectionReference();
 
-    // Called to find out if the transaction generated a complete response.
+    // Called to set/find out if the transaction generated a complete response.
     bool ResponseIsComplete() { return mResponseIsComplete; }
+    void SetResponseIsComplete() { mResponseIsComplete = true; }
 
     bool      ProxyConnectFailed() { return mProxyConnectFailed; }
 
@@ -271,6 +272,9 @@ private:
     int32_t                         mPipelinePosition;
     int64_t                         mMaxPipelineObjectSize;
 
+    nsHttpVersion                   mHttpVersion;
+    uint16_t                        mHttpResponseCode;
+
     // mCapsToClear holds flags that should be cleared in mCaps, e.g. unset
     // NS_HTTP_REFRESH_DNS when DNS refresh request has completed to avoid
     // redundant requests on the network. The member itself is atomic, but
@@ -279,9 +283,7 @@ private:
     // bitfields should be allowed: 'lost races' will thus err on the
     // conservative side, e.g. by going ahead with a 2nd DNS refresh.
     Atomic<uint32_t>                mCapsToClear;
-
-    nsHttpVersion                   mHttpVersion;
-    uint16_t                        mHttpResponseCode;
+    Atomic<bool, ReleaseAcquire>    mResponseIsComplete;
 
     // state flags, all logically boolean, but not packed together into a
     // bitfield so as to avoid bitfield-induced races.  See bug 560579.
@@ -290,7 +292,6 @@ private:
     bool                            mHaveStatusLine;
     bool                            mHaveAllHeaders;
     bool                            mTransactionDone;
-    bool                            mResponseIsComplete;
     bool                            mDidContentStart;
     bool                            mNoContent; // expecting an empty entity body
     bool                            mSentData;