From 2b194eb9d018e0f3d8ff8650ce863624f5df72d7 Mon Sep 17 00:00:00 2001
From: Doug Thayer <dothayer@mozilla.com>
Date: Wed, 9 Sep 2020 13:42:27 +0000
Subject: [PATCH] Bug 1656261 - Ensure we clean up after WriteToDisk r=froydnj

Given all of the MOZ_TRYs in WriteToDisk, it's entirely possible that
we occasionally exit it before writing everything out. WriteToDisk
additionally adds guards to try to ensure that we do not call it twice,
and that we do not try to read things out of the file via the memmapped
buffer after it has written to it. The latter should not generally be
a problem, since we write to a temp file and move that over at the end.
However, writing twice could be a problem as we modify our values'
mOffsets to keep track of their offsets within the resulting file.

Accordingly, this change simply moves the cleanup and guards into an
RAII helper to ensure they survive all of the early returns.

Differential Revision: https://phabricator.services.mozilla.com/D89253
---
 startupcache/StartupCache.cpp | 39 +++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/startupcache/StartupCache.cpp b/startupcache/StartupCache.cpp
index 66a482558b88..aa6c078fbc1b 100644
--- a/startupcache/StartupCache.cpp
+++ b/startupcache/StartupCache.cpp
@@ -1053,6 +1053,27 @@ Result<Ok, nsresult> StartupCache::WriteToDisk() {
   mFile->GetNativeLeafName(leafName);
   tmpFile->SetNativeLeafName(leafName + "-tmp"_ns);
 
+  auto cleanup = MakeScopeExit([&]() {
+    mDirty = false;
+    mWrittenOnce = true;
+    mCacheData.reset();
+
+    // If we're shutting down, we do not care about freeing up memory right now.
+    if (AppShutdown::IsShuttingDown()) {
+      return;
+    }
+
+    // We've just written (or attempted to write) our buffers to disk, and
+    // we're 60 seconds out from startup, so they're unlikely to be needed
+    // again any time soon; let's clean up what we can.
+    for (auto iter = mTable.iter(); !iter.done(); iter.next()) {
+      auto& value = iter.get().value();
+      if (!value.mFlags.contains(StartupCacheEntryFlags::DoNotFree)) {
+        value.mData = nullptr;
+      }
+    }
+  });
+
   {
     AutoFDClose fd;
     MOZ_TRY(tmpFile->OpenNSPRFileDesc(PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE,
@@ -1183,26 +1204,8 @@ Result<Ok, nsresult> StartupCache::WriteToDisk() {
     MOZ_TRY(Write(fd, buf.Get(), buf.cursor()));
   }
 
-  mDirty = false;
-  mWrittenOnce = true;
-  mCacheData.reset();
   tmpFile->MoveToNative(nullptr, leafName);
 
-  // If we're shutting down, we do not care about freeing up memory right now.
-  if (AppShutdown::IsShuttingDown()) {
-    return Ok();
-  }
-
-  // We've just written our buffers to disk, and we're 60 seconds out from
-  // startup, so they're unlikely to be needed again any time soon; let's
-  // clean up what we can.
-  for (auto iter = mTable.iter(); !iter.done(); iter.next()) {
-    auto& value = iter.get().value();
-    if (!value.mFlags.contains(StartupCacheEntryFlags::DoNotFree)) {
-      value.mData = nullptr;
-    }
-  }
-
   return Ok();
 }