Bug 960902 - Disallow concurrent HTTP cache read read when validation is needed, r=michal
authorHonza Bambas <honzab.moz@firemni.cz>
Thu, 06 Mar 2014 14:36:20 +0100
changeset 189509 7dec146029bd3142a6ff615577fc46105bb93ed4
parent 189508 be5f4eeb0ec24ac7e40b368d3e40bd00ac14feaa
child 189510 98cc8a50348fe41b4fe310ceb0f51cc4d3503ea7
push id3503
push userraliiev@mozilla.com
push dateMon, 28 Apr 2014 18:51:11 +0000
treeherdermozilla-beta@c95ac01e332e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmichal
bugs960902
milestone30.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 960902 - Disallow concurrent HTTP cache read read when validation is needed, r=michal
netwerk/protocol/http/nsHttpChannel.cpp
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -2826,17 +2826,16 @@ nsHttpChannel::OnCacheEntryCheck(nsICach
             doValidation = false;
         }
     }
     // check if validation is strictly required...
     else if (mCachedResponseHead->MustValidate()) {
         LOG(("Validating based on MustValidate() returning TRUE\n"));
         doValidation = true;
     }
-
     else if (MustValidateBasedOnQueryUrl()) {
         LOG(("Validating based on RFC 2616 section 13.9 "
              "(query-url w/o explicit expiration-time)\n"));
         doValidation = true;
     }
     // Check if the cache entry has expired...
     else {
         uint32_t time = 0; // a temporary variable for storing time values...
@@ -2937,31 +2936,47 @@ nsHttpChannel::OnCacheEntryCheck(nsICach
         //
         // the request method MUST be either GET or HEAD (see bug 175641).
         //
         // do not override conditional headers when consumer has defined its own
         if (!mCachedResponseHead->NoStore() &&
             (mRequestHead.Method() == nsHttp::Get ||
              mRequestHead.Method() == nsHttp::Head) &&
              !mCustomConditionalRequest) {
-            const char *val;
-            // Add If-Modified-Since header if a Last-Modified was given
-            // and we are allowed to do this (see bugs 510359 and 269303)
-            if (canAddImsHeader) {
-                val = mCachedResponseHead->PeekHeader(nsHttp::Last_Modified);
+
+            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
+                // life-time.  nsHttpChannel is not designed to do that, so rather
+                // turn off concurrent read and wait for entry's completion.
+                // Then only re-validation or range-re-validation request will go out.
+                mConcurentCacheAccess = 0;
+                // This will cause that OnCacheEntryCheck is called again with the same
+                // entry after the writer is done.
+                wantCompleteEntry = true;
+            } else {
+                const char *val;
+                // Add If-Modified-Since header if a Last-Modified was given
+                // and we are allowed to do this (see bugs 510359 and 269303)
+                if (canAddImsHeader) {
+                    val = mCachedResponseHead->PeekHeader(nsHttp::Last_Modified);
+                    if (val)
+                        mRequestHead.SetHeader(nsHttp::If_Modified_Since,
+                                               nsDependentCString(val));
+                }
+                // Add If-None-Match header if an ETag was given in the response
+                val = mCachedResponseHead->PeekHeader(nsHttp::ETag);
                 if (val)
-                    mRequestHead.SetHeader(nsHttp::If_Modified_Since,
+                    mRequestHead.SetHeader(nsHttp::If_None_Match,
                                            nsDependentCString(val));
+                mDidReval = true;
             }
-            // Add If-None-Match header if an ETag was given in the response
-            val = mCachedResponseHead->PeekHeader(nsHttp::ETag);
-            if (val)
-                mRequestHead.SetHeader(nsHttp::If_None_Match,
-                                       nsDependentCString(val));
-            mDidReval = true;
         }
     }
 
     if (mCachedContentIsValid || mDidReval) {
         rv = OpenCacheInputStream(entry, mCachedContentIsValid);
         if (NS_FAILED(rv)) {
             // If we can't get the entity then we have to act as though we
             // don't have the cache entry.