commit 0bcd37bd2b83f1a9ee17088037ebdfe6eab6d31a
author Bruce Dawson <brucedawson@google.com> Thu Aug 11 07:29:24 2022 -0700
committer Commit Bot <commit-bot@chromium.org> Thu Aug 11 16:48:02 2022 +0000
tree dce7dd9d19b0802d35354f1d382ebeac19a908cd
parent 3d773bba0927e67eae8fdaee5e28b0f6203d3bee

Avoid ACCESS_DENIED when writing build.ninja on Windows

Windows is more fussy about writing to files that are open which cause
updates to build.ninja to reliably fail after recent changes. This fixes
the issue by closing build_ninja_file (build.ninja) before writing to
it.

Minimal tests of WriteFileAtomically were also added in hopes that they
would reveal the failure, but that was before the nature of the bug was
known. Unfortunately this bug would only appear in an integration test.

The real issue was found by modifying the code to spin in a loop when
the failure was hit, attaching a debugger, pausing to inspect variables,
and then using sysinternals' "handle.exe" to find which file was locked.

Bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1351858
Change-Id: I99e7816771de0feb00ec9889640c9cdcaae6f3be
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/14380
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>

.gitignore

diff --git a/.gitignore b/.gitignore
index aa227f6..da52c70 100644
--- a/.gitignore
+++ b/.gitignore
@@ -62,3 +62,5 @@
 # Visual Studio Code
 /.vscode/
 /_out
+# VSChromium configuration file
+vs-chromium-project.txt

build/gen.py

diff --git a/build/gen.py b/build/gen.py
index ce9d768..fa23266 100755
--- a/build/gen.py
+++ b/build/gen.py
@@ -832,6 +832,7 @@
         'src/gn/visual_studio_writer_unittest.cc',
         'src/gn/xcode_object_unittest.cc',
         'src/gn/xml_element_writer_unittest.cc',
+        'src/util/atomic_write_unittest.cc',
         'src/util/test/gn_test.cc',
       ], 'libs': []},
   }

src/gn/commands.cc

diff --git a/src/gn/commands.cc b/src/gn/commands.cc
index 030f2d5..93365ad 100644
--- a/src/gn/commands.cc
+++ b/src/gn/commands.cc
@@ -520,6 +520,8 @@
         .PrintToStdout();
     return false;
   }
+  // Close build.ninja or else WriteFileAtomically will fail on Windows.
+  build_ninja_file.close();
   if (util::WriteFileAtomically(build_ninja_path, build_commands.data(),
                                 static_cast<int>(build_commands.size())) ==
       -1) {

src/util/atomic_write_unittest.cc

diff --git a/src/util/atomic_write_unittest.cc b/src/util/atomic_write_unittest.cc
new file mode 100644
index 0000000..c1d3372
--- /dev/null
+++ b/src/util/atomic_write_unittest.cc
@@ -0,0 +1,36 @@
+// Copyright 2022 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "util/atomic_write.h"
+
+#include <string>
+
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "util/test/test.h"
+
+class ImportantFileWriterTest : public testing::Test {
+ public:
+  ImportantFileWriterTest() = default;
+  void SetUp() override {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    file_ = temp_dir_.GetPath().AppendASCII("test-file");
+  }
+
+ protected:
+  base::FilePath file_;
+
+ private:
+  base::ScopedTempDir temp_dir_;
+};
+
+// Test that WriteFileAtomically works.
+TEST_F(ImportantFileWriterTest, Basic) {
+  const std::string data = "Test string for writing.";
+  EXPECT_FALSE(base::PathExists(file_));
+  EXPECT_TRUE(util::WriteFileAtomically(file_, data.data(), data.size()));
+  std::string actual;
+  EXPECT_TRUE(ReadFileToString(file_, &actual));
+  EXPECT_EQ(data, actual);
+}