Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Jan 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions shell/browser/api/electron_api_base_window.cc
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions shell/browser/api/electron_api_base_window.h
Expand Up @@ -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>,
Expand Down
28 changes: 21 additions & 7 deletions shell/common/api/electron_api_native_image.cc
Expand Up @@ -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"
Expand Down Expand Up @@ -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();
Expand All @@ -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;
}

Expand Down
10 changes: 7 additions & 3 deletions shell/common/api/electron_api_native_image.h
Expand Up @@ -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;
Expand Down
9 changes: 9 additions & 0 deletions spec-main/api-browser-window-spec.ts
Expand Up @@ -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', () => {
Expand Down