Bug 1493724 - Don't overwrite still-valid altsvc mappings until a new one is validated. r=valentin
authorNicholas Hurley <hurley@mozilla.com>
Mon, 15 Oct 2018 21:52:04 +0000
changeset 497100 32e5d5e6c9804247b0f82f931dc875fdc838f750
parent 497099 ed1c9d5965db3b824fa7d1adc91ced54f03bb070
child 497104 336f795d5b2621dd2d693649f815ad65182f5c17
push id9996
push userarchaeopteryx@coole-files.de
push dateThu, 18 Oct 2018 18:37:15 +0000
treeherdermozilla-beta@8efe26839243 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvalentin
bugs1493724
milestone64.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 1493724 - Don't overwrite still-valid altsvc mappings until a new one is validated. r=valentin Right now, as soon as we receive an alt-svc header or frame for an origin, we will overwrite any mapping we already have for that origin, even if it's still valid. This means that, should validation fail for the new mapping, we will have blown away a perfectly usable alt-svc mapping for no good reason. This patch prevents us from overwriting until we know the new mapping is good. Differential Revision: https://phabricator.services.mozilla.com/D8747
netwerk/protocol/http/AlternateServices.cpp
netwerk/protocol/http/AlternateServices.h
--- a/netwerk/protocol/http/AlternateServices.cpp
+++ b/netwerk/protocol/http/AlternateServices.cpp
@@ -181,16 +181,17 @@ AltSvcMapping::AltSvcMapping(DataStorage
   , mOriginPort(originPort)
   , mUsername(username)
   , mPrivate(privateBrowsing)
   , mExpiresAt(expiresAt)
   , mValidated(false)
   , mMixedScheme(false)
   , mNPNToken(npnToken)
   , mOriginAttributes(originAttributes)
+  , mSyncOnlyOnSuccess(false)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   if (NS_FAILED(SchemeIsHTTPS(originScheme, mHttps))) {
     LOG(("AltSvcMapping ctor %p invalid scheme\n", this));
     mExpiresAt = 0; // invalid
   }
 
@@ -264,16 +265,19 @@ AltSvcMapping::SyncString(const nsCStrin
 }
 
 void
 AltSvcMapping::Sync()
 {
   if (!mStorage) {
     return;
   }
+  if (mSyncOnlyOnSuccess && !mValidated) {
+    return;
+  }
   nsCString value;
   Serialize(value);
 
   if (!NS_IsMainThread()) {
     nsCOMPtr<nsIRunnable> r;
     r = NewRunnableMethod<nsCString>("net::AltSvcMapping::SyncString",
                                      this,
                                      &AltSvcMapping::SyncString,
@@ -374,16 +378,17 @@ AltSvcMapping::Serialize(nsCString &out)
   mOriginAttributes.CreateSuffix(suffix);
   out.Append(suffix);
   out.Append(':');
 }
 
 AltSvcMapping::AltSvcMapping(DataStorage *storage, int32_t epoch, const nsCString &str)
   : mStorage(storage)
   , mStorageEpoch(epoch)
+  , mSyncOnlyOnSuccess(false)
 {
   mValidated = false;
   nsresult code;
 
   // The the do {} while(0) loop acts like try/catch(e){} with the break in _NS_NEXT_TOKEN
   do {
 #ifdef _NS_NEXT_TOKEN
 COMPILER ERROR
@@ -878,33 +883,41 @@ AltSvcCache::UpdateAltServiceMapping(Alt
           LOG(("AltSvcCache::UpdateAltServiceMapping %p map %p tries to extend %p but"
                " cannot as without .wk\n",
                this, map, existing.get()));
         }
       }
       return;
     }
 
-    // new alternate. remove old entry and start new validation
-    LOG(("AltSvcCache::UpdateAltServiceMapping %p map %p overwrites %p\n",
+    if (map->GetExpiresAt() < existing->GetExpiresAt()) {
+      LOG(("AltSvcCache::UpdateAltServiceMapping %p map %p ttl shorter than %p, ignoring",
+           this, map, existing.get()));
+      return;
+    }
+
+    // new alternate. start new validation
+    LOG(("AltSvcCache::UpdateAltServiceMapping %p map %p may overwrite %p\n",
          this, map, existing.get()));
-    existing = nullptr;
-    mStorage->Remove(map->HashKey(),
-                     map->Private() ? DataStorage_Private : DataStorage_Persistent);
   }
 
   if (existing && !existing->Validated()) {
     LOG(("AltSvcCache::UpdateAltServiceMapping %p map %p ignored because %p "
          "still in progress\n", this, map, existing.get()));
     return;
   }
 
-  // start new validation
+  // start new validation, but don't overwrite a valid existing mapping unless
+  // this completes successfully
   MOZ_ASSERT(!map->Validated());
-  map->Sync();
+  if (!existing) {
+    map->Sync();
+  } else {
+    map->SetSyncOnlyOnSuccess(true);
+  }
 
   RefPtr<nsHttpConnectionInfo> ci;
   map->GetConnectionInfo(getter_AddRefs(ci), pi, originAttributes);
   caps |= ci->GetAnonymous() ? NS_HTTP_LOAD_ANONYMOUS : 0;
   caps |= NS_HTTP_ERROR_SOFTLY;
 
   if (map->HTTPS()) {
     LOG(("AltSvcCache::UpdateAltServiceMapping %p validation via "
--- a/netwerk/protocol/http/AlternateServices.h
+++ b/netwerk/protocol/http/AlternateServices.h
@@ -87,16 +87,17 @@ public:
   int32_t StorageEpoch() { return mStorageEpoch; }
   bool    Private() { return mPrivate; }
 
   void SetValidated(bool val);
   void SetMixedScheme(bool val);
   void SetExpiresAt(int32_t val);
   void SetExpired();
   void Sync();
+  void SetSyncOnlyOnSuccess(bool aSOOS) { mSyncOnlyOnSuccess = aSOOS; }
 
   static void MakeHashKey(nsCString &outKey,
                           const nsACString &originScheme,
                           const nsACString &originHost,
                           int32_t originPort,
                           bool privateBrowsing,
                           const OriginAttributes &originAttributes);
 
@@ -123,16 +124,18 @@ private:
 
   MOZ_INIT_OUTSIDE_CTOR bool mValidated;
   MOZ_INIT_OUTSIDE_CTOR bool mHttps; // origin is https://
   MOZ_INIT_OUTSIDE_CTOR bool mMixedScheme; // .wk allows http and https on same con
 
   nsCString mNPNToken;
 
   OriginAttributes mOriginAttributes;
+
+  bool mSyncOnlyOnSuccess;
 };
 
 class AltSvcOverride : public nsIInterfaceRequestor
                      , public nsISpeculativeConnectionOverrider
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSISPECULATIVECONNECTIONOVERRIDER