Removing bundle from login fails due to errSecParam

As part of the work to support PWA's running at OS login,
I noticed that the call to RemoveFromLoginItems was failing
with errSecParam (-50). The reason appears to be that
the original LSSharedFileListRef is being destroyed, and a new
one is created. This change factors the code so that we
always operate on the same LSSSharedFileListRef which
allows the operation to succeed.

Tested: Manually by mjackson on MacOS 11.0

Bug: 897302
Change-Id: I8d24495b9f3cae3d7df0341a5c5d0cb951e96176
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2522876
Commit-Queue: Mike Jackson <mjackson@microsoft.com>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832372}

base/mac/mac_util.mm

@@ -5,6 +5,7 @@
 #include "base/mac/mac_util.h"
 
 #import <Cocoa/Cocoa.h>
+#include <CoreServices/CoreServices.h>
 #import <IOKit/IOKitLib.h>
 #include <errno.h>
 #include <stddef.h>
@@ -31,46 +32,64 @@ namespace mac {
 
 namespace {
 
-// Looks into Shared File Lists corresponding to Login Items for the item
-// representing the current application.  If such an item is found, returns a
-// retained reference to it. Caller is responsible for releasing the reference.
-LSSharedFileListItemRef GetLoginItemForApp() {
-  ScopedCFTypeRef<LSSharedFileListRef> login_items(LSSharedFileListCreate(
-      NULL, kLSSharedFileListSessionLoginItems, NULL));
-
-  if (!login_items.get()) {
-    DLOG(ERROR) << "Couldn't get a Login Items list.";
-    return NULL;
+class LoginItemsFileList {
+ public:
+  LoginItemsFileList() = default;
+  LoginItemsFileList(const LoginItemsFileList&) = delete;
+  LoginItemsFileList& operator=(const LoginItemsFileList&) = delete;
+  ~LoginItemsFileList() = default;
+
+  bool Initialize() WARN_UNUSED_RESULT {
+    DCHECK(!login_items_.get()) << __func__ << " called more than once.";
+    login_items_.reset(LSSharedFileListCreate(
+        nullptr, kLSSharedFileListSessionLoginItems, nullptr));
+    DLOG_IF(ERROR, !login_items_.get()) << "Couldn't get a Login Items list.";
+    return login_items_.get();
   }
 
-  base::scoped_nsobject<NSArray> login_items_array(
-      CFToNSCast(LSSharedFileListCopySnapshot(login_items, NULL)));
-
-  NSURL* url = [NSURL fileURLWithPath:[base::mac::MainBundle() bundlePath]];
+  LSSharedFileListRef GetLoginFileList() {
+    DCHECK(login_items_.get()) << "Initialize() failed or not called.";
+    return login_items_;
+  }
 
-  for(NSUInteger i = 0; i < [login_items_array count]; ++i) {
-    LSSharedFileListItemRef item =
-        reinterpret_cast<LSSharedFileListItemRef>(login_items_array[i]);
-    base::ScopedCFTypeRef<CFErrorRef> error;
-    CFURLRef item_url_ref =
-        LSSharedFileListItemCopyResolvedURL(item, 0, error.InitializeInto());
-
-    // This function previously used LSSharedFileListItemResolve(), which could
-    // return a NULL URL even when returning no error. This caused
-    // <https://crbug.com/760989>. It's not clear one way or the other whether
-    // LSSharedFileListItemCopyResolvedURL() shares this behavior, so this check
-    // remains in place.
-    if (!error && item_url_ref) {
-      ScopedCFTypeRef<CFURLRef> item_url(item_url_ref);
-      if (CFEqual(item_url, url)) {
-        CFRetain(item);
-        return item;
+  // Looks into Shared File Lists corresponding to Login Items for the item
+  // representing the specified bundle.  If such an item is found, returns a
+  // retained reference to it. Caller is responsible for releasing the
+  // reference.
+  ScopedCFTypeRef<LSSharedFileListItemRef> GetLoginItemForApp() {
+    DCHECK(login_items_.get()) << "Initialize() failed or not called.";
+
+    NSURL* url = [NSURL fileURLWithPath:[base::mac::MainBundle() bundlePath]];
+
+    base::scoped_nsobject<NSArray> login_items_array(
+        CFToNSCast(LSSharedFileListCopySnapshot(login_items_, nullptr)));
+
+    for (id login_item in login_items_array.get()) {
+      LSSharedFileListItemRef item =
+          reinterpret_cast<LSSharedFileListItemRef>(login_item);
+      base::ScopedCFTypeRef<CFErrorRef> error;
+      ScopedCFTypeRef<CFURLRef> item_url(
+          LSSharedFileListItemCopyResolvedURL(item, 0, error.InitializeInto()));
+
+      // This function previously used LSSharedFileListItemResolve(), which
+      // could return a NULL URL even when returning no error. This caused
+      // <https://crbug.com/760989>. It's not clear one way or the other whether
+      // LSSharedFileListItemCopyResolvedURL() shares this behavior, so this
+      // check remains in place.
+      if (!error && item_url) {
+        if (CFEqual(item_url, url)) {
+          return ScopedCFTypeRef<LSSharedFileListItemRef>(
+              item, base::scoped_policy::RETAIN);
+        }
       }
     }
+
+    return ScopedCFTypeRef<LSSharedFileListItemRef>();
   }
 
-  return NULL;
-}
+ private:
+  ScopedCFTypeRef<LSSharedFileListRef> login_items_;
+};
 
 bool IsHiddenLoginItem(LSSharedFileListItemRef item) {
   ScopedCFTypeRef<CFBooleanRef> hidden(reinterpret_cast<CFBooleanRef>(
@@ -143,7 +162,12 @@ bool SetFileBackupExclusion(const FilePath& file_path) {
 }
 
 bool CheckLoginItemStatus(bool* is_hidden) {
-  ScopedCFTypeRef<LSSharedFileListItemRef> item(GetLoginItemForApp());
+  LoginItemsFileList login_items;
+  if (!login_items.Initialize())
+    return false;
+
+  base::ScopedCFTypeRef<LSSharedFileListItemRef> item(
+      login_items.GetLoginItemForApp());
   if (!item.get())
     return false;
 
@@ -154,22 +178,20 @@ bool CheckLoginItemStatus(bool* is_hidden) {
 }
 
 void AddToLoginItems(bool hide_on_startup) {
-  ScopedCFTypeRef<LSSharedFileListItemRef> item(GetLoginItemForApp());
-  if (item.get() && (IsHiddenLoginItem(item) == hide_on_startup)) {
-    return;  // Already is a login item with required hide flag.
-  }
+  LoginItemsFileList login_items;
+  if (!login_items.Initialize())
+    return;
 
-  ScopedCFTypeRef<LSSharedFileListRef> login_items(LSSharedFileListCreate(
-      NULL, kLSSharedFileListSessionLoginItems, NULL));
+  base::ScopedCFTypeRef<LSSharedFileListItemRef> item(
+      login_items.GetLoginItemForApp());
 
-  if (!login_items.get()) {
-    DLOG(ERROR) << "Couldn't get a Login Items list.";
-    return;
+  if (item.get() && (IsHiddenLoginItem(item) == hide_on_startup)) {
+    return;  // Already is a login item with required hide flag.
   }
 
   // Remove the old item, it has wrong hide flag, we'll create a new one.
   if (item.get()) {
-    LSSharedFileListItemRemove(login_items, item);
+    LSSharedFileListItemRemove(login_items.GetLoginFileList(), item);
   }
 
   NSURL* url = [NSURL fileURLWithPath:[base::mac::MainBundle() bundlePath]];
@@ -180,7 +202,7 @@ void AddToLoginItems(bool hide_on_startup) {
 
   ScopedCFTypeRef<LSSharedFileListItemRef> new_item;
   new_item.reset(LSSharedFileListInsertItemURL(
-      login_items, kLSSharedFileListItemLast, NULL, NULL,
+      login_items.GetLoginFileList(), kLSSharedFileListItemLast, NULL, NULL,
       reinterpret_cast<CFURLRef>(url),
       reinterpret_cast<CFDictionaryRef>(properties), NULL));
 
@@ -190,19 +212,16 @@ void AddToLoginItems(bool hide_on_startup) {
 }
 
 void RemoveFromLoginItems() {
-  ScopedCFTypeRef<LSSharedFileListItemRef> item(GetLoginItemForApp());
-  if (!item.get())
+  LoginItemsFileList login_items;
+  if (!login_items.Initialize())
     return;
 
-  ScopedCFTypeRef<LSSharedFileListRef> login_items(LSSharedFileListCreate(
-      NULL, kLSSharedFileListSessionLoginItems, NULL));
-
-  if (!login_items.get()) {
-    DLOG(ERROR) << "Couldn't get a Login Items list.";
+  base::ScopedCFTypeRef<LSSharedFileListItemRef> item(
+      login_items.GetLoginItemForApp());
+  if (!item.get())
     return;
-  }
 
-  LSSharedFileListItemRemove(login_items, item);
+  LSSharedFileListItemRemove(login_items.GetLoginFileList(), item);
 }
 
 bool WasLaunchedAsLoginOrResumeItem() {
@@ -257,7 +276,12 @@ bool WasLaunchedAsHiddenLoginItem() {
   if (!WasLaunchedAsLoginOrResumeItem())
     return false;
 
-  ScopedCFTypeRef<LSSharedFileListItemRef> item(GetLoginItemForApp());
+  LoginItemsFileList login_items;
+  if (!login_items.Initialize())
+    return false;
+
+  base::ScopedCFTypeRef<LSSharedFileListItemRef> item(
+      login_items.GetLoginItemForApp());
   if (!item.get()) {
     // OS X can launch items for the resume feature.
     return false;