Bug 1267474 - only revalidate strongly framed responses 1/3 r=mayhemer
authorPatrick McManus <mcmanus@ducksong.com>
Wed, 04 May 2016 17:03:59 -0400
changeset 297198 1c91c9a5fdba
parent 297197 1f0dc5f2cf72
child 297199 06a3edfb00cd
push id30253
push usercbook@mozilla.com
push date2016-05-13 09:59 +0000
treeherdermozilla-central@5a2deb5a9b09 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1267474
milestone49.0a1
Bug 1267474 - only revalidate strongly framed responses 1/3 r=mayhemer
netwerk/protocol/http/Http2Session.cpp
netwerk/protocol/http/Http2Stream.cpp
netwerk/protocol/http/Http2Stream.h
netwerk/protocol/http/SpdySession31.cpp
netwerk/protocol/http/SpdyStream31.cpp
netwerk/protocol/http/SpdyStream31.h
netwerk/protocol/http/nsHttpChannel.cpp
netwerk/protocol/http/nsHttpChannel.h
netwerk/protocol/http/nsHttpTransaction.cpp
netwerk/protocol/http/nsHttpTransaction.h
--- a/netwerk/protocol/http/Http2Session.cpp
+++ b/netwerk/protocol/http/Http2Session.cpp
@@ -3309,16 +3309,17 @@ void
 Http2Session::SetNeedsCleanup()
 {
   LOG3(("Http2Session::SetNeedsCleanup %p - recorded downstream fin of "
         "stream %p 0x%X", this, mInputFrameDataStream,
         mInputFrameDataStream->StreamID()));
 
   // This will result in Close() being called
   MOZ_ASSERT(!mNeedsCleanup, "mNeedsCleanup unexpectedly set");
+  mInputFrameDataStream->SetResponseIsComplete();
   mNeedsCleanup = mInputFrameDataStream;
   ResetDownstreamState();
 }
 
 void
 Http2Session::ConnectPushedStream(Http2Stream *stream)
 {
   mPushesReadyForRead.Push(stream);
--- a/netwerk/protocol/http/Http2Stream.cpp
+++ b/netwerk/protocol/http/Http2Stream.cpp
@@ -1078,16 +1078,25 @@ Http2Stream::ConvertPushHeaders(Http2Dec
 
 void
 Http2Stream::Close(nsresult reason)
 {
   mTransaction->Close(reason);
 }
 
 void
+Http2Stream::SetResponseIsComplete()
+{
+  nsHttpTransaction *trans = mTransaction->QueryHttpTransaction();
+  if (trans) {
+    trans->SetResponseIsComplete();
+  }
+}
+
+void
 Http2Stream::SetAllHeadersReceived()
 {
   if (mAllHeadersReceived) {
     return;
   }
 
   if (mState == RESERVED_BY_REMOTE) {
     // pushed streams needs to wait until headers have
--- a/netwerk/protocol/http/Http2Stream.h
+++ b/netwerk/protocol/http/Http2Stream.h
@@ -75,16 +75,17 @@ public:
 
   nsAHttpTransaction *Transaction() { return mTransaction; }
   virtual nsIRequestContext *RequestContext()
   {
     return mTransaction ? mTransaction->RequestContext() : nullptr;
   }
 
   void Close(nsresult reason);
+  void SetResponseIsComplete();
 
   void SetRecvdFin(bool aStatus);
   bool RecvdFin() { return mRecvdFin; }
 
   void SetRecvdData(bool aStatus) { mReceivedData = aStatus ? 1 : 0; }
   bool RecvdData() { return mReceivedData; }
 
   void SetSentFin(bool aStatus);
--- a/netwerk/protocol/http/SpdySession31.cpp
+++ b/netwerk/protocol/http/SpdySession31.cpp
@@ -2630,16 +2630,17 @@ void
 SpdySession31::SetNeedsCleanup()
 {
   LOG3(("SpdySession31::SetNeedsCleanup %p - recorded downstream fin of "
         "stream %p 0x%X", this, mInputFrameDataStream,
         mInputFrameDataStream->StreamID()));
 
   // This will result in Close() being called
   MOZ_ASSERT(!mNeedsCleanup, "mNeedsCleanup unexpectedly set");
+  mInputFrameDataStream->SetResponseIsComplete();
   mNeedsCleanup = mInputFrameDataStream;
   ResetDownstreamState();
 }
 
 void
 SpdySession31::ConnectPushedStream(SpdyStream31 *stream)
 {
   mReadyForRead.Push(stream);
--- a/netwerk/protocol/http/SpdyStream31.cpp
+++ b/netwerk/protocol/http/SpdyStream31.cpp
@@ -1472,16 +1472,25 @@ SpdyStream31::SetFullyOpen()
 
 void
 SpdyStream31::Close(nsresult reason)
 {
   mTransaction->Close(reason);
 }
 
 void
+SpdyStream31::SetResponseIsComplete()
+{
+  nsHttpTransaction *trans = mTransaction->QueryHttpTransaction();
+  if (trans) {
+    trans->SetResponseIsComplete();
+  }
+}
+
+void
 SpdyStream31::UpdateRemoteWindow(int32_t delta)
 {
   mRemoteWindow += delta;
 
   // If the stream had a <=0 window, that has now opened
   // schedule it for writing again
   if (mBlockedOnRwin && mSession->RemoteSessionWindow() > 0 &&
       mRemoteWindow > 0) {
--- a/netwerk/protocol/http/SpdyStream31.h
+++ b/netwerk/protocol/http/SpdyStream31.h
@@ -44,16 +44,17 @@ public:
 
   nsAHttpTransaction *Transaction() { return mTransaction; }
   virtual nsIRequestContext *RequestContext()
   {
     return mTransaction ? mTransaction->RequestContext() : nullptr;
   }
 
   void Close(nsresult reason);
+  void SetResponseIsComplete();
 
   void SetRecvdFin(bool aStatus) { mRecvdFin = aStatus ? 1 : 0; }
   bool RecvdFin() { return mRecvdFin; }
 
   void SetRecvdData(bool aStatus) { mReceivedData = aStatus ? 1 : 0; }
   bool RecvdData() { return mReceivedData; }
 
   void SetQueued(bool aStatus) { mQueued = aStatus ? 1 : 0; }
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -255,16 +255,17 @@ nsHttpChannel::nsHttpChannel()
     , mCacheEntriesToWaitFor(0)
     , mHasQueryString(0)
     , mConcurentCacheAccess(0)
     , mIsPartialRequest(0)
     , mHasAutoRedirectVetoNotifier(0)
     , mPinCacheContent(0)
     , mIsPackagedAppResource(0)
     , mIsCorsPreflightDone(0)
+    , mStronglyFramed(false)
     , mPushedStream(nullptr)
     , mLocalBlocklist(false)
     , mWarningReporter(nullptr)
     , mDidReval(false)
 {
     LOG(("Creating nsHttpChannel [this=%p]\n", this));
     mChannelCreationTime = PR_Now();
     mChannelCreationTimestamp = TimeStamp::Now();
@@ -3553,31 +3554,39 @@ nsHttpChannel::OnCacheEntryCheck(nsICach
 
     if (doValidation && mInterceptCache == INTERCEPTED) {
         doValidation = false;
     }
 
     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.
         //
         // do not attempt to validate no-store content, since servers will not
         // expect it to be cached.  (we only keep it in our cache for the
         // purposes of back/forward, etc.)
         //
         // 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) {
                 // In case of concurrent read and also validation request we
                 // must wait for the current writer to close the output stream
                 // first.  Otherwise, when the writer's job would have been interrupted
                 // before all the data were downloaded, we'd have to do a range request
                 // which would be a second request in line during this channel's
@@ -4352,16 +4361,19 @@ nsHttpChannel::InitCacheEntry()
 
         mCacheEntryIsWriteOnly = true;
     }
 
     // Set the expiration time for this cache entry
     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;
 
     mInitedCacheEntry = true;
 
     // Don't perform the check when writing (doesn't make sense)
     mConcurentCacheAccess = 0;
 
@@ -4556,16 +4568,22 @@ StoreAuthorizationMetaData(nsICacheEntry
 //  - may need to rewrite response headers if any headers changed
 //  - may need to recalculate the expiration time if any headers changed
 //  - called only for freshly written cache entries
 nsresult
 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();
         if (NS_FAILED(rv)) return rv;
     }
     return NS_OK;
 }
 
@@ -6063,16 +6081,19 @@ nsHttpChannel::OnStopRequest(nsIRequest 
     nsCOMPtr<nsICompressConvStats> conv = do_QueryInterface(mCompressListener);
     if (conv) {
         conv->GetDecodedDataLength(&mDecodedBodySize);
     }
 
     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
         // authentication request over it or use it for an upgrade
         // to another protocol.
         //
         // this code relies on the code in nsHttpTransaction::Close, which
         // tests for NS_HTTP_STICKY_CONNECTION to determine whether or not to
--- a/netwerk/protocol/http/nsHttpChannel.h
+++ b/netwerk/protocol/http/nsHttpChannel.h
@@ -534,16 +534,21 @@ private:
     // Whether fetching the content is meant to be handled by the
     // packaged app service, which behaves like a caching layer.
     // Upon successfully fetching the package, the resource will be placed in
     // the cache, and served by calling OnCacheEntryAvailable.
     uint32_t                          mIsPackagedAppResource : 1;
     // 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;
 
     // Needed for accurate DNS timing
     RefPtr<nsDNSPrefetch>           mDNSPrefetch;
 
     Http2PushedStream                 *mPushedStream;
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -96,25 +96,25 @@ nsHttpTransaction::nsHttpTransaction()
     , mInitialRwin(0)
     , mChunkedDecoder(nullptr)
     , mStatus(NS_OK)
     , mPriority(0)
     , mRestartCount(0)
     , 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)
     , mReceivedData(false)
     , mStatusEventPending(false)
     , mHasRequestBody(false)
     , mProxyConnectFailed(false)
     , mHttpResponseMatched(false)
--- a/netwerk/protocol/http/nsHttpTransaction.h
+++ b/netwerk/protocol/http/nsHttpTransaction.h
@@ -100,18 +100,19 @@ public:
     // Called to take ownership of the response headers; the transaction
     // will drop any reference to the response headers after this call.
     nsHttpResponseHead *TakeResponseHead();
 
     // Provides a thread safe reference of the connection
     // 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; }
 
     void EnableKeepAlive() { mCaps |= NS_HTTP_ALLOW_KEEPALIVE; }
     void MakeSticky() { mCaps |= NS_HTTP_STICKY_CONNECTION; }
 
     // SetPriority() may only be used by the connection manager.
     void    SetPriority(int32_t priority) { mPriority = priority; }
@@ -266,36 +267,36 @@ private:
     int16_t                         mPriority;
 
     uint16_t                        mRestartCount;        // the number of times this transaction has been restarted
     uint32_t                        mCaps;
     enum Classifier                 mClassification;
     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
     // access to it from the networking thread may happen either before or
     // after the main thread modifies it. To deal with raciness, only unsetting
     // 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.
     bool                            mClosed;
     bool                            mConnected;
     bool                            mHaveStatusLine;
     bool                            mHaveAllHeaders;
     bool                            mTransactionDone;
-    bool                            mResponseIsComplete;
     bool                            mDidContentStart;
     bool                            mNoContent; // expecting an empty entity body
     bool                            mSentData;
     bool                            mReceivedData;
     bool                            mStatusEventPending;
     bool                            mHasRequestBody;
     bool                            mProxyConnectFailed;
     bool                            mHttpResponseMatched;