fix: don't throw on bad icons in BrowserWindow constructor (#27441)

* fix: do not throw if NativeImage conversion fails. Throwing is an unannounced semver/major breaking change, so revert that behavior but keep the rest of the #26546 refactor. * test: add invalid icon test * refactor: be explicit about when to throw or warn.
electron · Jan 25, 2021 · 5a8f40a · 5a8f40a
1 parent d69e0d0
commit 5a8f40a
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 12 deletions.
diff --git a/shell/browser/api/electron_api_base_window.cc b/shell/browser/api/electron_api_base_window.cc
@@ -101,7 +101,7 @@ BaseWindow::BaseWindow(v8::Isolate* isolate,
 #if defined(TOOLKIT_VIEWS)
   v8::Local<v8::Value> icon;
   if (options.Get(options::kIcon, &icon)) {
-    SetIcon(isolate, icon);
+    SetIconImpl(isolate, icon, NativeImage::OnConvertError::kWarn);
   }
 #endif
 }
@@ -1009,8 +1009,15 @@ bool BaseWindow::SetThumbarButtons(gin_helper::Arguments* args) {
 
 #if defined(TOOLKIT_VIEWS)
 void BaseWindow::SetIcon(v8::Isolate* isolate, v8::Local<v8::Value> icon) {
+  SetIconImpl(isolate, icon, NativeImage::OnConvertError::kThrow);
+}
+
+void BaseWindow::SetIconImpl(v8::Isolate* isolate,
+                             v8::Local<v8::Value> icon,
+                             NativeImage::OnConvertError on_error) {
   NativeImage* native_image = nullptr;
-  if (!NativeImage::TryConvertNativeImage(isolate, icon, &native_image))
+  if (!NativeImage::TryConvertNativeImage(isolate, icon, &native_image,
+                                          on_error))
     return;
 
 #if defined(OS_WIN)

diff --git a/shell/browser/api/electron_api_base_window.h b/shell/browser/api/electron_api_base_window.h
@@ -224,6 +224,9 @@ class BaseWindow : public gin_helper::TrackableObject<BaseWindow>,
   bool SetThumbarButtons(gin_helper::Arguments* args);
 #if defined(TOOLKIT_VIEWS)
   void SetIcon(v8::Isolate* isolate, v8::Local<v8::Value> icon);
+  void SetIconImpl(v8::Isolate* isolate,
+                   v8::Local<v8::Value> icon,
+                   NativeImage::OnConvertError on_error);
 #endif
 #if defined(OS_WIN)
   typedef base::RepeatingCallback<void(v8::Local<v8::Value>,

diff --git a/shell/common/api/electron_api_native_image.cc b/shell/common/api/electron_api_native_image.cc
@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/files/file_util.h"
+#include "base/logging.h"
 #include "base/strings/pattern.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -146,7 +147,10 @@ NativeImage::~NativeImage() {
 // static
 bool NativeImage::TryConvertNativeImage(v8::Isolate* isolate,
                                         v8::Local<v8::Value> image,
-                                        NativeImage** native_image) {
+                                        NativeImage** native_image,
+                                        OnConvertError on_error) {
+  std::string error_message;
+
   base::FilePath icon_path;
   if (gin::ConvertFromV8(isolate, image, &icon_path)) {
     *native_image = NativeImage::CreateFromPath(isolate, icon_path).get();
@@ -156,17 +160,27 @@ bool NativeImage::TryConvertNativeImage(v8::Isolate* isolate,
 #else
       const auto img_path = icon_path.value();
 #endif
-      isolate->ThrowException(v8::Exception::Error(gin::StringToV8(
-          isolate, "Failed to load image from path '" + img_path + "'")));
-      return false;
+      error_message = "Failed to load image from path '" + img_path + "'";
     }
   } else {
     if (!gin::ConvertFromV8(isolate, image, native_image)) {
-      isolate->ThrowException(v8::Exception::Error(gin::StringToV8(
-          isolate, "Argument must be a file path or a NativeImage")));
-      return false;
+      error_message = "Argument must be a file path or a NativeImage";
     }
   }
+
+  if (!error_message.empty()) {
+    switch (on_error) {
+      case OnConvertError::kThrow:
+        isolate->ThrowException(
+            v8::Exception::Error(gin::StringToV8(isolate, error_message)));
+        break;
+      case OnConvertError::kWarn:
+        LOG(WARNING) << error_message;
+        break;
+    }
+    return false;
+  }
+
   return true;
 }
 

diff --git a/shell/common/api/electron_api_native_image.h b/shell/common/api/electron_api_native_image.h
@@ -75,9 +75,13 @@ class NativeImage : public gin::Wrappable<NativeImage> {
       const gfx::Size& size);
 #endif
 
-  static bool TryConvertNativeImage(v8::Isolate* isolate,
-                                    v8::Local<v8::Value> image,
-                                    NativeImage** native_image);
+  enum class OnConvertError { kThrow, kWarn };
+
+  static bool TryConvertNativeImage(
+      v8::Isolate* isolate,
+      v8::Local<v8::Value> image,
+      NativeImage** native_image,
+      OnConvertError on_error = OnConvertError::kThrow);
 
   // gin::Wrappable
   static gin::WrapperInfo kWrapperInfo;

diff --git a/spec-main/api-browser-window-spec.ts b/spec-main/api-browser-window-spec.ts
@@ -62,6 +62,15 @@ describe('BrowserWindow module', () => {
       const appProcess = childProcess.spawn(process.execPath, [appPath]);
       await new Promise((resolve) => { appProcess.once('exit', resolve); });
     });
+
+    it('does not crash or throw when passed an invalid icon', async () => {
+      expect(() => {
+        const w = new BrowserWindow({
+          icon: undefined
+        } as any);
+        w.destroy();
+      }).not.to.throw();
+    });
   });
 
   describe('garbage collection', () => {