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: zombie windows when fullscreening and closing #34392

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
10 changes: 10 additions & 0 deletions shell/browser/native_window_mac.h
Expand Up @@ -176,6 +176,10 @@ class NativeWindowMac : public NativeWindow,
// Handle fullscreen transitions.
void SetFullScreenTransitionState(FullScreenTransitionState state);
void HandlePendingFullscreenTransitions();
bool HandleDeferredClose();
void SetHasDeferredWindowClose(bool defer_close) {
has_deferred_window_close_ = defer_close;
}

enum class VisualEffectState {
kFollowWindow,
Expand Down Expand Up @@ -249,6 +253,12 @@ class NativeWindowMac : public NativeWindow,
FullScreenTransitionState fullscreen_transition_state_ =
FullScreenTransitionState::NONE;

// Trying to close an NSWindow during a fullscreen transition will cause the
// window to lock up. Use this to track if CloseWindow was called during a
// fullscreen transition, to defer the -[NSWindow close] call until the
// transition is complete.
bool has_deferred_window_close_ = false;

NSInteger attention_request_id_ = 0; // identifier from requestUserAttention

// The presentation options before entering kiosk mode.
Expand Down
14 changes: 14 additions & 0 deletions shell/browser/native_window_mac.mm
Expand Up @@ -474,6 +474,11 @@ void ViewDidMoveToSuperview(NSView* self, SEL _cmd) {
return;
}

if (fullscreen_transition_state() != FullScreenTransitionState::NONE) {
SetHasDeferredWindowClose(true);
return;
}

// If a sheet is attached to the window when we call
// [window_ performClose:nil], the window won't close properly
// even after the user has ended the sheet.
Expand Down Expand Up @@ -678,6 +683,15 @@ void ViewDidMoveToSuperview(NSView* self, SEL _cmd) {
SetFullScreen(next_transition);
}

bool NativeWindowMac::HandleDeferredClose() {
if (has_deferred_window_close_) {
SetHasDeferredWindowClose(false);
Close();
return true;
}
return false;
}

void NativeWindowMac::SetFullScreen(bool fullscreen) {
// [NSWindow -toggleFullScreen] is an asynchronous operation, which means
// that it's possible to call it while a fullscreen transition is currently
Expand Down
6 changes: 6 additions & 0 deletions shell/browser/ui/cocoa/electron_ns_window_delegate.mm
Expand Up @@ -248,6 +248,9 @@ - (void)windowDidEnterFullScreen:(NSNotification*)notification {

shell_->NotifyWindowEnterFullScreen();

if (shell_->HandleDeferredClose())
return;

shell_->HandlePendingFullscreenTransitions();
}

Expand All @@ -263,6 +266,9 @@ - (void)windowDidExitFullScreen:(NSNotification*)notification {
shell_->SetResizable(is_resizable_);
shell_->NotifyWindowLeaveFullScreen();

if (shell_->HandleDeferredClose())
return;

shell_->HandlePendingFullscreenTransitions();
}

Expand Down
21 changes: 16 additions & 5 deletions spec-main/webview-spec.ts
Expand Up @@ -434,27 +434,29 @@ describe('<webview> tag', function () {
return [w, webview];
};

afterEach(closeAllWindows);
afterEach(async () => {
// The leaving animation is un-observable but can interfere with future tests
// Specifically this is async on macOS but can be on other platforms too
await delay(1000);

closeAllWindows();
});

// TODO(jkleinsc) fix this test on arm64 macOS. It causes the tests following it to fail/be flaky
ifit(process.platform !== 'darwin' || process.arch !== 'arm64')('should make parent frame element fullscreen too', async () => {
it('should make parent frame element fullscreen too', async () => {
const [w, webview] = await loadWebViewWindow();
expect(await w.webContents.executeJavaScript('isIframeFullscreen()')).to.be.false();

const parentFullscreen = emittedOnce(ipcMain, 'fullscreenchange');
await webview.executeJavaScript('document.getElementById("div").requestFullscreen()', true);
await parentFullscreen;
expect(await w.webContents.executeJavaScript('isIframeFullscreen()')).to.be.true();

w.close();
await emittedOnce(w, 'closed');
});

// FIXME(zcbenz): Fullscreen events do not work on Linux.
// This test is flaky on arm64 macOS.
ifit(process.platform !== 'linux' && process.arch !== 'arm64')('exiting fullscreen should unfullscreen window', async () => {
ifit(process.platform !== 'linux')('exiting fullscreen should unfullscreen window', async () => {
const [w, webview] = await loadWebViewWindow();
const enterFullScreen = emittedOnce(w, 'enter-full-screen');
await webview.executeJavaScript('document.getElementById("div").requestFullscreen()', true);
Expand All @@ -465,6 +467,9 @@ describe('<webview> tag', function () {
await leaveFullScreen;
await delay(0);
expect(w.isFullScreen()).to.be.false();

w.close();
await emittedOnce(w, 'closed');
});

// Sending ESC via sendInputEvent only works on Windows.
Expand All @@ -479,6 +484,9 @@ describe('<webview> tag', function () {
await leaveFullScreen;
await delay(0);
expect(w.isFullScreen()).to.be.false();

w.close();
await emittedOnce(w, 'closed');
});

it('pressing ESC should emit the leave-html-full-screen event', async () => {
Expand Down Expand Up @@ -507,6 +515,9 @@ describe('<webview> tag', function () {
webContents.sendInputEvent({ type: 'keyDown', keyCode: 'Escape' });
await leaveFSWindow;
await leaveFSWebview;

w.close();
await emittedOnce(w, 'closed');
});
});

Expand Down