From 441cc3e57ea926a9cc2b27f5d6e8a67b7abe2de3 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 4 Nov 2015 16:50:19 +0800 Subject: [PATCH 1/5] Style fix --- atom/browser/api/lib/app.coffee | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/atom/browser/api/lib/app.coffee b/atom/browser/api/lib/app.coffee index 18c80dc2b1f4..cc676df0dd5a 100644 --- a/atom/browser/api/lib/app.coffee +++ b/atom/browser/api/lib/app.coffee @@ -11,14 +11,14 @@ wrapSession = (session) -> # session is an Event Emitter. session.__proto__ = EventEmitter.prototype -wrapDownloadItem = (download_item) -> - # download_item is an Event Emitter. - download_item.__proto__ = EventEmitter.prototype +wrapDownloadItem = (downloadItem) -> + # downloadItem is an Event Emitter. + downloadItem.__proto__ = EventEmitter.prototype # Be compatible with old APIs. - download_item.url = download_item.getUrl() - download_item.filename = download_item.getFilename() - download_item.mimeType = download_item.getMimeType() - download_item.hasUserGesture = download_item.hasUserGesture() + downloadItem.url = downloadItem.getUrl() + downloadItem.filename = downloadItem.getFilename() + downloadItem.mimeType = downloadItem.getMimeType() + downloadItem.hasUserGesture = downloadItem.hasUserGesture() app.setApplicationMenu = (menu) -> require('menu').setApplicationMenu menu @@ -57,7 +57,7 @@ app.setDataPath = (path) -> @setPath 'userData', path app.resolveProxy = -> @defaultSession.resolveProxy.apply @defaultSession, arguments app.on 'activate', (event, hasVisibleWindows) -> @emit 'activate-with-no-open-windows' if not hasVisibleWindows -# Session wrapper. +# Wrappers for native classes. sessionBindings._setWrapSession wrapSession process.once 'exit', sessionBindings._clearWrapSession From 75f49477ca4e2d25c6dfb4d19440325d84d5a3b5 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 4 Nov 2015 16:54:36 +0800 Subject: [PATCH 2/5] Cleanup wrapper function when app has done quitting The app may still do something when quitting, we need to make sure the wrapper function is still there at that time. --- atom/browser/api/atom_api_download_item.cc | 22 +++++++++++++--------- atom/browser/api/atom_api_session.cc | 16 ++++++++++------ atom/browser/api/atom_api_web_contents.cc | 13 ++++++++----- atom/browser/api/lib/app.coffee | 3 --- atom/browser/api/lib/web-contents.coffee | 1 - 5 files changed, 31 insertions(+), 24 deletions(-) diff --git a/atom/browser/api/atom_api_download_item.cc b/atom/browser/api/atom_api_download_item.cc index ec4dcd84b285..691cfbfef594 100644 --- a/atom/browser/api/atom_api_download_item.cc +++ b/atom/browser/api/atom_api_download_item.cc @@ -6,6 +6,7 @@ #include +#include "atom/browser/atom_browser_main_parts.h" #include "atom/common/native_mate_converters/callback.h" #include "atom/common/native_mate_converters/file_path_converter.h" #include "atom/common/native_mate_converters/gurl_converter.h" @@ -159,14 +160,6 @@ mate::ObjectTemplateBuilder DownloadItem::GetObjectTemplateBuilder( .SetMethod("setSavePath", &DownloadItem::SetSavePath); } -void SetWrapDownloadItem(const WrapDownloadItemCallback& callback) { - g_wrap_download_item = callback; -} - -void ClearWrapDownloadItem() { - g_wrap_download_item.Reset(); -} - // static mate::Handle DownloadItem::Create( v8::Isolate* isolate, content::DownloadItem* item) { @@ -182,6 +175,18 @@ void* DownloadItem::UserDataKey() { return &kDownloadItemSavePathKey; } +void ClearWrapDownloadItem() { + g_wrap_download_item.Reset(); +} + +void SetWrapDownloadItem(const WrapDownloadItemCallback& callback) { + g_wrap_download_item = callback; + + // Cleanup the wrapper on exit. + atom::AtomBrowserMainParts::Get()->RegisterDestructionCallback( + base::Bind(ClearWrapDownloadItem)); +} + } // namespace api } // namespace atom @@ -193,7 +198,6 @@ void Initialize(v8::Local exports, v8::Local unused, v8::Isolate* isolate = context->GetIsolate(); mate::Dictionary dict(isolate, exports); dict.SetMethod("_setWrapDownloadItem", &atom::api::SetWrapDownloadItem); - dict.SetMethod("_clearWrapDownloadItem", &atom::api::ClearWrapDownloadItem); } } // namespace diff --git a/atom/browser/api/atom_api_session.cc b/atom/browser/api/atom_api_session.cc index 5bb96710f226..0ec9c05ed84e 100644 --- a/atom/browser/api/atom_api_session.cc +++ b/atom/browser/api/atom_api_session.cc @@ -9,9 +9,10 @@ #include "atom/browser/api/atom_api_cookies.h" #include "atom/browser/api/atom_api_download_item.h" -#include "atom/browser/atom_browser_context.h" #include "atom/browser/api/atom_api_web_contents.h" #include "atom/browser/api/save_page_handler.h" +#include "atom/browser/atom_browser_context.h" +#include "atom/browser/atom_browser_main_parts.h" #include "atom/common/native_mate_converters/callback.h" #include "atom/common/native_mate_converters/gurl_converter.h" #include "atom/common/native_mate_converters/file_path_converter.h" @@ -395,14 +396,18 @@ mate::Handle Session::FromPartition( static_cast(browser_context.get())); } -void SetWrapSession(const WrapSessionCallback& callback) { - g_wrap_session = callback; -} - void ClearWrapSession() { g_wrap_session.Reset(); } +void SetWrapSession(const WrapSessionCallback& callback) { + g_wrap_session = callback; + + // Cleanup the wrapper on exit. + atom::AtomBrowserMainParts::Get()->RegisterDestructionCallback( + base::Bind(ClearWrapSession)); +} + } // namespace api } // namespace atom @@ -415,7 +420,6 @@ void Initialize(v8::Local exports, v8::Local unused, mate::Dictionary dict(isolate, exports); dict.SetMethod("fromPartition", &atom::api::Session::FromPartition); dict.SetMethod("_setWrapSession", &atom::api::SetWrapSession); - dict.SetMethod("_clearWrapSession", &atom::api::ClearWrapSession); } } // namespace diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index c329b257a290..5165877d1670 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -1124,14 +1124,18 @@ mate::Handle WebContents::Create( return handle; } -void SetWrapWebContents(const WrapWebContentsCallback& callback) { - g_wrap_web_contents = callback; -} - void ClearWrapWebContents() { g_wrap_web_contents.Reset(); } +void SetWrapWebContents(const WrapWebContentsCallback& callback) { + g_wrap_web_contents = callback; + + // Cleanup the wrapper on exit. + atom::AtomBrowserMainParts::Get()->RegisterDestructionCallback( + base::Bind(ClearWrapWebContents)); +} + } // namespace api } // namespace atom @@ -1145,7 +1149,6 @@ void Initialize(v8::Local exports, v8::Local unused, mate::Dictionary dict(isolate, exports); dict.SetMethod("create", &atom::api::WebContents::Create); dict.SetMethod("_setWrapWebContents", &atom::api::SetWrapWebContents); - dict.SetMethod("_clearWrapWebContents", &atom::api::ClearWrapWebContents); } } // namespace diff --git a/atom/browser/api/lib/app.coffee b/atom/browser/api/lib/app.coffee index cc676df0dd5a..ba68076177ae 100644 --- a/atom/browser/api/lib/app.coffee +++ b/atom/browser/api/lib/app.coffee @@ -59,10 +59,7 @@ app.on 'activate', (event, hasVisibleWindows) -> @emit 'activate-with-no-open-wi # Wrappers for native classes. sessionBindings._setWrapSession wrapSession -process.once 'exit', sessionBindings._clearWrapSession - downloadItemBindings._setWrapDownloadItem wrapDownloadItem -process.once 'exit', downloadItemBindings._clearWrapDownloadItem # Only one App object pemitted. module.exports = app diff --git a/atom/browser/api/lib/web-contents.coffee b/atom/browser/api/lib/web-contents.coffee index eab9f1967158..331a561189d7 100644 --- a/atom/browser/api/lib/web-contents.coffee +++ b/atom/browser/api/lib/web-contents.coffee @@ -112,7 +112,6 @@ wrapWebContents = (webContents) -> @_printToPDF printingSetting, callback binding._setWrapWebContents wrapWebContents -process.once 'exit', binding._clearWrapWebContents module.exports.create = (options={}) -> binding.create(options) From 84410a7e1c1a053220333e374b1445bd9d2259ff Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 4 Nov 2015 17:23:27 +0800 Subject: [PATCH 3/5] mac: Destroy the app delegate before running destruction callbacks Otherwise users might be able to access wrapper functions after they are destroyed. --- atom/browser/atom_browser_main_parts.cc | 4 ++++ atom/browser/atom_browser_main_parts.h | 5 ++++- atom/browser/atom_browser_main_parts_mac.mm | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index 5fae5bfdbedd..f6d1070e875d 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -128,6 +128,10 @@ void AtomBrowserMainParts::PostMainMessageLoopStart() { void AtomBrowserMainParts::PostMainMessageLoopRun() { brightray::BrowserMainParts::PostMainMessageLoopRun(); +#if defined(OS_MACOSX) + FreeAppDelegate(); +#endif + // Make sure destruction callbacks are called before message loop is // destroyed, otherwise some objects that need to be deleted on IO thread // won't be freed. diff --git a/atom/browser/atom_browser_main_parts.h b/atom/browser/atom_browser_main_parts.h index 65b142157dc1..360b1cba1169 100644 --- a/atom/browser/atom_browser_main_parts.h +++ b/atom/browser/atom_browser_main_parts.h @@ -46,7 +46,6 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts { void PostMainMessageLoopRun() override; #if defined(OS_MACOSX) void PreMainMessageLoopStart() override; - void PostDestroyThreads() override; #endif private: @@ -56,6 +55,10 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts { void HandleShutdownSignals(); #endif +#if defined(OS_MACOSX) + void FreeAppDelegate(); +#endif + // A fake BrowserProcess object that used to feed the source code from chrome. scoped_ptr fake_browser_process_; diff --git a/atom/browser/atom_browser_main_parts_mac.mm b/atom/browser/atom_browser_main_parts_mac.mm index 1de07dfc0e44..42e3100f490e 100644 --- a/atom/browser/atom_browser_main_parts_mac.mm +++ b/atom/browser/atom_browser_main_parts_mac.mm @@ -34,7 +34,7 @@ void AtomBrowserMainParts::PreMainMessageLoopStart() { setObject:@"NO" forKey:@"NSTreatUnknownArgumentsAsOpen"]; } -void AtomBrowserMainParts::PostDestroyThreads() { +void AtomBrowserMainParts::FreeAppDelegate() { [[NSApp delegate] release]; [NSApp setDelegate:nil]; } From 07c55f321fd5baf8dc5c8646d171f6de90d7f165 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 4 Nov 2015 17:31:10 +0800 Subject: [PATCH 4/5] Destroy JS env immediately after running destruction callbacks --- atom/browser/atom_browser_main_parts.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index f6d1070e875d..041c9bfc8fbe 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -137,6 +137,14 @@ void AtomBrowserMainParts::PostMainMessageLoopRun() { // won't be freed. for (const auto& callback : destruction_callbacks_) callback.Run(); + + // Destroy JavaScript environment immediately after running destruction + // callbacks. + gc_timer_.Stop(); + node_debugger_.reset(); + atom_bindings_.reset(); + node_bindings_.reset(); + js_env_.reset(); } } // namespace atom From d56b34de3b9e3065f1b4aa8facdd78a840bb1acc Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 4 Nov 2015 18:21:03 +0800 Subject: [PATCH 5/5] Make sure all native resources get freed on exit --- atom/browser/api/atom_api_global_shortcut.cc | 3 +++ atom/browser/api/atom_api_global_shortcut.h | 9 ++++++--- atom/browser/api/atom_api_menu.cc | 8 ++++++++ atom/browser/api/atom_api_menu.h | 10 +++++++--- atom/browser/api/atom_api_menu_mac.h | 1 + atom/browser/api/atom_api_menu_mac.mm | 5 +++++ atom/browser/api/atom_api_power_monitor.cc | 3 +++ atom/browser/api/atom_api_power_monitor.h | 9 ++++++--- atom/browser/api/atom_api_power_save_blocker.cc | 5 +++++ atom/browser/api/atom_api_power_save_blocker.h | 10 ++++++---- atom/browser/api/atom_api_tray.h | 10 ++++++---- 11 files changed, 56 insertions(+), 17 deletions(-) diff --git a/atom/browser/api/atom_api_global_shortcut.cc b/atom/browser/api/atom_api_global_shortcut.cc index f5a03e4abf90..6ab4fa4b6186 100644 --- a/atom/browser/api/atom_api_global_shortcut.cc +++ b/atom/browser/api/atom_api_global_shortcut.cc @@ -23,6 +23,9 @@ GlobalShortcut::GlobalShortcut() { } GlobalShortcut::~GlobalShortcut() { +} + +void GlobalShortcut::Destroy() { UnregisterAll(); } diff --git a/atom/browser/api/atom_api_global_shortcut.h b/atom/browser/api/atom_api_global_shortcut.h index 15cd3d4e0edb..93eb7853ae0d 100644 --- a/atom/browser/api/atom_api_global_shortcut.h +++ b/atom/browser/api/atom_api_global_shortcut.h @@ -8,9 +8,9 @@ #include #include +#include "atom/browser/api/trackable_object.h" #include "base/callback.h" #include "chrome/browser/extensions/global_shortcut_listener.h" -#include "native_mate/wrappable.h" #include "native_mate/handle.h" #include "ui/base/accelerators/accelerator.h" @@ -19,13 +19,16 @@ namespace atom { namespace api { class GlobalShortcut : public extensions::GlobalShortcutListener::Observer, - public mate::Wrappable { + public mate::TrackableObject { public: static mate::Handle Create(v8::Isolate* isolate); protected: GlobalShortcut(); - virtual ~GlobalShortcut(); + ~GlobalShortcut() override; + + // mate::TrackableObject: + void Destroy() override; // mate::Wrappable implementations: mate::ObjectTemplateBuilder GetObjectTemplateBuilder( diff --git a/atom/browser/api/atom_api_menu.cc b/atom/browser/api/atom_api_menu.cc index 9bd724a9612e..1f16a428da01 100644 --- a/atom/browser/api/atom_api_menu.cc +++ b/atom/browser/api/atom_api_menu.cc @@ -27,6 +27,14 @@ Menu::Menu() Menu::~Menu() { } +void Menu::Destroy() { + model_.reset(); +} + +bool Menu::IsDestroyed() const { + return !model_; +} + void Menu::AfterInit(v8::Isolate* isolate) { mate::Dictionary wrappable(isolate, GetWrapper(isolate)); mate::Dictionary delegate; diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index 0d93c0d46be6..545dd18e386c 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -8,16 +8,16 @@ #include #include "atom/browser/api/atom_api_window.h" +#include "atom/browser/api/trackable_object.h" #include "atom/browser/ui/atom_menu_model.h" #include "base/callback.h" #include "base/memory/scoped_ptr.h" -#include "native_mate/wrappable.h" namespace atom { namespace api { -class Menu : public mate::Wrappable, +class Menu : public mate::TrackableObject, public AtomMenuModel::Delegate { public: static mate::Wrappable* Create(); @@ -37,9 +37,13 @@ class Menu : public mate::Wrappable, protected: Menu(); - virtual ~Menu(); + ~Menu() override; + + // mate::TrackableObject: + void Destroy() override; // mate::Wrappable: + bool IsDestroyed() const override; void AfterInit(v8::Isolate* isolate) override; // ui::SimpleMenuModel::Delegate: diff --git a/atom/browser/api/atom_api_menu_mac.h b/atom/browser/api/atom_api_menu_mac.h index 5a086776a639..baa2aff349e1 100644 --- a/atom/browser/api/atom_api_menu_mac.h +++ b/atom/browser/api/atom_api_menu_mac.h @@ -19,6 +19,7 @@ class MenuMac : public Menu { protected: MenuMac(); + void Destroy() override; void Popup(Window* window) override; void PopupAt(Window* window, int x, int y) override; diff --git a/atom/browser/api/atom_api_menu_mac.mm b/atom/browser/api/atom_api_menu_mac.mm index 071753218d6a..5936e0439f4f 100644 --- a/atom/browser/api/atom_api_menu_mac.mm +++ b/atom/browser/api/atom_api_menu_mac.mm @@ -18,6 +18,11 @@ namespace api { MenuMac::MenuMac() { } +void MenuMac::Destroy() { + menu_controller_.reset(); + Menu::Destroy(); +} + void MenuMac::Popup(Window* window) { NativeWindow* native_window = window->window(); if (!native_window) diff --git a/atom/browser/api/atom_api_power_monitor.cc b/atom/browser/api/atom_api_power_monitor.cc index 31b35e10cea8..eeb9475c2847 100644 --- a/atom/browser/api/atom_api_power_monitor.cc +++ b/atom/browser/api/atom_api_power_monitor.cc @@ -19,6 +19,9 @@ PowerMonitor::PowerMonitor() { } PowerMonitor::~PowerMonitor() { +} + +void PowerMonitor::Destroy() { base::PowerMonitor::Get()->RemoveObserver(this); } diff --git a/atom/browser/api/atom_api_power_monitor.h b/atom/browser/api/atom_api_power_monitor.h index dcf0906b437f..9303b3ab288f 100644 --- a/atom/browser/api/atom_api_power_monitor.h +++ b/atom/browser/api/atom_api_power_monitor.h @@ -5,7 +5,7 @@ #ifndef ATOM_BROWSER_API_ATOM_API_POWER_MONITOR_H_ #define ATOM_BROWSER_API_ATOM_API_POWER_MONITOR_H_ -#include "atom/browser/api/event_emitter.h" +#include "atom/browser/api/trackable_object.h" #include "base/compiler_specific.h" #include "base/power_monitor/power_observer.h" #include "native_mate/handle.h" @@ -14,14 +14,17 @@ namespace atom { namespace api { -class PowerMonitor : public mate::EventEmitter, +class PowerMonitor : public mate::TrackableObject, public base::PowerObserver { public: static v8::Local Create(v8::Isolate* isolate); protected: PowerMonitor(); - virtual ~PowerMonitor(); + ~PowerMonitor() override; + + // mate::TrackableObject: + void Destroy() override; // base::PowerObserver implementations: void OnPowerStateChange(bool on_battery_power) override; diff --git a/atom/browser/api/atom_api_power_save_blocker.cc b/atom/browser/api/atom_api_power_save_blocker.cc index 58983e6c846a..f77979ae417f 100644 --- a/atom/browser/api/atom_api_power_save_blocker.cc +++ b/atom/browser/api/atom_api_power_save_blocker.cc @@ -45,6 +45,11 @@ PowerSaveBlocker::PowerSaveBlocker() PowerSaveBlocker::~PowerSaveBlocker() { } +void PowerSaveBlocker::Destroy() { + power_save_blocker_types_.clear(); + power_save_blocker_.reset(); +} + void PowerSaveBlocker::UpdatePowerSaveBlocker() { if (power_save_blocker_types_.empty()) { power_save_blocker_.reset(); diff --git a/atom/browser/api/atom_api_power_save_blocker.h b/atom/browser/api/atom_api_power_save_blocker.h index 9861f2b0f7cd..e7ce97878ffb 100644 --- a/atom/browser/api/atom_api_power_save_blocker.h +++ b/atom/browser/api/atom_api_power_save_blocker.h @@ -7,10 +7,10 @@ #include +#include "atom/browser/api/trackable_object.h" #include "base/memory/scoped_ptr.h" #include "content/public/browser/power_save_blocker.h" #include "native_mate/handle.h" -#include "native_mate/wrappable.h" namespace mate { class Dictionary; @@ -20,13 +20,16 @@ namespace atom { namespace api { -class PowerSaveBlocker : public mate::Wrappable { +class PowerSaveBlocker : public mate::TrackableObject { public: static mate::Handle Create(v8::Isolate* isolate); protected: PowerSaveBlocker(); - virtual ~PowerSaveBlocker(); + ~PowerSaveBlocker() override; + + // mate::TrackableObject: + void Destroy() override; // mate::Wrappable implementations: mate::ObjectTemplateBuilder GetObjectTemplateBuilder( @@ -48,7 +51,6 @@ class PowerSaveBlocker : public mate::Wrappable { std::map; PowerSaveBlockerTypeMap power_save_blocker_types_; - DISALLOW_COPY_AND_ASSIGN(PowerSaveBlocker); }; diff --git a/atom/browser/api/atom_api_tray.h b/atom/browser/api/atom_api_tray.h index dc9302597cf3..9a423f61764f 100644 --- a/atom/browser/api/atom_api_tray.h +++ b/atom/browser/api/atom_api_tray.h @@ -8,7 +8,7 @@ #include #include -#include "atom/browser/api/event_emitter.h" +#include "atom/browser/api/trackable_object.h" #include "atom/browser/ui/tray_icon_observer.h" #include "base/memory/scoped_ptr.h" @@ -29,7 +29,7 @@ namespace api { class Menu; -class Tray : public mate::EventEmitter, +class Tray : public mate::TrackableObject, public TrayIconObserver { public: static mate::Wrappable* New(v8::Isolate* isolate, const gfx::Image& image); @@ -39,7 +39,7 @@ class Tray : public mate::EventEmitter, protected: explicit Tray(const gfx::Image& image); - virtual ~Tray(); + ~Tray() override; // TrayIconObserver: void OnClicked(const gfx::Rect& bounds, int modifiers) override; @@ -53,7 +53,9 @@ class Tray : public mate::EventEmitter, // mate::Wrappable: bool IsDestroyed() const override; - void Destroy(); + // mate::TrackableObject: + void Destroy() override; + void SetImage(mate::Arguments* args, const gfx::Image& image); void SetPressedImage(mate::Arguments* args, const gfx::Image& image); void SetToolTip(mate::Arguments* args, const std::string& tool_tip);