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: potential async_hooks crash in NotifyWindowRestore on Windows #40576

Merged
merged 2 commits into from
Jan 26, 2024
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
23 changes: 18 additions & 5 deletions shell/browser/api/electron_api_auto_updater.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,26 @@ void AutoUpdater::OnError(const std::string& message) {
v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
v8::HandleScope handle_scope(isolate);
v8::Local<v8::Object> wrapper;

// We do not use gin::EmitEvent here because we do not want to
// put this in its own CallbackScope and delegate to Node.js'
// specialized handling for Error objects.
if (GetWrapper(isolate).ToLocal(&wrapper)) {
auto error = v8::Exception::Error(gin::StringToV8(isolate, message));
gin_helper::EmitEvent(
isolate, wrapper, "error",
error->ToObject(isolate->GetCurrentContext()).ToLocalChecked(),
// Message is also emitted to keep compatibility with old code.
message);
std::vector<v8::Local<v8::Value>> args = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autoUpdater is then only place in the codebase we use gin to emit a proper Error object. By using gin::EmitEvent to emit the Error object (thus adding a new CallbackScope there), and because Errors are handled specially in Node.js, we would otherwise trigger the uncaught exception handler to bypass the try/catch in our autoUpdate specs.

Either we add CallbackScopes on a case-by-case basis (as i initially tried in this PR) or we account for this edge case. I've chosen to account for this edge case.

gin::StringToV8(isolate, "error"),
gin::ConvertToV8(
isolate,
error->ToObject(isolate->GetCurrentContext()).ToLocalChecked()),
gin::StringToV8(isolate, message),
};

gin_helper::MicrotasksScope microtasks_scope(
isolate, wrapper->GetCreationContextChecked()->GetMicrotaskQueue(),
true);

node::MakeCallback(isolate, wrapper, "emit", args.size(), args.data(),
{0, 0});
}
}

Expand Down
8 changes: 8 additions & 0 deletions shell/common/gin_helper/event_emitter_caller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@ v8::Local<v8::Value> CallMethodWithArgs(v8::Isolate* isolate,
v8::Local<v8::Object> obj,
const char* method,
ValueVector* args) {
// Only set up the node::CallbackScope if there's a node environment.
std::unique_ptr<node::CallbackScope> callback_scope;
if (node::Environment::GetCurrent(isolate)) {
v8::HandleScope handle_scope(isolate);
callback_scope = std::make_unique<node::CallbackScope>(
isolate, v8::Object::New(isolate), node::async_context{0, 0});
}

// Perform microtask checkpoint after running JavaScript.
gin_helper::MicrotasksScope microtasks_scope(
isolate, obj->GetCreationContextChecked()->GetMicrotaskQueue(), true);
Expand Down
8 changes: 3 additions & 5 deletions shell/renderer/electron_api_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,10 @@ void InvokeIpcCallback(v8::Local<v8::Context> context,

// Only set up the node::CallbackScope if there's a node environment.
// Sandboxed renderers don't have a node environment.
node::Environment* env = node::Environment::GetCurrent(context);
std::unique_ptr<node::CallbackScope> callback_scope;
if (env) {
node::async_context async_context = {};
callback_scope = std::make_unique<node::CallbackScope>(isolate, ipcNative,
async_context);
if (node::Environment::GetCurrent(context)) {
callback_scope = std::make_unique<node::CallbackScope>(
isolate, ipcNative, node::async_context{0, 0});
}

auto callback_key = gin::ConvertToV8(isolate, callback_name)
Expand Down
31 changes: 31 additions & 0 deletions spec/api-browser-window-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@
import { ifit, ifdescribe, defer, listen } from './lib/spec-helpers';
import { closeWindow, closeAllWindows } from './lib/window-helpers';
import { areColorsSimilar, captureScreen, HexColors, getPixelColor } from './lib/screen-helpers';
import { once } from 'node:events';
import { setTimeout } from 'node:timers/promises';

Check failure on line 16 in spec/api-browser-window-spec.ts

View check run for this annotation

trop / Backportable? - 26-x-y

spec/api-browser-window-spec.ts#L15-L16

Patch Conflict
Raw output
++<<<<<<< HEAD
 +import { once } from 'events';
 +import { setTimeout } from 'timers/promises';
++=======
+ import { once } from 'node:events';
+ import { setTimeout } from 'node:timers/promises';
+ import { setTimeout as syncSetTimeout } from 'node:timers';
++>>>>>>> fix: potential async_hooks crash in NotifyWindowRestore on Windows
import { setTimeout as syncSetTimeout } from 'node:timers';

const fixtures = path.resolve(__dirname, 'fixtures');
const mainFixtures = path.resolve(__dirname, 'fixtures');
Expand Down Expand Up @@ -1809,6 +1810,36 @@
expect(w.isFullScreen()).to.equal(true);
});

it('does not crash if maximized, minimized, then restored to maximized state', (done) => {
w.destroy();
w = new BrowserWindow({ show: false });

w.show();

let count = 0;

w.on('maximize', () => {
if (count === 0) syncSetTimeout(() => { w.minimize(); });
count++;
});

w.on('minimize', () => {
if (count === 1) syncSetTimeout(() => { w.restore(); });
count++;
});

w.on('restore', () => {
try {
throw new Error('hey!');
} catch (e: any) {
expect(e.message).to.equal('hey!');
done();
}
});

w.maximize();
});

it('checks normal bounds for maximized transparent window', async () => {
w.destroy();
w = new BrowserWindow({
Expand Down