fix: app.setLoginItemSettings error getting swallowed by gin conversion (#41647)

* fix: errors getting swallowed by gin conversion

* fix: Windows build

* test: re-enable disabled test

filenames.gni

@@ -583,6 +583,8 @@ filenames = {
     "shell/common/gin_converters/hid_device_info_converter.h",
     "shell/common/gin_converters/image_converter.cc",
     "shell/common/gin_converters/image_converter.h",
+    "shell/common/gin_converters/login_item_settings_converter.cc",
+    "shell/common/gin_converters/login_item_settings_converter.h",
     "shell/common/gin_converters/media_converter.cc",
     "shell/common/gin_converters/media_converter.h",
     "shell/common/gin_converters/message_box_converter.cc",

shell/browser/api/electron_api_app.cc

@@ -89,7 +89,6 @@
 
 #if BUILDFLAG(IS_MAC)
 #include <CoreFoundation/CoreFoundation.h>
-#include "base/mac/mac_util.h"
 #include "shell/browser/ui/cocoa/electron_bundle_mover.h"
 #endif
 
@@ -324,80 +323,6 @@ struct Converter<JumpListResult> {
 };
 #endif
 
-#if BUILDFLAG(IS_WIN)
-template <>
-struct Converter<Browser::LaunchItem> {
-  static bool FromV8(v8::Isolate* isolate,
-                     v8::Local<v8::Value> val,
-                     Browser::LaunchItem* out) {
-    gin_helper::Dictionary dict;
-    if (!ConvertFromV8(isolate, val, &dict))
-      return false;
-
-    dict.Get("name", &(out->name));
-    dict.Get("path", &(out->path));
-    dict.Get("args", &(out->args));
-    dict.Get("scope", &(out->scope));
-    dict.Get("enabled", &(out->enabled));
-    return true;
-  }
-
-  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
-                                   Browser::LaunchItem val) {
-    auto dict = gin_helper::Dictionary::CreateEmpty(isolate);
-    dict.Set("name", val.name);
-    dict.Set("path", val.path);
-    dict.Set("args", val.args);
-    dict.Set("scope", val.scope);
-    dict.Set("enabled", val.enabled);
-    return dict.GetHandle();
-  }
-};
-#endif
-
-template <>
-struct Converter<Browser::LoginItemSettings> {
-  static bool FromV8(v8::Isolate* isolate,
-                     v8::Local<v8::Value> val,
-                     Browser::LoginItemSettings* out) {
-    gin_helper::Dictionary dict;
-    if (!ConvertFromV8(isolate, val, &dict))
-      return false;
-
-    dict.Get("openAtLogin", &(out->open_at_login));
-    dict.Get("openAsHidden", &(out->open_as_hidden));
-    dict.Get("path", &(out->path));
-    dict.Get("args", &(out->args));
-#if BUILDFLAG(IS_WIN)
-    dict.Get("name", &(out->name));
-    dict.Get("enabled", &(out->enabled));
-#elif BUILDFLAG(IS_MAC)
-    dict.Get("serviceName", &(out->service_name));
-    dict.Get("type", &(out->type));
-#endif
-    return true;
-  }
-
-  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
-                                   Browser::LoginItemSettings val) {
-    auto dict = gin_helper::Dictionary::CreateEmpty(isolate);
-#if BUILDFLAG(IS_WIN)
-    dict.Set("launchItems", val.launch_items);
-    dict.Set("executableWillLaunchAtLogin",
-             val.executable_will_launch_at_login);
-#elif BUILDFLAG(IS_MAC)
-    if (base::mac::MacOSMajorVersion() >= 13)
-      dict.Set("status", val.status);
-#endif
-    dict.Set("openAtLogin", val.open_at_login);
-    dict.Set("openAsHidden", val.open_as_hidden);
-    dict.Set("restoreState", val.restore_state);
-    dict.Set("wasOpenedAtLogin", val.opened_at_login);
-    dict.Set("wasOpenedAsHidden", val.opened_as_hidden);
-    return dict.GetHandle();
-  }
-};
-
 template <>
 struct Converter<content::CertificateRequestResultType> {
   static bool FromV8(v8::Isolate* isolate,
@@ -1216,8 +1141,8 @@ void App::SetAccessibilitySupportEnabled(gin_helper::ErrorThrower thrower,
   Browser::Get()->OnAccessibilitySupportChanged();
 }
 
-Browser::LoginItemSettings App::GetLoginItemSettings(gin::Arguments* args) {
+v8::Local<v8::Value> App::GetLoginItemSettings(gin::Arguments* args) {
-  Browser::LoginItemSettings options;
+  LoginItemSettings options;
   args->GetNext(&options);
   return Browser::Get()->GetLoginItemSettings(options);
 }

shell/browser/api/electron_api_app.h

@@ -206,7 +206,7 @@ class App : public ElectronBrowserClient::Delegate,
   bool IsAccessibilitySupportEnabled();
   void SetAccessibilitySupportEnabled(gin_helper::ErrorThrower thrower,
                                       bool enabled);
-  Browser::LoginItemSettings GetLoginItemSettings(gin::Arguments* args);
+  v8::Local<v8::Value> GetLoginItemSettings(gin::Arguments* args);
 #if BUILDFLAG(USE_NSS_CERTS)
   void ImportCertificate(gin_helper::ErrorThrower thrower,
                          base::Value options,

shell/browser/browser.cc

@@ -27,6 +27,16 @@
 
 namespace electron {
 
+LoginItemSettings::LoginItemSettings() = default;
+LoginItemSettings::~LoginItemSettings() = default;
+LoginItemSettings::LoginItemSettings(const LoginItemSettings& other) = default;
+
+#if BUILDFLAG(IS_WIN)
+LaunchItem::LaunchItem() = default;
+LaunchItem::~LaunchItem() = default;
+LaunchItem::LaunchItem(const LaunchItem& other) = default;
+#endif
+
 namespace {
 
 // Call |quit| after Chromium is fully started.
@@ -43,17 +53,6 @@ void RunQuitClosure(base::OnceClosure quit) {
 
 }  // namespace
 
-#if BUILDFLAG(IS_WIN)
-Browser::LaunchItem::LaunchItem() = default;
-Browser::LaunchItem::~LaunchItem() = default;
-Browser::LaunchItem::LaunchItem(const LaunchItem& other) = default;
-#endif
-
-Browser::LoginItemSettings::LoginItemSettings() = default;
-Browser::LoginItemSettings::~LoginItemSettings() = default;
-Browser::LoginItemSettings::LoginItemSettings(const LoginItemSettings& other) =
-    default;
-
 Browser::Browser() {
   WindowList::AddObserver(this);
 }

shell/browser/browser.h

@@ -17,6 +17,7 @@
 #include "gin/dictionary.h"
 #include "shell/browser/browser_observer.h"
 #include "shell/browser/window_list_observer.h"
+#include "shell/common/gin_converters/login_item_settings_converter.h"
 #include "shell/common/gin_helper/promise.h"
 
 #if BUILDFLAG(IS_WIN)
@@ -41,6 +42,48 @@ namespace electron {
 
 class ElectronMenuModel;
 
+#if BUILDFLAG(IS_WIN)
+struct LaunchItem {
+  std::wstring name;
+  std::wstring path;
+  std::wstring scope;
+  std::vector<std::wstring> args;
+  bool enabled = true;
+
+  LaunchItem();
+  ~LaunchItem();
+  LaunchItem(const LaunchItem&);
+};
+#endif
+
+struct LoginItemSettings {
+  bool open_at_login = false;
+  bool open_as_hidden = false;
+  bool restore_state = false;
+  bool opened_at_login = false;
+  bool opened_as_hidden = false;
+  std::u16string path;
+  std::vector<std::u16string> args;
+
+#if BUILDFLAG(IS_MAC)
+  std::string type = "mainAppService";
+  std::string service_name;
+  std::string status;
+#elif BUILDFLAG(IS_WIN)
+  // used in browser::setLoginItemSettings
+  bool enabled = true;
+  std::wstring name;
+
+  // used in browser::getLoginItemSettings
+  bool executable_will_launch_at_login = false;
+  std::vector<LaunchItem> launch_items;
+#endif
+
+  LoginItemSettings();
+  ~LoginItemSettings();
+  LoginItemSettings(const LoginItemSettings&);
+};
+
 // This class is used for control application-wide operations.
 class Browser : public WindowListObserver {
  public:
@@ -112,50 +155,8 @@ class Browser : public WindowListObserver {
   bool SetBadgeCount(std::optional<int> count);
   [[nodiscard]] int badge_count() const { return badge_count_; }
 
-#if BUILDFLAG(IS_WIN)
-  struct LaunchItem {
-    std::wstring name;
-    std::wstring path;
-    std::wstring scope;
-    std::vector<std::wstring> args;
-    bool enabled = true;
-
-    LaunchItem();
-    ~LaunchItem();
-    LaunchItem(const LaunchItem&);
-  };
-#endif
-
-  // Set/Get the login item settings of the app
-  struct LoginItemSettings {
-    bool open_at_login = false;
-    bool open_as_hidden = false;
-    bool restore_state = false;
-    bool opened_at_login = false;
-    bool opened_as_hidden = false;
-    std::u16string path;
-    std::vector<std::u16string> args;
-
-#if BUILDFLAG(IS_MAC)
-    std::string type = "mainAppService";
-    std::string service_name;
-    std::string status;
-#elif BUILDFLAG(IS_WIN)
-    // used in browser::setLoginItemSettings
-    bool enabled = true;
-    std::wstring name;
-
-    // used in browser::getLoginItemSettings
-    bool executable_will_launch_at_login = false;
-    std::vector<LaunchItem> launch_items;
-#endif
-
-    LoginItemSettings();
-    ~LoginItemSettings();
-    LoginItemSettings(const LoginItemSettings&);
-  };
   void SetLoginItemSettings(LoginItemSettings settings);
-  LoginItemSettings GetLoginItemSettings(const LoginItemSettings& options);
+  v8::Local<v8::Value> GetLoginItemSettings(const LoginItemSettings& options);
 
 #if BUILDFLAG(IS_MAC)
   // Set the handler which decides whether to shutdown.

shell/browser/browser_linux.cc

@@ -11,9 +11,11 @@
 #include "base/environment.h"
 #include "base/process/launch.h"
 #include "electron/electron_version.h"
+#include "shell/browser/javascript_environment.h"
 #include "shell/browser/native_window.h"
 #include "shell/browser/window_list.h"
 #include "shell/common/application_info.h"
+#include "shell/common/gin_converters/login_item_settings_converter.h"
 #include "shell/common/thread_restrictions.h"
 
 #if BUILDFLAG(IS_LINUX)
@@ -138,9 +140,10 @@ bool Browser::SetBadgeCount(std::optional<int> count) {
 
 void Browser::SetLoginItemSettings(LoginItemSettings settings) {}
 
-Browser::LoginItemSettings Browser::GetLoginItemSettings(
+v8::Local<v8::Value> Browser::GetLoginItemSettings(
     const LoginItemSettings& options) {
-  return LoginItemSettings();
+  LoginItemSettings settings;
+  return gin::ConvertToV8(JavascriptEnvironment::GetIsolate(), settings);
 }
 
 std::string Browser::GetExecutableFileVersion() const {

shell/browser/browser_mac.mm

@@ -29,6 +29,7 @@
 #include "shell/common/api/electron_api_native_image.h"
 #include "shell/common/application_info.h"
 #include "shell/common/gin_converters/image_converter.h"
+#include "shell/common/gin_converters/login_item_settings_converter.h"
 #include "shell/common/gin_helper/arguments.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/error_thrower.h"
@@ -88,8 +89,8 @@ bool CheckLoginItemStatus(bool* is_hidden) {
   return true;
 }
 
-Browser::LoginItemSettings GetLoginItemSettingsDeprecated() {
+LoginItemSettings GetLoginItemSettingsDeprecated() {
-  Browser::LoginItemSettings settings;
+  LoginItemSettings settings;
   settings.open_at_login = CheckLoginItemStatus(&settings.open_as_hidden);
   settings.restore_state = base::mac::WasLaunchedAsLoginItemRestoreState();
   settings.opened_at_login = base::mac::WasLaunchedAsLoginOrResumeItem();
@@ -375,13 +376,15 @@ void Browser::ApplyForcedRTL() {
   }
 }
 
-Browser::LoginItemSettings Browser::GetLoginItemSettings(
+v8::Local<v8::Value> Browser::GetLoginItemSettings(
     const LoginItemSettings& options) {
   LoginItemSettings settings;
+  v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
+
   if (options.type != "mainAppService" && options.service_name.empty()) {
-    gin_helper::ErrorThrower(JavascriptEnvironment::GetIsolate())
+    gin_helper::ErrorThrower(isolate).ThrowTypeError(
-        .ThrowTypeError("'name' is required when type is not mainAppService");
+        "'name' is required when type is not mainAppService");
-    return settings;
+    return v8::Local<v8::Value>();
   }
 
 #if IS_MAS_BUILD()
@@ -408,7 +411,7 @@ Browser::LoginItemSettings Browser::GetLoginItemSettings(
     settings = settings_deprecated;
   }
 #endif
-  return settings;
+  return gin::ConvertToV8(isolate, settings);
 }
 
 void Browser::SetLoginItemSettings(LoginItemSettings settings) {

shell/browser/browser_win.cc

@@ -30,12 +30,14 @@
 #include "shell/browser/api/electron_api_app.h"
 #include "shell/browser/badging/badge_manager.h"
 #include "shell/browser/electron_browser_main_parts.h"
+#include "shell/browser/javascript_environment.h"
 #include "shell/browser/ui/message_box.h"
 #include "shell/browser/ui/win/jump_list.h"
 #include "shell/browser/window_list.h"
 #include "shell/common/application_info.h"
 #include "shell/common/gin_converters/file_path_converter.h"
 #include "shell/common/gin_converters/image_converter.h"
+#include "shell/common/gin_converters/login_item_settings_converter.h"
 #include "shell/common/gin_helper/arguments.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/skia_util.h"
@@ -154,12 +156,12 @@ bool FormatCommandLineString(std::wstring* exe,
 // a list of launchItem with matching paths to our application.
 // if a launchItem with a matching path also has a matching entry within the
 // startup_approved_key_path, set executable_will_launch_at_login to be `true`
-std::vector<Browser::LaunchItem> GetLoginItemSettingsHelper(
+std::vector<LaunchItem> GetLoginItemSettingsHelper(
     base::win::RegistryValueIterator* it,
     boolean* executable_will_launch_at_login,
     std::wstring scope,
-    const Browser::LoginItemSettings& options) {
+    const LoginItemSettings& options) {
-  std::vector<Browser::LaunchItem> launch_items;
+  std::vector<LaunchItem> launch_items;
 
   base::FilePath lookup_exe_path;
   if (options.path.empty()) {
@@ -183,7 +185,7 @@ std::vector<Browser::LaunchItem> GetLoginItemSettingsHelper(
 
       // add launch item to vector if it has a matching path (case-insensitive)
       if (exe_match) {
-        Browser::LaunchItem launch_item;
+        LaunchItem launch_item;
         launch_item.name = it->Name();
         launch_item.path = registry_launch_path.value();
         launch_item.args = registry_launch_cmd.GetArgs();
@@ -656,7 +658,7 @@ void Browser::SetLoginItemSettings(LoginItemSettings settings) {
   }
 }
 
-Browser::LoginItemSettings Browser::GetLoginItemSettings(
+v8::Local<v8::Value> Browser::GetLoginItemSettings(
     const LoginItemSettings& options) {
   LoginItemSettings settings;
   std::wstring keyPath = L"Software\\Microsoft\\Windows\\CurrentVersion\\Run";
@@ -675,7 +677,7 @@ Browser::LoginItemSettings Browser::GetLoginItemSettings(
   // if there exists a launch entry with property enabled=='true',
   // set executable_will_launch_at_login to 'true'.
   boolean executable_will_launch_at_login = false;
-  std::vector<Browser::LaunchItem> launch_items;
+  std::vector<LaunchItem> launch_items;
   base::win::RegistryValueIterator hkcu_iterator(HKEY_CURRENT_USER,
                                                  keyPath.c_str());
   base::win::RegistryValueIterator hklm_iterator(HKEY_LOCAL_MACHINE,
@@ -683,16 +685,14 @@ Browser::LoginItemSettings Browser::GetLoginItemSettings(
 
   launch_items = GetLoginItemSettingsHelper(
       &hkcu_iterator, &executable_will_launch_at_login, L"user", options);
-  std::vector<Browser::LaunchItem> launch_items_hklm =
+  std::vector<LaunchItem> launch_items_hklm = GetLoginItemSettingsHelper(
-      GetLoginItemSettingsHelper(&hklm_iterator,
+      &hklm_iterator, &executable_will_launch_at_login, L"machine", options);
-                                 &executable_will_launch_at_login, L"machine",
-                                 options);
   launch_items.insert(launch_items.end(), launch_items_hklm.begin(),
                       launch_items_hklm.end());
 
   settings.executable_will_launch_at_login = executable_will_launch_at_login;
   settings.launch_items = launch_items;
-  return settings;
+  return gin::ConvertToV8(JavascriptEnvironment::GetIsolate(), settings);
 }
 
 PCWSTR Browser::GetAppUserModelID() {

shell/common/gin_converters/login_item_settings_converter.cc

@@ -0,0 +1,86 @@
+// Copyright (c) 2024 Microsoft, GmbH.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "shell/common/gin_converters/login_item_settings_converter.h"
+
+#if BUILDFLAG(IS_MAC)
+#include "base/mac/mac_util.h"
+#endif
+
+#include "shell/browser/browser.h"
+#include "shell/common/gin_helper/dictionary.h"
+
+namespace gin {
+
+#if BUILDFLAG(IS_WIN)
+bool Converter<electron::LaunchItem>::FromV8(v8::Isolate* isolate,
+                                             v8::Local<v8::Value> val,
+                                             electron::LaunchItem* out) {
+  gin_helper::Dictionary dict;
+  if (!ConvertFromV8(isolate, val, &dict))
+    return false;
+
+  dict.Get("name", &(out->name));
+  dict.Get("path", &(out->path));
+  dict.Get("args", &(out->args));
+  dict.Get("scope", &(out->scope));
+  dict.Get("enabled", &(out->enabled));
+  return true;
+}
+
+v8::Local<v8::Value> Converter<electron::LaunchItem>::ToV8(
+    v8::Isolate* isolate,
+    electron::LaunchItem val) {
+  auto dict = gin_helper::Dictionary::CreateEmpty(isolate);
+  dict.Set("name", val.name);
+  dict.Set("path", val.path);
+  dict.Set("args", val.args);
+  dict.Set("scope", val.scope);
+  dict.Set("enabled", val.enabled);
+  return dict.GetHandle();
+}
+#endif
+
+bool Converter<electron::LoginItemSettings>::FromV8(
+    v8::Isolate* isolate,
+    v8::Local<v8::Value> val,
+    electron::LoginItemSettings* out) {
+  gin_helper::Dictionary dict;
+  if (!ConvertFromV8(isolate, val, &dict))
+    return false;
+
+  dict.Get("openAtLogin", &(out->open_at_login));
+  dict.Get("openAsHidden", &(out->open_as_hidden));
+  dict.Get("path", &(out->path));
+  dict.Get("args", &(out->args));
+#if BUILDFLAG(IS_WIN)
+  dict.Get("name", &(out->name));
+  dict.Get("enabled", &(out->enabled));
+#elif BUILDFLAG(IS_MAC)
+  dict.Get("serviceName", &(out->service_name));
+  dict.Get("type", &(out->type));
+#endif
+  return true;
+}
+
+v8::Local<v8::Value> Converter<electron::LoginItemSettings>::ToV8(
+    v8::Isolate* isolate,
+    electron::LoginItemSettings val) {
+  auto dict = gin_helper::Dictionary::CreateEmpty(isolate);
+#if BUILDFLAG(IS_WIN)
+  dict.Set("launchItems", val.launch_items);
+  dict.Set("executableWillLaunchAtLogin", val.executable_will_launch_at_login);
+#elif BUILDFLAG(IS_MAC)
+  if (base::mac::MacOSMajorVersion() >= 13)
+    dict.Set("status", val.status);
+#endif
+  dict.Set("openAtLogin", val.open_at_login);
+  dict.Set("openAsHidden", val.open_as_hidden);
+  dict.Set("restoreState", val.restore_state);
+  dict.Set("wasOpenedAtLogin", val.opened_at_login);
+  dict.Set("wasOpenedAsHidden", val.opened_as_hidden);
+  return dict.GetHandle();
+}
+
+}  // namespace gin

shell/common/gin_converters/login_item_settings_converter.h

@@ -0,0 +1,39 @@
+// Copyright (c) 2024 Microsoft, GmbH.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef ELECTRON_SHELL_COMMON_GIN_CONVERTERS_LOGIN_ITEM_SETTINGS_CONVERTER_H_
+#define ELECTRON_SHELL_COMMON_GIN_CONVERTERS_LOGIN_ITEM_SETTINGS_CONVERTER_H_
+
+#include "gin/converter.h"
+
+namespace electron {
+struct LoginItemSettings;
+struct LaunchItem;
+}  // namespace electron
+
+namespace gin {
+
+#if BUILDFLAG(IS_WIN)
+template <>
+struct Converter<electron::LaunchItem> {
+  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
+                                   electron::LaunchItem val);
+  static bool FromV8(v8::Isolate* isolate,
+                     v8::Local<v8::Value> val,
+                     electron::LaunchItem* out);
+};
+#endif
+
+template <>
+struct Converter<electron::LoginItemSettings> {
+  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
+                                   electron::LoginItemSettings val);
+  static bool FromV8(v8::Isolate* isolate,
+                     v8::Local<v8::Value> val,
+                     electron::LoginItemSettings* out);
+};
+
+}  // namespace gin
+
+#endif  // ELECTRON_SHELL_COMMON_GIN_CONVERTERS_LOGIN_ITEM_SETTINGS_CONVERTER_H_

spec/api-app-spec.ts

@@ -760,8 +760,7 @@ describe('app module', () => {
         }).to.throw(/'name' is required when type is not mainAppService/);
       });
 
-      // TODO this test does not work on CircleCI arm64 macs
+      ifit(isVenturaOrHigher)('throws when getting non-default type with no name', () => {
-      ifit(isVenturaOrHigher && process.arch !== 'arm64')('throws when getting non-default type with no name', () => {
         expect(() => {
           app.getLoginItemSettings({
             type: 'daemonService'