Skip to content

Commit

Permalink
Remove base/mac/scoped_dispatch_object.h
Browse files Browse the repository at this point in the history 
In ARC, dispatch objects are Objective-C objects and need to be
managed as such.

See https://chromium.googlesource.com/chromium/src/+/main/docs/mac/arc.md
for information about this conversion.

Bug: 1280317
Change-Id: I5db820ac85bd9aad54feab951c408c3e7785cb95
Include-Ci-Only-Tests: true
Validate-Test-Flakiness: skip
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4519014
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Owners-Override: Avi Drissman <avi@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1143321}
  • Loading branch information
Avi Drissman authored and Chromium LUCI CQ committed May 12, 2023
1 parent b2fa725 commit c22cb79
Show file tree
Hide file tree
Showing 21 changed files with 276 additions and 213 deletions.
49 changes: 40 additions & 9 deletions base/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,40 @@ buildflag_header("message_pump_buildflags") {
flags = [ "ENABLE_MESSAGE_PUMP_EPOLL=$enable_message_pump_epoll" ]
}

if (is_apple) {
# TODO(https://crbug.com/1280317): Merge back into the `base` target once all
# .mm files are ARCed.
source_set("base_arc") {
sources = [
"mac/dispatch_source_mach.h",
"mac/dispatch_source_mach.mm",
]
configs += [ "//build/config/compiler:enable_arc" ]
deps = [
":base_static",
"//third_party/abseil-cpp:absl",
]

if (is_mac) {
sources += [
"files/file_path_watcher_fsevents.h",
"files/file_path_watcher_fsevents.mm",
"mac/mach_port_rendezvous.h",
"mac/mach_port_rendezvous.mm",
"synchronization/waitable_event_watcher_mac.mm",
]
libs = [ "bsm" ]
frameworks = [
"AppKit.framework",
"CoreServices.framework",
]
}
if (is_ios) {
frameworks = [ "UIKit.framework" ]
}
}
}

