Use ARC in NewDiscovery

It's currently unclear what the best way forward is for passing around
NSWindows in C++ code when gfx/native_widget_types can't be included.
Therefore, in the meanwhile, remove all the memset/memcpy calls from
NewDiscovery. Bridge casts are all that's needed.

Assorted style/ARC fixes are done too:
- Remove scattered __strong qualifiers that are not needed
- Switch to the standard ARC boilerplate for ease of removal later
- Use NOTREACHED_NORETURN() which is now recommended style

Fixed: 1453961
Change-Id: Ie27117e5479004fe368b3d020650cb52fbcd5f28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4659940
Commit-Queue: Adam Langley <agl@chromium.org>
Auto-Submit: Avi Drissman <avi@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1164750}

device/fido/mac/fake_icloud_keychain_sys.h

@@ -16,7 +16,7 @@
 #include "device/fido/mac/icloud_keychain_sys.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
-#if !defined(__OBJC__) || !defined(__has_feature) || !__has_feature(objc_arc)
+#if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
@@ -76,14 +76,12 @@ class API_AVAILABLE(macos(13.3)) FakeSystemInterface : public SystemInterface {
   void MakeCredential(
       NSWindow* window,
       CtapMakeCredentialRequest request,
-      base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>
-          callback) override;
+      base::OnceCallback<void(ASAuthorization*, NSError*)> callback) override;
 
   void GetAssertion(
       NSWindow* window,
       CtapGetAssertionRequest request,
-      base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>
-          callback) override;
+      base::OnceCallback<void(ASAuthorization*, NSError*)> callback) override;
 
  protected:
   friend class base::RefCounted<SystemInterface>;

device/fido/mac/fake_icloud_keychain_sys.mm

