From e5334a5a1819fcb205450a4d18ac97735fdc174e Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Fri, 5 May 2023 14:17:14 +0300 Subject: [PATCH] mac: Enable ARC in libcef (fixes #3496) Also enables debugging of zombie Objective-C objects. --- BUILD.gn | 3 ++ .../browser_platform_delegate_native_mac.h | 10 +++- .../browser_platform_delegate_native_mac.mm | 45 +++++++++------- libcef/browser/native/cursor_util_mac.mm | 14 +++-- .../native/javascript_dialog_runner_mac.h | 3 +- .../native/javascript_dialog_runner_mac.mm | 54 ++++++++++--------- libcef/browser/native/menu_runner_mac.h | 4 +- libcef/browser/native/menu_runner_mac.mm | 23 ++++---- libcef/browser/views/native_widget_mac.mm | 9 ++-- libcef/common/alloy/alloy_main_delegate.cc | 10 +++- 10 files changed, 105 insertions(+), 70 deletions(-) diff --git a/BUILD.gn b/BUILD.gn index 3271460b0..de84119d0 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -1156,6 +1156,8 @@ source_set("libcef_static") { "//chrome/app/chrome_main_mac.h", "//chrome/app/chrome_main_mac.mm", ] + + configs += [ "//build/config/compiler:enable_arc" ] } if (ozone_platform_x11) { @@ -1602,6 +1604,7 @@ if (is_mac) { configs += [ ":libcef_autogen_config", + "//build/config/compiler:enable_arc", ] # We don't link the framework so just use the path from the main executable. diff --git a/libcef/browser/native/browser_platform_delegate_native_mac.h b/libcef/browser/native/browser_platform_delegate_native_mac.h index ba7211df3..4d88dd86a 100644 --- a/libcef/browser/native/browser_platform_delegate_native_mac.h +++ b/libcef/browser/native/browser_platform_delegate_native_mac.h @@ -7,6 +7,12 @@ #include "libcef/browser/native/browser_platform_delegate_native.h" +#if defined(__OBJC__) +@class CefWindowDelegate; +#else +class CefWindowDelegate; +#endif + namespace content { class RenderWidgetHostViewMac; } @@ -67,7 +73,9 @@ class CefBrowserPlatformDelegateNativeMac content::RenderWidgetHostViewMac* GetHostView() const; // True if the host window has been created. - bool host_window_created_; + bool host_window_created_ = false; + + CefWindowDelegate* __strong window_delegate_; }; #endif // CEF_LIBCEF_BROWSER_NATIVE_BROWSER_PLATFORM_DELEGATE_NATIVE_MAC_H_ diff --git a/libcef/browser/native/browser_platform_delegate_native_mac.mm b/libcef/browser/native/browser_platform_delegate_native_mac.mm index 0e07c8411..95f36f067 100644 --- a/libcef/browser/native/browser_platform_delegate_native_mac.mm +++ b/libcef/browser/native/browser_platform_delegate_native_mac.mm @@ -7,6 +7,7 @@ #import #import +#include "include/internal/cef_types_mac.h" #include "libcef/browser/alloy/alloy_browser_host_impl.h" #include "libcef/browser/context.h" #include "libcef/browser/native/javascript_dialog_runner_mac.h" @@ -28,6 +29,10 @@ #include "ui/events/keycodes/keyboard_codes_posix.h" #include "ui/gfx/geometry/rect.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + // Wrapper NSView for the native view. Necessary to destroy the browser when // the view is deleted. @interface CefBrowserHostView : NSView { @@ -49,8 +54,6 @@ // PlatformCreateWindow(). static_cast(browser_)->WindowDestroyed(); } - - [super dealloc]; } @end @@ -59,7 +62,7 @@ @interface CefWindowDelegate : NSObject { @private CefBrowserHostBase* browser_; // weak - NSWindow* window_; + NSWindow* __strong window_; } - (id)initWithWindow:(NSWindow*)window andBrowser:(CefBrowserHostBase*)browser; @end @@ -70,16 +73,12 @@ if (self = [super init]) { window_ = window; browser_ = browser; - - [window_ setDelegate:self]; } return self; } - (void)dealloc { [[NSNotificationCenter defaultCenter] removeObserver:self]; - - [super dealloc]; } - (BOOL)windowShouldClose:(id)window { @@ -88,6 +87,11 @@ return NO; } + // For an NSWindow object, the default is to be released on |close|. We + // instead want it to remain valid until all strong references are released + // via |cleanup:| and |BrowserDestroyed|. + ((NSWindow*)window).releasedWhenClosed = NO; + // Clean ourselves up after clearing the stack of anything that might have the // window on it. [self performSelectorOnMainThread:@selector(cleanup:) @@ -100,7 +104,7 @@ - (void)cleanup:(id)window { [window_ setDelegate:nil]; - [self release]; + window_ = nil; } @end @@ -239,16 +243,16 @@ void GetNSBoundsInDisplay(const gfx::Rect& dip_bounds, CefBrowserPlatformDelegateNativeMac::CefBrowserPlatformDelegateNativeMac( const CefWindowInfo& window_info, SkColor background_color) - : CefBrowserPlatformDelegateNative(window_info, background_color), - host_window_created_(false) {} + : CefBrowserPlatformDelegateNative(window_info, background_color) {} void CefBrowserPlatformDelegateNativeMac::BrowserDestroyed( CefBrowserHostBase* browser) { CefBrowserPlatformDelegateNative::BrowserDestroyed(browser); if (host_window_created_) { - // Release the reference added in CreateHostWindow(). + // Release the references added in CreateHostWindow(). browser->Release(); + window_delegate_ = nil; } } @@ -285,12 +289,15 @@ bool CefBrowserPlatformDelegateNativeMac::CreateHostWindow() { defer:NO]; // Create the delegate for control and browser window events. - [[CefWindowDelegate alloc] initWithWindow:new_window andBrowser:browser_]; + // Add a reference that will be released in BrowserDestroyed(). + window_delegate_ = [[CefWindowDelegate alloc] initWithWindow:new_window + andBrowser:browser_]; + [new_window setDelegate:window_delegate_]; parent_view = [new_window contentView]; browser_view_rect = [parent_view bounds]; - window_info_.parent_view = parent_view; + window_info_.parent_view = CAST_NSVIEW_TO_CEF_WINDOW_HANDLE(parent_view); // Make the content view for the window have a layer. This will make all // sub-views have layers. This is necessary to ensure correct layer @@ -314,7 +321,6 @@ bool CefBrowserPlatformDelegateNativeMac::CreateHostWindow() { [parent_view addSubview:browser_view]; [browser_view setAutoresizingMask:(NSViewWidthSizable | NSViewHeightSizable)]; [browser_view setNeedsDisplay:YES]; - [browser_view release]; // Parent the WebContents to the browser view. const NSRect bounds = [browser_view bounds]; @@ -324,7 +330,7 @@ bool CefBrowserPlatformDelegateNativeMac::CreateHostWindow() { [native_view setAutoresizingMask:(NSViewWidthSizable | NSViewHeightSizable)]; [native_view setNeedsDisplay:YES]; - window_info_.view = browser_view; + window_info_.view = CAST_NSVIEW_TO_CEF_WINDOW_HANDLE(browser_view); if (new_window != nil && !window_info_.hidden) { // Show the window. @@ -461,7 +467,7 @@ void CefBrowserPlatformDelegate::HandleExternalProtocol(const GURL& url) {} CefEventHandle CefBrowserPlatformDelegateNativeMac::GetEventHandle( const content::NativeWebKeyboardEvent& event) const { - return event.os_event; + return CAST_NSEVENT_TO_CEF_EVENT_HANDLE(event.os_event); } std::unique_ptr @@ -507,11 +513,10 @@ CefBrowserPlatformDelegateNativeMac::TranslateWebKeyEvent( } NSString* charactersIgnoringModifiers = - [[[NSString alloc] initWithCharacters:&key_event.unmodified_character - length:1] autorelease]; + [[NSString alloc] initWithCharacters:&key_event.unmodified_character + length:1]; NSString* characters = - [[[NSString alloc] initWithCharacters:&key_event.character - length:1] autorelease]; + [[NSString alloc] initWithCharacters:&key_event.character length:1]; NSEvent* synthetic_event = [NSEvent keyEventWithType:event_type diff --git a/libcef/browser/native/cursor_util_mac.mm b/libcef/browser/native/cursor_util_mac.mm index c2ed9ac50..2789ae625 100644 --- a/libcef/browser/native/cursor_util_mac.mm +++ b/libcef/browser/native/cursor_util_mac.mm @@ -2,11 +2,15 @@ // Use of this source code is governed by a BSD-style license that can be found // in the LICENSE file. +#include "include/internal/cef_types_mac.h" #include "libcef/browser/native/cursor_util.h" -#import "base/mac/scoped_nsobject.h" #import "ui/base/cocoa/cursor_utils.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace cursor_util { namespace { @@ -15,14 +19,16 @@ class ScopedCursorHandleImpl : public ScopedCursorHandle { public: explicit ScopedCursorHandleImpl(NSCursor* native_cursor) { if (native_cursor) { - cursor_.reset([native_cursor retain]); + cursor_ = native_cursor; } } - cef_cursor_handle_t GetCursorHandle() override { return cursor_.get(); } + cef_cursor_handle_t GetCursorHandle() override { + return CAST_NSCURSOR_TO_CEF_CURSOR_HANDLE(cursor_); + } private: - base::scoped_nsobject cursor_; + NSCursor* __strong cursor_; }; } // namespace diff --git a/libcef/browser/native/javascript_dialog_runner_mac.h b/libcef/browser/native/javascript_dialog_runner_mac.h index 6cfc99d6b..828c08fb4 100644 --- a/libcef/browser/native/javascript_dialog_runner_mac.h +++ b/libcef/browser/native/javascript_dialog_runner_mac.h @@ -10,7 +10,6 @@ #include "libcef/browser/javascript_dialog_runner.h" #include "base/functional/callback.h" -#include "base/mac/scoped_nsobject.h" #include "base/memory/weak_ptr.h" #if __OBJC__ @@ -40,7 +39,7 @@ class CefJavaScriptDialogRunnerMac : public CefJavaScriptDialogRunner { private: DialogClosedCallback callback_; - base::scoped_nsobject helper_; + CefJavaScriptDialogHelper* __strong helper_; // Must be the last member. base::WeakPtrFactory weak_ptr_factory_; diff --git a/libcef/browser/native/javascript_dialog_runner_mac.mm b/libcef/browser/native/javascript_dialog_runner_mac.mm index 5769aff89..9ec8e4587 100644 --- a/libcef/browser/native/javascript_dialog_runner_mac.mm +++ b/libcef/browser/native/javascript_dialog_runner_mac.mm @@ -12,12 +12,16 @@ #include "base/strings/utf_string_conversions.h" #include "components/url_formatter/elide_url.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + // Helper object that receives the notification that the dialog/sheet is // going away. Is responsible for cleaning itself up. @interface CefJavaScriptDialogHelper : NSObject { @private - base::scoped_nsobject alert_; - NSTextField* textField_; // WEAK; owned by alert_ + NSAlert* __strong alert_; + NSTextField* __weak textField_; // Copies of the fields in CefJavaScriptDialog because they're private. CefJavaScriptDialogRunner::DialogClosedCallback callback_; @@ -46,18 +50,20 @@ } - (NSAlert*)alert { - alert_.reset([[NSAlert alloc] init]); + alert_ = [[NSAlert alloc] init]; return alert_; } - (NSTextField*)textField { - textField_ = [[NSTextField alloc] initWithFrame:NSMakeRect(0, 0, 300, 22)]; - [[textField_ cell] setLineBreakMode:NSLineBreakByTruncatingTail]; - [alert_ setAccessoryView:textField_]; - [[alert_ window] setInitialFirstResponder:textField_]; - [textField_ release]; + NSTextField* textField = + [[NSTextField alloc] initWithFrame:NSMakeRect(0, 0, 300, 22)]; + textField.cell.lineBreakMode = NSLineBreakByTruncatingTail; - return textField_; + alert_.accessoryView = textField; + alert_.window.initialFirstResponder = textField; + + textField_ = textField; + return textField; } - (void)alertDidEnd:(NSAlert*)alert @@ -70,15 +76,15 @@ bool success = returnCode == NSAlertFirstButtonReturn; std::u16string input; if (textField_) { - input = base::SysNSStringToUTF16([textField_ stringValue]); + input = base::SysNSStringToUTF16(textField_.stringValue); } std::move(callback_).Run(success, input); } - (void)cancel { - [NSApp endSheet:[alert_ window]]; - alert_.reset(); + [NSApp endSheet:alert_.window]; + alert_ = nil; } @end @@ -97,26 +103,26 @@ void CefJavaScriptDialogRunnerMac::Run( const std::u16string& message_text, const std::u16string& default_prompt_text, DialogClosedCallback callback) { - DCHECK(!helper_.get()); + DCHECK(!helper_); callback_ = std::move(callback); bool text_field = message_type == content::JAVASCRIPT_DIALOG_TYPE_PROMPT; bool one_button = message_type == content::JAVASCRIPT_DIALOG_TYPE_ALERT; - helper_.reset([[CefJavaScriptDialogHelper alloc] + helper_ = [[CefJavaScriptDialogHelper alloc] initHelperWithCallback:base::BindOnce( &CefJavaScriptDialogRunnerMac::DialogClosed, - weak_ptr_factory_.GetWeakPtr())]); + weak_ptr_factory_.GetWeakPtr())]; // Show the modal dialog. NSAlert* alert = [helper_ alert]; NSTextField* field = nil; if (text_field) { field = [helper_ textField]; - [field setStringValue:base::SysUTF16ToNSString(default_prompt_text)]; + field.stringValue = base::SysUTF16ToNSString(default_prompt_text); } - [alert setDelegate:helper_]; - [alert setInformativeText:base::SysUTF16ToNSString(message_text)]; + alert.delegate = helper_; + alert.informativeText = base::SysUTF16ToNSString(message_text); std::u16string label; switch (message_type) { @@ -137,12 +143,12 @@ void CefJavaScriptDialogRunnerMac::Run( label += u" - " + display_url; } - [alert setMessageText:base::SysUTF16ToNSString(label)]; + alert.messageText = base::SysUTF16ToNSString(label); [alert addButtonWithTitle:@"OK"]; if (!one_button) { NSButton* other = [alert addButtonWithTitle:@"Cancel"]; - [other setKeyEquivalent:@"\e"]; + other.keyEquivalent = @"\e"; } // Calling beginSheetModalForWindow:nil is wrong API usage. For now work @@ -168,21 +174,21 @@ void CefJavaScriptDialogRunnerMac::Run( void CefJavaScriptDialogRunnerMac::Handle( bool accept, const std::u16string* prompt_override) { - if (helper_.get()) { + if (helper_) { DialogClosed(accept, prompt_override ? *prompt_override : std::u16string()); } } void CefJavaScriptDialogRunnerMac::Cancel() { - if (helper_.get()) { + if (helper_) { [helper_ cancel]; - helper_.reset(nil); + helper_ = nil; } } void CefJavaScriptDialogRunnerMac::DialogClosed( bool success, const std::u16string& user_input) { - helper_.reset(nil); + helper_ = nil; std::move(callback_).Run(success, user_input); } diff --git a/libcef/browser/native/menu_runner_mac.h b/libcef/browser/native/menu_runner_mac.h index 44f3ef50b..125a451fe 100644 --- a/libcef/browser/native/menu_runner_mac.h +++ b/libcef/browser/native/menu_runner_mac.h @@ -8,8 +8,6 @@ #include "libcef/browser/menu_runner.h" -#include "base/mac/scoped_nsobject.h" - #if __OBJC__ @class MenuControllerCocoa; #else @@ -28,7 +26,7 @@ class CefMenuRunnerMac : public CefMenuRunner { void CancelContextMenu() override; private: - base::scoped_nsobject menu_controller_; + MenuControllerCocoa* __strong menu_controller_; }; #endif // CEF_LIBCEF_BROWSER_NATIVE_MENU_RUNNER_MAC_H_ diff --git a/libcef/browser/native/menu_runner_mac.mm b/libcef/browser/native/menu_runner_mac.mm index 9b3c766a4..1812621af 100644 --- a/libcef/browser/native/menu_runner_mac.mm +++ b/libcef/browser/native/menu_runner_mac.mm @@ -11,6 +11,10 @@ #import "ui/base/cocoa/menu_controller.h" #include "ui/gfx/geometry/point.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + CefMenuRunnerMac::CefMenuRunnerMac() {} CefMenuRunnerMac::~CefMenuRunnerMac() {} @@ -20,17 +24,12 @@ bool CefMenuRunnerMac::RunContextMenu( CefMenuModelImpl* model, const content::ContextMenuParams& params) { // Create a menu controller based on the model. - menu_controller_.reset([[MenuControllerCocoa alloc] - initWithModel:model->model() - delegate:nil - useWithPopUpButtonCell:NO]); + MenuControllerCocoa* menu_controller = + [[MenuControllerCocoa alloc] initWithModel:model->model() + delegate:nil + useWithPopUpButtonCell:NO]; - // Keep the menu controller alive (by adding an additional retain) until after - // the menu has been dismissed. Otherwise it will crash if the browser is - // destroyed (and consequently the menu controller is destroyed) while the - // menu is still pending. - base::scoped_nsobject menu_controller_ref( - menu_controller_); + menu_controller_ = menu_controller; // Make sure events can be pumped while the menu is up. base::CurrentThread::ScopedAllowApplicationTasksInNativeNestedLoop allow; @@ -86,7 +85,7 @@ bool CefMenuRunnerMac::RunContextMenu( } void CefMenuRunnerMac::CancelContextMenu() { - if (menu_controller_.get()) { - [menu_controller_ cancel]; + if (menu_controller_) { + menu_controller_ = nil; } } diff --git a/libcef/browser/views/native_widget_mac.mm b/libcef/browser/views/native_widget_mac.mm index fa9959a88..9edb766db 100644 --- a/libcef/browser/views/native_widget_mac.mm +++ b/libcef/browser/views/native_widget_mac.mm @@ -17,6 +17,10 @@ #import "components/remote_cocoa/app_shim/native_widget_ns_window_bridge.h" #import "ui/views/cocoa/native_widget_mac_ns_window_host.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { AppShimHost* GetHostForBrowser(Browser* browser) { @@ -133,9 +137,8 @@ void CefNativeWidgetMac::OnWindowInitialized() { // From BrowserFrameMac::OnWindowInitialized. if (auto* bridge = GetInProcessNSWindowBridge()) { - bridge->SetCommandDispatcher( - [[[ChromeCommandDispatcherDelegate alloc] init] autorelease], - [[[BrowserWindowCommandHandler alloc] init] autorelease]); + bridge->SetCommandDispatcher([[ChromeCommandDispatcherDelegate alloc] init], + [[BrowserWindowCommandHandler alloc] init]); } else { if (auto* host = GetHostForBrowser(browser_view_->browser())) { host->GetAppShim()->CreateCommandDispatcherForWidget( diff --git a/libcef/common/alloy/alloy_main_delegate.cc b/libcef/common/alloy/alloy_main_delegate.cc index ab6ee426f..e90d8f192 100644 --- a/libcef/common/alloy/alloy_main_delegate.cc +++ b/libcef/common/alloy/alloy_main_delegate.cc @@ -49,6 +49,7 @@ #include "ui/base/ui_base_switches.h" #if BUILDFLAG(IS_MAC) +#include "components/crash/core/common/objc_zombie.h" #include "libcef/common/util_mac.h" #elif BUILDFLAG(IS_POSIX) #include "libcef/common/util_linux.h" @@ -94,7 +95,8 @@ absl::optional AlloyMainDelegate::BasicStartupComplete() { crash_reporting::BasicStartupComplete(command_line); #endif - if (process_type.empty()) { + const bool is_browser = process_type.empty(); + if (is_browser) { // In the browser process. Populate the global command-line object. if (settings_->command_line_args_disabled) { // Remove any existing command-line arguments. @@ -293,6 +295,12 @@ absl::optional AlloyMainDelegate::BasicStartupComplete() { std::ignore = commandLinePtr->Detach(nullptr); } +#if BUILDFLAG(IS_MAC) + // Turns all deallocated Objective-C objects into zombies. Give the browser + // process a longer treadmill, since crashes there have more impact. + ObjcEvilDoers::ZombieEnable(true, is_browser ? 10000 : 1000); +#endif + // Initialize logging. logging::LoggingSettings log_settings;