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.
This commit is contained in:
parent
d69e0d0573
commit
5a8f40ae13
5 changed files with 49 additions and 12 deletions
|
@ -101,7 +101,7 @@ BaseWindow::BaseWindow(v8::Isolate* isolate,
|
||||||
#if defined(TOOLKIT_VIEWS)
|
#if defined(TOOLKIT_VIEWS)
|
||||||
v8::Local<v8::Value> icon;
|
v8::Local<v8::Value> icon;
|
||||||
if (options.Get(options::kIcon, &icon)) {
|
if (options.Get(options::kIcon, &icon)) {
|
||||||
SetIcon(isolate, icon);
|
SetIconImpl(isolate, icon, NativeImage::OnConvertError::kWarn);
|
||||||
}
|
}
|
||||||
#endif
|
#endif
|
||||||
}
|
}
|
||||||
|
@ -1009,8 +1009,15 @@ bool BaseWindow::SetThumbarButtons(gin_helper::Arguments* args) {
|
||||||
|
|
||||||
#if defined(TOOLKIT_VIEWS)
|
#if defined(TOOLKIT_VIEWS)
|
||||||
void BaseWindow::SetIcon(v8::Isolate* isolate, v8::Local<v8::Value> icon) {
|
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;
|
NativeImage* native_image = nullptr;
|
||||||
if (!NativeImage::TryConvertNativeImage(isolate, icon, &native_image))
|
if (!NativeImage::TryConvertNativeImage(isolate, icon, &native_image,
|
||||||
|
on_error))
|
||||||
return;
|
return;
|
||||||
|
|
||||||
#if defined(OS_WIN)
|
#if defined(OS_WIN)
|
||||||
|
|
|
@ -224,6 +224,9 @@ class BaseWindow : public gin_helper::TrackableObject<BaseWindow>,
|
||||||
bool SetThumbarButtons(gin_helper::Arguments* args);
|
bool SetThumbarButtons(gin_helper::Arguments* args);
|
||||||
#if defined(TOOLKIT_VIEWS)
|
#if defined(TOOLKIT_VIEWS)
|
||||||
void SetIcon(v8::Isolate* isolate, v8::Local<v8::Value> icon);
|
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
|
#endif
|
||||||
#if defined(OS_WIN)
|
#if defined(OS_WIN)
|
||||||
typedef base::RepeatingCallback<void(v8::Local<v8::Value>,
|
typedef base::RepeatingCallback<void(v8::Local<v8::Value>,
|
||||||
|
|
|
@ -10,6 +10,7 @@
|
||||||
#include <vector>
|
#include <vector>
|
||||||
|
|
||||||
#include "base/files/file_util.h"
|
#include "base/files/file_util.h"
|
||||||
|
#include "base/logging.h"
|
||||||
#include "base/strings/pattern.h"
|
#include "base/strings/pattern.h"
|
||||||
#include "base/strings/string_util.h"
|
#include "base/strings/string_util.h"
|
||||||
#include "base/strings/utf_string_conversions.h"
|
#include "base/strings/utf_string_conversions.h"
|
||||||
|
@ -146,7 +147,10 @@ NativeImage::~NativeImage() {
|
||||||
// static
|
// static
|
||||||
bool NativeImage::TryConvertNativeImage(v8::Isolate* isolate,
|
bool NativeImage::TryConvertNativeImage(v8::Isolate* isolate,
|
||||||
v8::Local<v8::Value> image,
|
v8::Local<v8::Value> image,
|
||||||
NativeImage** native_image) {
|
NativeImage** native_image,
|
||||||
|
OnConvertError on_error) {
|
||||||
|
std::string error_message;
|
||||||
|
|
||||||
base::FilePath icon_path;
|
base::FilePath icon_path;
|
||||||
if (gin::ConvertFromV8(isolate, image, &icon_path)) {
|
if (gin::ConvertFromV8(isolate, image, &icon_path)) {
|
||||||
*native_image = NativeImage::CreateFromPath(isolate, icon_path).get();
|
*native_image = NativeImage::CreateFromPath(isolate, icon_path).get();
|
||||||
|
@ -156,17 +160,27 @@ bool NativeImage::TryConvertNativeImage(v8::Isolate* isolate,
|
||||||
#else
|
#else
|
||||||
const auto img_path = icon_path.value();
|
const auto img_path = icon_path.value();
|
||||||
#endif
|
#endif
|
||||||
isolate->ThrowException(v8::Exception::Error(gin::StringToV8(
|
error_message = "Failed to load image from path '" + img_path + "'";
|
||||||
isolate, "Failed to load image from path '" + img_path + "'")));
|
|
||||||
return false;
|
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
if (!gin::ConvertFromV8(isolate, image, native_image)) {
|
if (!gin::ConvertFromV8(isolate, image, native_image)) {
|
||||||
isolate->ThrowException(v8::Exception::Error(gin::StringToV8(
|
error_message = "Argument must be a file path or a NativeImage";
|
||||||
isolate, "Argument must be a file path or a NativeImage")));
|
|
||||||
return false;
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
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;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -75,9 +75,13 @@ class NativeImage : public gin::Wrappable<NativeImage> {
|
||||||
const gfx::Size& size);
|
const gfx::Size& size);
|
||||||
#endif
|
#endif
|
||||||
|
|
||||||
static bool TryConvertNativeImage(v8::Isolate* isolate,
|
enum class OnConvertError { kThrow, kWarn };
|
||||||
v8::Local<v8::Value> image,
|
|
||||||
NativeImage** native_image);
|
static bool TryConvertNativeImage(
|
||||||
|
v8::Isolate* isolate,
|
||||||
|
v8::Local<v8::Value> image,
|
||||||
|
NativeImage** native_image,
|
||||||
|
OnConvertError on_error = OnConvertError::kThrow);
|
||||||
|
|
||||||
// gin::Wrappable
|
// gin::Wrappable
|
||||||
static gin::WrapperInfo kWrapperInfo;
|
static gin::WrapperInfo kWrapperInfo;
|
||||||
|
|
|
@ -62,6 +62,15 @@ describe('BrowserWindow module', () => {
|
||||||
const appProcess = childProcess.spawn(process.execPath, [appPath]);
|
const appProcess = childProcess.spawn(process.execPath, [appPath]);
|
||||||
await new Promise((resolve) => { appProcess.once('exit', resolve); });
|
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', () => {
|
describe('garbage collection', () => {
|
||||||
|
|
Loading…
Reference in a new issue