Bug 1267474 - only revalidate strongly framed responses 1/3 r=mayhemer
💩💩 backed out by 6bd80255be5e 💩 💩
authorPatrick McManus <mcmanus@ducksong.com>
Wed, 04 May 2016 17:03:59 -0400
changeset 296994 0c059a0e87d7
parent 296993 3271caad1b5c
child 296995 960d93f30c66
push id76553
push usermcmanus@ducksong.com
push date2016-05-11 15:41 +0000
treeherdermozilla-inbound@057f3f50441f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1267474
milestone49.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 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;