@@ -201,8 +201,7 @@ void FakeSystemInterface::GetPlatformCredentials(
 void FakeSystemInterface::MakeCredential(
     NSWindow* window,
     CtapMakeCredentialRequest request,
-    base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>
-        callback) {
+    base::OnceCallback<void(ASAuthorization*, NSError*)> callback) {
   auto attestation_object_bytes =
       std::move(make_credential_attestation_object_bytes_);
   make_credential_attestation_object_bytes_.reset();
@@ -238,8 +237,7 @@ void FakeSystemInterface::MakeCredential(
 void FakeSystemInterface::GetAssertion(
     NSWindow* window,
     CtapGetAssertionRequest request,
-    base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>
-        callback) {
+    base::OnceCallback<void(ASAuthorization*, NSError*)> callback) {
   if (!get_assertion_authenticator_data_) {
     std::move(callback).Run(nullptr, [[NSError alloc] initWithDomain:@""
                                                                 code:1001

device/fido/mac/icloud_keychain.mm

@@ -4,12 +4,12 @@
 
 #include "device/fido/mac/icloud_keychain.h"
 
-#import <AuthenticationServices/ASFoundation.h>
 #import <AuthenticationServices/AuthenticationServices.h>
 
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/notreached.h"
 #include "base/ranges/algorithm.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/sys_string_conversions.h"
@@ -195,10 +195,7 @@ class API_AVAILABLE(macos(13.3)) Authenticator : public FidoAuthenticator {
     return FidoTransportProtocol::kInternal;
   }
 
-  void GetTouch(base::OnceClosure callback) override {
-    NOTREACHED();
-    std::move(callback).Run();
-  }
+  void GetTouch(base::OnceClosure callback) override { NOTREACHED_NORETURN(); }
 
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override {
     return weak_factory_.GetWeakPtr();
@@ -206,8 +203,8 @@ class API_AVAILABLE(macos(13.3)) Authenticator : public FidoAuthenticator {
 
  private:
   void OnMakeCredentialComplete(MakeCredentialCallback callback,
-                                ASAuthorization* __strong authorization,
-                                NSError* __strong error) {
+                                ASAuthorization* authorization,
+                                NSError* error) {
     if (error) {
       const std::string domain = base::SysNSStringToUTF8(error.domain);
       FIDO_LOG(ERROR) << "iCKC: makeCredential failed, domain: " << domain
@@ -279,8 +276,8 @@ class API_AVAILABLE(macos(13.3)) Authenticator : public FidoAuthenticator {
   }
 
   void OnGetAssertionComplete(GetAssertionCallback callback,
-                              ASAuthorization* __strong authorization,
-                              NSError* __strong error) {
+                              ASAuthorization* authorization,
+                              NSError* error) {
     if (error) {
       FIDO_LOG(ERROR) << "iCKC: getAssertion failed, domain: "
                       << base::SysNSStringToUTF8(error.domain)
@@ -334,7 +331,7 @@ class API_AVAILABLE(macos(13.3)) Authenticator : public FidoAuthenticator {
                             std::move(responses));
   }
 
-  NSWindow* const window_;
+  NSWindow* __strong window_;
   base::WeakPtrFactory<Authenticator> weak_factory_{this};
 };
 
@@ -361,7 +358,7 @@ class API_AVAILABLE(macos(13.3)) Discovery : public FidoDiscoveryBase {
                                  {authenticator_.get()});
   }
 
-  NSWindow* const window_;
+  NSWindow* __strong window_;
   std::unique_ptr<Authenticator> authenticator_;
   base::WeakPtrFactory<Discovery> weak_factory_{this};
 };
@@ -377,20 +374,13 @@ bool IsSupported() {
 
 std::unique_ptr<FidoDiscoveryBase> NewDiscovery(uintptr_t ns_window) {
   if (@available(macOS 13.3, *)) {
-    NSWindow* window;
+    NSWindow* window = (__bridge NSWindow*)(void*)ns_window;
     static_assert(sizeof(window) == sizeof(ns_window));
-    memcpy((void*)&window, &ns_window, sizeof(ns_window));
 
-    auto discovery = std::make_unique<Discovery>(window);
-
-    // Clear pointer so that ObjC doesn't try to release it.
-    memset((void*)&window, 0, sizeof(window));
-
-    return discovery;
+    return std::make_unique<Discovery>(window);
   }
 
-  NOTREACHED();
-  return nullptr;
+  NOTREACHED_NORETURN();
 }
 
 }  // namespace device::fido::icloud_keychain

device/fido/mac/icloud_keychain_sys.h

@@ -17,7 +17,7 @@
 #include "device/fido/ctap_get_assertion_request.h"
 #include "device/fido/ctap_make_credential_request.h"
 
-#if !defined(__OBJC__) || !defined(__has_feature) || !__has_feature(objc_arc)
+#if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
@@ -63,14 +63,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) API_AVAILABLE(macos(13.3)) SystemInterface
   virtual void MakeCredential(
       NSWindow* window,
       CtapMakeCredentialRequest request,
-      base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>
-          callback) = 0;
+      base::OnceCallback<void(ASAuthorization*, NSError*)> callback) = 0;
 
   virtual void GetAssertion(
       NSWindow* window,
       CtapGetAssertionRequest request,
-      base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>
-          callback) = 0;
+      base::OnceCallback<void(ASAuthorization*, NSError*)> callback) = 0;
 
  protected:
   friend class base::RefCounted<SystemInterface>;

device/fido/mac/icloud_keychain_sys.mm

@@ -50,20 +50,17 @@ API_AVAILABLE(macos(13.3))
 API_AVAILABLE(macos(13.3))
 @interface ICloudKeychainDelegate : NSObject <ASAuthorizationControllerDelegate>
 - (void)setCallback:
-    (base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>)
-        callback;
+    (base::OnceCallback<void(ASAuthorization*, NSError*)>)callback;
 - (void)setCleanupCallback:(base::OnceClosure)callback;
 @end
 
 @implementation ICloudKeychainDelegate {
-  base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>
-      _callback;
+  base::OnceCallback<void(ASAuthorization*, NSError*)> _callback;
   base::OnceClosure _cleanupCallback;
 }
 
 - (void)setCallback:
-    (base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>)
-        callback {
+    (base::OnceCallback<void(ASAuthorization*, NSError*)>)callback {
   _callback = std::move(callback);
 }
 
@@ -257,8 +254,7 @@ class API_AVAILABLE(macos(13.3)) NativeSystemInterface
   void MakeCredential(
       NSWindow* window,
       CtapMakeCredentialRequest request,
-      base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>
-          callback) override {
+      base::OnceCallback<void(ASAuthorization*, NSError*)> callback) override {
     DCHECK(!create_controller_);
     DCHECK(!get_controller_);
     DCHECK(!delegate_);
@@ -303,8 +299,7 @@ class API_AVAILABLE(macos(13.3)) NativeSystemInterface
   void GetAssertion(
       NSWindow* window,
       CtapGetAssertionRequest request,
-      base::OnceCallback<void(ASAuthorization* __strong, NSError* __strong)>
-          callback) override {
+      base::OnceCallback<void(ASAuthorization*, NSError*)> callback) override {
     DCHECK(!create_controller_);
     DCHECK(!get_controller_);
     DCHECK(!delegate_);

device/fido/mac/icloud_keychain_unittest.mm

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "device/fido/mac/icloud_keychain.h"
+
 #include "base/logging.h"
 #include "base/memory/raw_ptr.h"
 #include "base/ranges/algorithm.h"
@@ -128,12 +129,15 @@ class iCloudKeychainTest : public testing::Test, FidoDiscoveryBase::Observer {
  public:
   void SetUp() override {
     if (@available(macOS 13.3, *)) {
+      NSWindow* window = [[NSWindow alloc] init];
+      window.releasedWhenClosed = NO;  // Required by ARC.
+
       fake_ = base::MakeRefCounted<FakeSystemInterface>();
       SetSystemInterfaceForTesting(fake_);
-      NSWindow* window = [[NSWindow alloc] init];
-      uintptr_t ns_window;
+
+      uintptr_t ns_window = (uintptr_t)(__bridge void*)window;
       static_assert(sizeof(window) == sizeof(ns_window));
-      memcpy(&ns_window, (void*)&window, sizeof(ns_window));
+
       discovery_ = NewDiscovery(ns_window);
       discovery_->set_observer(this);
       discovery_->Start();

device/fido/mac/scoped_icloud_keychain_test_environment.mm

@@ -7,7 +7,7 @@
 #include "device/fido/mac/fake_icloud_keychain_sys.h"
 #include "device/fido/mac/icloud_keychain_sys.h"
 
-#if !defined(__OBJC__) || !defined(__has_feature) || !__has_feature(objc_arc)
+#if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif