Use smart Refs for IOKit and CoreFoundation objects.

Bug: dawn:89
Change-Id: Idea634bcc65ab4ec017f4e4c2431e95915f533df
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/32661
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>

src/common/BUILD.gn

@@ -157,11 +157,13 @@ if (is_win || is_linux || is_chromeos || is_mac || is_fuchsia || is_android) {
       "BitSetIterator.h",
       "Compiler.h",
       "Constants.h",
+      "CoreFoundationRef.h",
       "DynamicLib.cpp",
       "DynamicLib.h",
       "GPUInfo.cpp",
       "GPUInfo.h",
       "HashUtils.h",
+      "IOKitRef.h",
       "LinkedList.h",
       "Log.cpp",
       "Log.h",

src/common/CMakeLists.txt

@@ -19,11 +19,13 @@ target_sources(dawn_common PRIVATE
     "BitSetIterator.h"
     "Compiler.h"
     "Constants.h"
+    "CoreFoundationRef.h"
     "DynamicLib.cpp"
     "DynamicLib.h"
     "GPUInfo.cpp"
     "GPUInfo.h"
     "HashUtils.h"
+    "IOKitRef.h"
     "LinkedList.h"
     "Log.cpp"
     "Log.h"

src/common/CoreFoundationRef.h

@@ -0,0 +1,46 @@
+// Copyright 2020 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef COMMON_COREFOUNDATIONREF_H_
+#define COMMON_COREFOUNDATIONREF_H_
+
+#include "common/RefBase.h"
+
+#include <CoreFoundation/CoreFoundation.h>
+
+template <typename T>
+struct CoreFoundationRefTraits {
+    static constexpr T kNullValue = nullptr;
+    static void Reference(T value) {
+        CFRetain(value);
+    }
+    static void Release(T value) {
+        CFRelease(value);
+    }
+};
+
+template <typename T>
+class CFRef : public RefBase<T, CoreFoundationRefTraits<T>> {
+  public:
+    using RefBase<T, CoreFoundationRefTraits<T>>::RefBase;
+};
+
+template <typename T>
+CFRef<T> AcquireCFRef(T pointee) {
+    CFRef<T> ref;
+    ref.Acquire(pointee);
+    return ref;
+}
+
+#endif  // COMMON_COREFOUNDATIONREF_H_

src/common/IOKitRef.h

@@ -0,0 +1,46 @@
+// Copyright 2020 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef COMMON_IOKITREF_H_
+#define COMMON_IOKITREF_H_
+
+#include "common/RefBase.h"
+
+#include <IOKit/IOKitLib.h>
+
+template <typename T>
+struct IOKitRefTraits {
+    static constexpr T kNullValue = IO_OBJECT_NULL;
+    static void Reference(T value) {
+        IOObjectRetain(value);
+    }
+    static void Release(T value) {
+        IOObjectRelease(value);
+    }
+};
+
+template <typename T>
+class IORef : public RefBase<T, IOKitRefTraits<T>> {
+  public:
+    using RefBase<T, IOKitRefTraits<T>>::RefBase;
+};
+
+template <typename T>
+IORef<T> AcquireIORef(T pointee) {
+    IORef<T> ref;
+    ref.Acquire(pointee);
+    return ref;
+}
+
+#endif  // COMMON_IOKITREF_H_

src/common/RefBase.h

@@ -15,6 +15,7 @@
 #ifndef COMMON_REFBASE_H_
 #define COMMON_REFBASE_H_
 
+#include "common/Assert.h"
 #include "common/Compiler.h"
 
 #include <type_traits>
@@ -163,6 +164,11 @@ class RefBase {
         mValue = value;
     }
 
+    T* InitializeInto() DAWN_NO_DISCARD {
+        ASSERT(mValue == kNullValue);
+        return &mValue;
+    }
+
   private:
     // Friend is needed so that instances of RefBase<U> can call Reference and Release on
     // RefBase<T>.

src/dawn_native/metal/BackendMTL.mm

@@ -14,6 +14,7 @@
 
 #include "dawn_native/metal/BackendMTL.h"
 
+#include "common/CoreFoundationRef.h"
 #include "common/GPUInfo.h"
 #include "common/NSRef.h"
 #include "common/Platform.h"
@@ -23,6 +24,7 @@
 
 #if defined(DAWN_PLATFORM_MACOS)
 #    import <IOKit/IOKitLib.h>
+#    include "common/IOKitRef.h"
 #endif
 
 namespace dawn_native { namespace metal {
@@ -72,18 +74,17 @@ namespace dawn_native { namespace metal {
 
             // Recursively search registry entry and its parents for property name
             // The data should release with CFRelease
-            CFDataRef data = static_cast<CFDataRef>(IORegistryEntrySearchCFProperty(
+            CFRef<CFDataRef> data =
-                entry, kIOServicePlane, name, kCFAllocatorDefault,
+                AcquireCFRef(static_cast<CFDataRef>(IORegistryEntrySearchCFProperty(
-                kIORegistryIterateRecursively | kIORegistryIterateParents));
+                    entry, kIOServicePlane, name, kCFAllocatorDefault,
+                    kIORegistryIterateRecursively | kIORegistryIterateParents)));
 
             if (data == nullptr) {
                 return value;
             }
 
             // CFDataGetBytePtr() is guaranteed to return a read-only pointer
-            value = *reinterpret_cast<const uint32_t*>(CFDataGetBytePtr(data));
+            value = *reinterpret_cast<const uint32_t*>(CFDataGetBytePtr(data.Get()));
-
-            CFRelease(data);
             return value;
         }
 
@@ -107,38 +108,35 @@ namespace dawn_native { namespace metal {
         MaybeError API_AVAILABLE(macos(10.13))
             GetDeviceIORegistryPCIInfo(id<MTLDevice> device, PCIIDs* ids) {
             // Get a matching dictionary for the IOGraphicsAccelerator2
-            CFMutableDictionaryRef matchingDict = IORegistryEntryIDMatching([device registryID]);
+            CFRef<CFMutableDictionaryRef> matchingDict =
+                AcquireCFRef(IORegistryEntryIDMatching([device registryID]));
             if (matchingDict == nullptr) {
                 return DAWN_INTERNAL_ERROR("Failed to create the matching dict for the device");
             }
 
             // IOServiceGetMatchingService will consume the reference on the matching dictionary,
             // so we don't need to release the dictionary.
-            io_registry_entry_t acceleratorEntry =
+            IORef<io_registry_entry_t> acceleratorEntry = AcquireIORef(
-                IOServiceGetMatchingService(kIOMasterPortDefault, matchingDict);
+                IOServiceGetMatchingService(kIOMasterPortDefault, matchingDict.Detach()));
             if (acceleratorEntry == IO_OBJECT_NULL) {
                 return DAWN_INTERNAL_ERROR(
                     "Failed to get the IO registry entry for the accelerator");
             }
 
             // Get the parent entry that will be the IOPCIDevice
-            io_registry_entry_t deviceEntry = IO_OBJECT_NULL;
+            IORef<io_registry_entry_t> deviceEntry;
-            if (IORegistryEntryGetParentEntry(acceleratorEntry, kIOServicePlane, &deviceEntry) !=
+            if (IORegistryEntryGetParentEntry(acceleratorEntry.Get(), kIOServicePlane,
-                kIOReturnSuccess) {
+                                              deviceEntry.InitializeInto()) != kIOReturnSuccess) {
-                IOObjectRelease(acceleratorEntry);
                 return DAWN_INTERNAL_ERROR("Failed to get the IO registry entry for the device");
             }
 
             ASSERT(deviceEntry != IO_OBJECT_NULL);
-            IOObjectRelease(acceleratorEntry);
 
-            uint32_t vendorId = GetEntryProperty(deviceEntry, CFSTR("vendor-id"));
+            uint32_t vendorId = GetEntryProperty(deviceEntry.Get(), CFSTR("vendor-id"));
-            uint32_t deviceId = GetEntryProperty(deviceEntry, CFSTR("device-id"));
+            uint32_t deviceId = GetEntryProperty(deviceEntry.Get(), CFSTR("device-id"));
 
             *ids = PCIIDs{vendorId, deviceId};
 
-            IOObjectRelease(deviceEntry);
-
             return {};
         }
 

src/tests/unittests/RefCountedTests.cpp

@@ -402,3 +402,17 @@ TEST(Ref, MoveAssignmentDerived) {
     destination = nullptr;
     EXPECT_TRUE(deleted);
 }
+
+// Test Ref's InitializeInto.
+TEST(Ref, InitializeInto) {
+    bool deleted = false;
+    RCTest* original = new RCTest(&deleted);
+
+    // InitializeInto acquires the ref.
+    Ref<RCTest> ref;
+    *ref.InitializeInto() = original;
+    EXPECT_EQ(original->GetRefCountForTesting(), 1u);
+
+    ref = nullptr;
+    EXPECT_TRUE(deleted);
+}