# Base and everything it depends on should be a static library rather than
# a source set. Base is more of a "library" in the classic sense in that many
# small parts of it are used in many different contexts. This combined with a
Expand Down Expand Up @@ -1967,8 +2001,6 @@ component("base") {
"enterprise_util.cc",
"enterprise_util.h",
"enterprise_util_mac.mm",
"files/file_path_watcher_fsevents.cc",
"files/file_path_watcher_fsevents.h",
"files/file_path_watcher_kqueue.cc",
"files/file_path_watcher_kqueue.h",
"files/file_path_watcher_mac.cc",
Expand All @@ -1981,16 +2013,13 @@ component("base") {
"mac/launchd.h",
"mac/mac_util.h",
"mac/mac_util.mm",
"mac/mach_port_rendezvous.cc",
"mac/mach_port_rendezvous.h",
"mac/os_crash_dumps.cc",
"mac/os_crash_dumps.h",
"mac/scoped_aedesc.h",
"mac/scoped_authorizationref.h",
"mac/scoped_authorizationref.mm",
"mac/scoped_cffiledescriptorref.h",
"mac/scoped_cftyperef.h",
"mac/scoped_dispatch_object.h",
"mac/scoped_ionotificationportref.h",
"mac/scoped_ioobject.h",
"mac/scoped_ioplugininterface.h",
Expand Down Expand Up @@ -2024,7 +2053,6 @@ component("base") {
"profiler/stack_sampler_mac.cc",
"profiler/suspendable_thread_delegate_mac.cc",
"profiler/suspendable_thread_delegate_mac.h",
"synchronization/waitable_event_watcher_mac.cc",
"system/sys_info_mac.mm",
"time/time_exploded_posix.cc",
]
Expand All @@ -2045,6 +2073,7 @@ component("base") {

# Mac or iOS.
if (is_apple) {
allow_circular_includes_from = [ ":base_arc" ]
sources += [
"apple/bridging.h",
"file_version_info_mac.h",
Expand All @@ -2057,8 +2086,6 @@ component("base") {
"mac/call_with_eh_frame.cc",
"mac/call_with_eh_frame.h",
"mac/call_with_eh_frame_asm.S",
"mac/dispatch_source_mach.cc",
"mac/dispatch_source_mach.h",
"mac/foundation_util.h",
"mac/foundation_util.mm",
"mac/mac_logging.h",
Expand Down Expand Up @@ -2088,6 +2115,7 @@ component("base") {
"time/time_mac.mm",
]
frameworks += [ "Security.framework" ]
public_deps += [ ":base_arc" ]
}

# Linux.
Expand Down Expand Up @@ -3464,7 +3492,10 @@ test("base_unittests") {
if (is_apple) {
public_deps = [ ":base_unittests_bundle_data" ]

deps += [ ":base_unittests_arc" ]
deps += [
":base_arc",
":base_unittests_arc",
]
}

if (!is_ios) {
Expand Down
7 changes: 3 additions & 4 deletions base/files/file_path_watcher_fsevents.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

#include "base/files/file_path.h"
#include "base/files/file_path_watcher.h"
#include "base/mac/scoped_dispatch_object.h"
#include "base/memory/weak_ptr.h"

namespace base {
Expand Down Expand Up @@ -75,9 +74,6 @@ class FilePathWatcherFSEvents : public FilePathWatcher::PlatformDelegate {
// (Only accessed from the task_runner() thread.)
FilePathWatcher::Callback callback_;

// The dispatch queue on which the event stream is scheduled.
ScopedDispatchObject<dispatch_queue_t> queue_;

// Target path to watch (passed to callback).
// (Only accessed from the libdispatch queue.)
FilePath target_;
Expand All @@ -90,6 +86,9 @@ class FilePathWatcherFSEvents : public FilePathWatcher::PlatformDelegate {
// (Only accessed from the libdispatch queue.)
FSEventStreamRef fsevent_stream_ = nullptr;

struct ObjCStorage;
std::unique_ptr<ObjCStorage> objc_storage_;

WeakPtrFactory<FilePathWatcherFSEvents> weak_factory_{this};
};

Expand Down
84 changes: 50 additions & 34 deletions base/files/file_path_watcher_fsevents.cc → base/files/file_path_watcher_fsevents.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
#include "base/task/sequenced_task_runner.h"
#include "base/threading/scoped_blocking_call.h"

#if !defined(__has_feature) || !__has_feature(objc_arc)
#error "This file requires ARC support."
#endif

namespace base {

namespace {
Expand All @@ -30,8 +34,8 @@ FilePath ResolvePath(const FilePath& path) {
const unsigned kMaxLinksToResolve = 255;

std::vector<FilePath::StringType> component_vector = path.GetComponents();
std::list<FilePath::StringType>
components(component_vector.begin(), component_vector.end());
std::list<FilePath::StringType> components(component_vector.begin(),
component_vector.end());

FilePath result;
unsigned resolve_count = 0;
Expand All @@ -48,8 +52,9 @@ FilePath ResolvePath(const FilePath& path) {

FilePath target;
if (ReadSymbolicLink(current, &target)) {
if (target.IsAbsolute())
if (target.IsAbsolute()) {
result.clear();
}
std::vector<FilePath::StringType> target_components =
target.GetComponents();
components.insert(components.begin(), target_components.begin(),
Expand All @@ -60,18 +65,25 @@ FilePath ResolvePath(const FilePath& path) {
}
}

if (resolve_count >= kMaxLinksToResolve)
if (resolve_count >= kMaxLinksToResolve) {
result.clear();
}
return result;
}

} // namespace

struct FilePathWatcherFSEvents::ObjCStorage {
// The dispatch queue on which the event stream is scheduled.
dispatch_queue_t __strong queue;
};

FilePathWatcherFSEvents::FilePathWatcherFSEvents()
: queue_(dispatch_queue_create(
base::StringPrintf("org.chromium.base.FilePathWatcher.%p", this)
.c_str(),
DISPATCH_QUEUE_SERIAL)) {}
: objc_storage_(std::make_unique<ObjCStorage>()) {
objc_storage_->queue = dispatch_queue_create(
base::StringPrintf("org.chromium.base.FilePathWatcher.%p", this).c_str(),
DISPATCH_QUEUE_SERIAL);
}

FilePathWatcherFSEvents::~FilePathWatcherFSEvents() {
DCHECK(!task_runner() || task_runner()->RunsTasksInCurrentSequence());
Expand All @@ -87,8 +99,9 @@ bool FilePathWatcherFSEvents::Watch(const FilePath& path,

// This class could support non-recursive watches, but that is currently
// left to FilePathWatcherKQueue.
if (type != Type::kRecursive)
if (type != Type::kRecursive) {
return false;
}

set_task_runner(SequencedTaskRunner::GetCurrentDefault());
callback_ = callback;
Expand All @@ -99,8 +112,8 @@ bool FilePathWatcherFSEvents::Watch(const FilePath& path,
// captured by the block's scope.
const FilePath path_copy(path);

dispatch_async(queue_, ^{
StartEventStream(start_event, path_copy);
dispatch_async(objc_storage_->queue, ^{
StartEventStream(start_event, path_copy);
});
return true;
}
Expand All @@ -113,7 +126,7 @@ void FilePathWatcherFSEvents::Cancel() {
// Switch to the dispatch queue to tear down the event stream. As the queue is
// owned by |this|, and this method is called from the destructor, execute the
// block synchronously.
dispatch_sync(queue_, ^{
dispatch_sync(objc_storage_->queue, ^{
if (fsevent_stream_) {
DestroyEventStream();
target_.clear();
Expand All @@ -136,12 +149,14 @@ void FilePathWatcherFSEvents::FSEventsCallback(
std::vector<FilePath> paths;
FSEventStreamEventId root_change_at = FSEventStreamGetLatestEventId(stream);
for (size_t i = 0; i < num_events; i++) {
if (flags[i] & kFSEventStreamEventFlagRootChanged)
if (flags[i] & kFSEventStreamEventFlagRootChanged) {
root_changed = true;
if (event_ids[i])
}
if (event_ids[i]) {
root_change_at = std::min(root_change_at, event_ids[i]);
paths.push_back(FilePath(
reinterpret_cast<char**>(event_paths)[i]).StripTrailingSeparators());
}
paths.push_back(FilePath(reinterpret_cast<char**>(event_paths)[i])
.StripTrailingSeparators());
}

// Reinitialize the event stream if we find changes to the root. This is
Expand All @@ -163,10 +178,11 @@ void FilePathWatcherFSEvents::FSEventsCallback(
FROM_HERE, BindOnce(
[](WeakPtr<FilePathWatcherFSEvents> weak_watcher,
FSEventStreamEventId root_change_at) {
if (!weak_watcher)
if (!weak_watcher) {
return;
}
FilePathWatcherFSEvents* watcher = weak_watcher.get();
dispatch_async(watcher->queue_, ^{
dispatch_async(watcher->objc_storage_->queue, ^{
watcher->UpdateEventStream(root_change_at);
});
},
Expand Down Expand Up @@ -207,35 +223,35 @@ void FilePathWatcherFSEvents::UpdateEventStream(
FSEventStreamEventId start_event) {
// It can happen that the watcher gets canceled while tasks that call this
// function are still in flight, so abort if this situation is detected.
if (resolved_target_.empty())
if (resolved_target_.empty()) {
return;
}

if (fsevent_stream_)
if (fsevent_stream_) {
DestroyEventStream();
}

ScopedCFTypeRef<CFStringRef> cf_path(CFStringCreateWithCString(
NULL, resolved_target_.value().c_str(), kCFStringEncodingMacHFS));
nullptr, resolved_target_.value().c_str(), kCFStringEncodingMacHFS));
ScopedCFTypeRef<CFStringRef> cf_dir_path(CFStringCreateWithCString(
NULL, resolved_target_.DirName().value().c_str(),
nullptr, resolved_target_.DirName().value().c_str(),
kCFStringEncodingMacHFS));
CFStringRef paths_array[] = { cf_path.get(), cf_dir_path.get() };
CFStringRef paths_array[] = {cf_path.get(), cf_dir_path.get()};
ScopedCFTypeRef<CFArrayRef> watched_paths(
CFArrayCreate(NULL, reinterpret_cast<const void**>(paths_array),
CFArrayCreate(nullptr, reinterpret_cast<const void**>(paths_array),
std::size(paths_array), &kCFTypeArrayCallBacks));

FSEventStreamContext context;
context.version = 0;
context.info = this;
context.retain = NULL;
context.release = NULL;
context.copyDescription = NULL;
context.retain = nullptr;
context.release = nullptr;
context.copyDescription = nullptr;

fsevent_stream_ = FSEventStreamCreate(NULL, &FSEventsCallback, &context,
watched_paths,
start_event,
kEventLatencySeconds,
kFSEventStreamCreateFlagWatchRoot);
FSEventStreamSetDispatchQueue(fsevent_stream_, queue_);
fsevent_stream_ = FSEventStreamCreate(
nullptr, &FSEventsCallback, &context, watched_paths, start_event,
kEventLatencySeconds, kFSEventStreamCreateFlagWatchRoot);
FSEventStreamSetDispatchQueue(fsevent_stream_, objc_storage_->queue);

if (!FSEventStreamStart(fsevent_stream_)) {
task_runner()->PostTask(FROM_HERE,
Expand Down Expand Up @@ -267,7 +283,7 @@ void FilePathWatcherFSEvents::DestroyEventStream() {
FSEventStreamStop(fsevent_stream_);
FSEventStreamInvalidate(fsevent_stream_);
FSEventStreamRelease(fsevent_stream_);
fsevent_stream_ = NULL;
fsevent_stream_ = nullptr;
}

void FilePathWatcherFSEvents::StartEventStream(FSEventStreamEventId start_event,
Expand Down
46 changes: 0 additions & 46 deletions base/mac/dispatch_source_mach.cc
View file

This file was deleted.

Loading

0 comments on commit c22cb79

Please sign in to comment.