Convert move event from writable.close to modified event.
When FileSystemWritableFileStream is closed, the swap file overrides
the original file, causing a "moved" event. While a file move happens
under the hood, at a higher level this should be reported as "modified"
as the swap file is an implementation detail for writable.
This change converts the "moved" event to "modified" in this case,
fixing the issue when a directory is watched and its descendent file is
modified via writable. However, if an observed file is being modified by
writable, we currently do not have a way to detect this case to do
conversion. There should be a follow-up to mitigate the observed-file
case.
Bug: 340584120
Change-Id: Id2ae12fcf9cec52f44436a4e2937cb2be68e4d1c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5792324
Commit-Queue: Daseul Lee <dslee@chromium.org>
Reviewed-by: Nathan Memmott <memmott@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1343814}
diff --git a/content/browser/file_system_access/file_system_access_observer_browsertest.cc b/content/browser/file_system_access/file_system_access_observer_browsertest.cc
index 0fc6ebb4..532f314 100644
--- a/content/browser/file_system_access/file_system_access_observer_browsertest.cc
+++ b/content/browser/file_system_access/file_system_access_observer_browsertest.cc
@@ -730,8 +730,8 @@
#endif // !BUILDFLAG(IS_MAC)
// TODO(crbug.com/343961295): Windows reports two events when a swap file
-// is closed: a "disappear" for the target file being overwritten, and a "move"
-// for the swap file being moved to the target file.
+// is closed: a "disappeared" for the target file being overwritten, and a
+// "appeared" for the swap file being moved to the target file.
//
// TODO(crbug.com/357134621): Like on Windows, FSEvents (Mac) also reports two
// events when the swap file is closed. This test fails due to a "disappear"
@@ -754,8 +754,16 @@
ASSERT_THAT(records.GetList(), testing::Not(testing::IsEmpty()));
// TODO(crbug.com/321980270): Support change types for the local file system
// on more platforms.
+ //
+ // TODO(crbug.com/340584120): On Local FS, when writable close() causes the
+ // swap file to override the file that is being observed, it reports
+ // "appeared" when watching a file. (Note that this does not happen when
+ // watching a directory and its descendent file gets written via writable).
+ // On Bucket FS, it correctly reports as "modified".
const std::string expected_change_type =
- SupportsChangeInfo() ? "appeared" : "unknown";
+ GetTestFileSystemType() == TestFileSystemType::kBucket
+ ? "modified"
+ : (SupportsChangeInfo() ? "appeared" : "unknown");
EXPECT_THAT(*records.GetList().front().GetDict().FindString("type"),
testing::StrEq(expected_change_type));
}
@@ -1108,11 +1116,15 @@
}
#endif // !BUILDFLAG(IS_MAC)
+// TODO(crbug.com/343961295): Windows reports two events when a swap file
+// is closed: a "disappeared" for the target file being overwritten, and a
+// "appeared" for the swap file being moved to the target file.
+//
// TODO(b/321980270): Filter out changes to swap files reported by FSEvents,
// and re-enable this test on Mac.
-#if !BUILDFLAG(IS_MAC)
+#if !BUILDFLAG(IS_WIN) && !BUILDFLAG(IS_MAC)
IN_PROC_BROWSER_TEST_P(FileSystemAccessObserverBrowserTest,
- IgnoreSwapFileChanges) {
+ WritableReportsSingleModifiedEventOnClose) {
base::FilePath dir_path = CreateDirectoryToBePicked();
// Set up the directory structure.
@@ -1137,27 +1149,28 @@
SET_CHANGE_TIMEOUT
"})()";
// clang-format on
+
+ // Expect one modified event upon closing the writable.
auto records = EvalJs(shell(), script).ExtractList();
- ASSERT_THAT(records.GetList(), testing::Not(testing::IsEmpty()));
+ EXPECT_THAT(records.GetList(), testing::SizeIs(1));
+ // TODO(b/350790385): On inotify, coalescing move events are not always
+ // guaranteed, and when it fails to coalesce, it results in "appeared" event
+ // on the new path. In the case of swap file, this unfortunately means it
+ // cannot be converted to "modified". Consider improving coalescing
+ // implementation and/or have `FileSystemAccessWatcherManager` convert
+ // "moved" to "appeard" or "disappeared" instead of `FilePathWatcher` being
+ // responsible for the conversion on inotify-based platforms.
+ auto& record_dict = records.GetList().front().GetDict();
+ EXPECT_THAT(
+ *record_dict.FindString("type"),
+ testing::AnyOf(testing::StrEq("modified"), testing::StrEq("appeared")));
const auto relative_path_component_matcher = testing::Conditional(
SupportsReportingModifiedPath(), testing::ElementsAre("file.txt"),
testing::IsEmpty());
- EXPECT_THAT(
- *records.GetList().front().GetDict().FindList("relativePathComponents"),
- relative_path_component_matcher);
-
- // Check that none of the events are for swap files.
- const auto relative_path_component_matcher_for_swap_file =
- testing::Conditional(
- SupportsReportingModifiedPath(),
- testing::ElementsAre(testing::Not("file.txt.crswap")),
- testing::IsEmpty());
- for (const auto& record : records.GetList()) {
- EXPECT_THAT(*record.GetDict().FindList("relativePathComponents"),
- relative_path_component_matcher_for_swap_file);
- }
+ EXPECT_THAT(*record_dict.FindList("relativePathComponents"),
+ relative_path_component_matcher);
}
-#endif // !BUILDFLAG(IS_MAC)
+#endif // !BUILDFLAG(IS_WIN) && !BUILDFLAG(IS_MAC)
INSTANTIATE_TEST_SUITE_P(
/* no prefix */,
diff --git a/content/browser/file_system_access/file_system_access_watcher_manager.cc b/content/browser/file_system_access/file_system_access_watcher_manager.cc
index d7cbba0b..db3b611 100644
--- a/content/browser/file_system_access/file_system_access_watcher_manager.cc
+++ b/content/browser/file_system_access/file_system_access_watcher_manager.cc
@@ -162,26 +162,43 @@
void FileSystemAccessWatcherManager::OnRawChange(
const storage::FileSystemURL& changed_url,
bool error,
- const FileSystemAccessChangeSource::ChangeInfo& change_info,
+ const FileSystemAccessChangeSource::ChangeInfo& raw_change_info,
const FileSystemAccessWatchScope& scope) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// TODO(crbug.com/40268906): Batch changes.
- // Changes to swap files are implementation details and should be ignored.
+ // Writes via FileSystemWritableFileStream are done on a temporary swap file,
+ // so ignore the swap file changes. When a writable is closed, the swap file
+ // overrides the original file, producing a moved event. We will convert this
+ // "moved" event to "modified" below.
if (changed_url.virtual_path().FinalExtension() ==
FILE_PATH_LITERAL(".crswap")) {
return;
}
- if (change_info != FileSystemAccessChangeSource::ChangeInfo()) {
+ if (raw_change_info != FileSystemAccessChangeSource::ChangeInfo()) {
// If non-empty ChangeInfo exists, this change should not be an error.
CHECK(!error);
}
// For ChangeType::kMoved, ChangeInfo.moved_from_path is expected.
- bool is_move_event = change_info.change_type ==
+ bool is_move_event = raw_change_info.change_type ==
FileSystemAccessChangeSource::ChangeType::kMoved;
- CHECK(!is_move_event || change_info.moved_from_path.has_value());
+ CHECK(!is_move_event || raw_change_info.moved_from_path.has_value());
+
+ // Convert the "moved" event to "modified" if the event is from the swap file
+ // overriding the original file due to `FileSystemWritableFileStream.close()`.
+ FileSystemAccessChangeSource::ChangeInfo change_info = raw_change_info;
+ if (is_move_event &&
+ raw_change_info.moved_from_path.value().FinalExtension() ==
+ FILE_PATH_LITERAL(".crswap")) {
+ is_move_event = false;
+ change_info = FileSystemAccessChangeSource::ChangeInfo(
+ raw_change_info.file_path_type,
+ FileSystemAccessChangeSource::ChangeType::kModified,
+ raw_change_info.modified_path);
+ }
+
std::optional<storage::FileSystemURL> moved_from_url =
is_move_event ? std::make_optional(
ToFileSystemURL(*manager()->context(), changed_url,
diff --git a/third_party/blink/web_tests/external/wpt/fs/FileSystemObserver-writable-file-stream.https.tentative.any-expected.txt b/third_party/blink/web_tests/external/wpt/fs/FileSystemObserver-writable-file-stream.https.tentative.any-expected.txt
deleted file mode 100644
index b44a5b5..0000000
--- a/third_party/blink/web_tests/external/wpt/fs/FileSystemObserver-writable-file-stream.https.tentative.any-expected.txt
+++ /dev/null
@@ -1,5 +0,0 @@
-This is a testharness.js-based test.
-[FAIL] Closing a FileSystemWritableFileStream that's modified the file produces a "modified" event
- assert_equals: A record's type didn't match the expected type expected "modified" but got "appeared"
-Harness: the test ran to completion.
-
diff --git a/third_party/blink/web_tests/external/wpt/fs/FileSystemObserver-writable-file-stream.https.tentative.any.sharedworker-expected.txt b/third_party/blink/web_tests/external/wpt/fs/FileSystemObserver-writable-file-stream.https.tentative.any.sharedworker-expected.txt
deleted file mode 100644
index b44a5b5..0000000
--- a/third_party/blink/web_tests/external/wpt/fs/FileSystemObserver-writable-file-stream.https.tentative.any.sharedworker-expected.txt
+++ /dev/null
@@ -1,5 +0,0 @@
-This is a testharness.js-based test.
-[FAIL] Closing a FileSystemWritableFileStream that's modified the file produces a "modified" event
- assert_equals: A record's type didn't match the expected type expected "modified" but got "appeared"
-Harness: the test ran to completion.
-
diff --git a/third_party/blink/web_tests/external/wpt/fs/FileSystemObserver-writable-file-stream.https.tentative.any.worker-expected.txt b/third_party/blink/web_tests/external/wpt/fs/FileSystemObserver-writable-file-stream.https.tentative.any.worker-expected.txt
deleted file mode 100644
index b44a5b5..0000000
--- a/third_party/blink/web_tests/external/wpt/fs/FileSystemObserver-writable-file-stream.https.tentative.any.worker-expected.txt
+++ /dev/null
@@ -1,5 +0,0 @@
-This is a testharness.js-based test.
-[FAIL] Closing a FileSystemWritableFileStream that's modified the file produces a "modified" event
- assert_equals: A record's type didn't match the expected type expected "modified" but got "appeared"
-Harness: the test ran to completion.
-
