From 4625f051c84eee8879424d082601333910465e75 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 5 Dec 2013 22:12:27 +0800 Subject: [PATCH 1/9] Simplify V8 operations in renderer ipc code. --- renderer/api/atom_api_renderer_ipc.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/renderer/api/atom_api_renderer_ipc.cc b/renderer/api/atom_api_renderer_ipc.cc index d12634797226..ddf16a5b50e7 100644 --- a/renderer/api/atom_api_renderer_ipc.cc +++ b/renderer/api/atom_api_renderer_ipc.cc @@ -45,10 +45,10 @@ RenderView* GetCurrentRenderView() { v8::Handle RendererIPC::Send(const v8::Arguments &args) { v8::HandleScope scope; - if (!args[0]->IsString()) + string16 channel; + if (!FromV8Arguments(args, &channel)) return node::ThrowTypeError("Bad argument"); - string16 channel = FromV8Value(args[0]); RenderView* render_view = GetCurrentRenderView(); // Convert Arguments to Array, so we can use V8ValueConverter to convert it @@ -78,12 +78,10 @@ v8::Handle RendererIPC::Send(const v8::Arguments &args) { v8::Handle RendererIPC::SendSync(const v8::Arguments &args) { v8::HandleScope scope; - if (!args[0]->IsString()) + string16 channel; + if (!FromV8Arguments(args, &channel)) return node::ThrowTypeError("Bad argument"); - v8::Handle context = v8::Context::GetCurrent(); - string16 channel = FromV8Value(args[0]); - // Convert Arguments to Array, so we can use V8ValueConverter to convert it // to ListValue. v8::Local v8_args = v8::Array::New(args.Length() - 1); @@ -91,7 +89,8 @@ v8::Handle RendererIPC::SendSync(const v8::Arguments &args) { v8_args->Set(i, args[i + 1]); scoped_ptr converter(V8ValueConverter::create()); - scoped_ptr arguments(converter->FromV8Value(v8_args, context)); + scoped_ptr arguments( + converter->FromV8Value(v8_args, v8::Context::GetCurrent())); DCHECK(arguments && arguments->IsType(base::Value::TYPE_LIST)); From 287c948845b24e5dd64fd25750d34c898ef664bc Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 5 Dec 2013 23:25:14 +0800 Subject: [PATCH 2/9] Enable getting scoped_ptr type from v8 value. God damned C++ template. --- atom.gyp | 1 + common/swap_or_assign.h | 41 +++++++++++++++++++++++++++++++++++++++++ common/v8_conversions.h | 20 +++++++++++++++++++- 3 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 common/swap_or_assign.h diff --git a/atom.gyp b/atom.gyp index bde0baa42cd6..e52121e6e5c5 100644 --- a/atom.gyp +++ b/atom.gyp @@ -164,6 +164,7 @@ 'common/platform_util.h', 'common/platform_util_mac.mm', 'common/platform_util_win.cc', + 'common/swap_or_assign.h', 'common/v8_conversions.h', 'common/v8_value_converter_impl.cc', 'common/v8_value_converter_impl.h', diff --git a/common/swap_or_assign.h b/common/swap_or_assign.h new file mode 100644 index 000000000000..3953653f8f57 --- /dev/null +++ b/common/swap_or_assign.h @@ -0,0 +1,41 @@ +// Copyright (c) 2013 GitHub, Inc. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef ATOM_COMMON_SWAP_OR_ASSIGN_H_ +#define ATOM_COMMON_SWAP_OR_ASSIGN_H_ + +namespace internal { + +// Helper to detect whether value has specified method. +template +class HasSwapMethod { + typedef char one; + typedef long two; + template static one test(char[sizeof(&C::swap)]) ; + template static two test(...); + public: + enum { value = sizeof(test(0)) == sizeof(char) }; +}; + +template +struct enable_if {}; + +template +struct enable_if { typedef T type; }; + +template +typename enable_if::value>::type SwapOrAssign( + T& v1, const T& v2) { + v1.swap(const_cast(v2)); +} + +template +typename enable_if::value>::type SwapOrAssign( + T& v1, const T& v2) { + v1 = v2; +} + +} // namespace internal + +#endif // ATOM_COMMON_SWAP_OR_ASSIGN_H_ diff --git a/common/v8_conversions.h b/common/v8_conversions.h index 55b770996a7a..a1800ce25b8c 100644 --- a/common/v8_conversions.h +++ b/common/v8_conversions.h @@ -11,7 +11,11 @@ #include "base/files/file_path.h" #include "base/string16.h" +#include "base/template_util.h" +#include "base/values.h" #include "browser/api/atom_api_window.h" +#include "common/swap_or_assign.h" +#include "content/public/renderer/v8_value_converter.h" #include "googleurl/src/gurl.h" #include "ui/gfx/rect.h" #include "v8/include/v8.h" @@ -60,6 +64,13 @@ struct FromV8Value { width->IntegerValue(), height->IntegerValue()); } + operator scoped_ptr() { + scoped_ptr converter( + content::V8ValueConverter::create()); + return scoped_ptr( + converter->FromV8Value(value_, v8::Context::GetCurrent())); + } + operator std::vector() { std::vector array; v8::Handle v8_array = v8::Handle::Cast(value_); @@ -184,6 +195,12 @@ bool V8ValueCanBeConvertedTo(v8::Handle value) { return value->IsObject(); } +template<> inline +bool V8ValueCanBeConvertedTo>( + v8::Handle value) { + return value->IsObject(); +} + template<> inline bool V8ValueCanBeConvertedTo>( v8::Handle value) { @@ -218,7 +235,8 @@ template inline bool FromV8Arguments(const v8::Arguments& args, T1* value, int index = 0) { if (!V8ValueCanBeConvertedTo(args[index])) return false; - *value = static_cast(FromV8Value(args[index])); + internal::SwapOrAssign(*value, + static_cast(FromV8Value(args[index]))); return true; } From e5afa72b4dd4f5945c8efd26d29999fb3c04bde0 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 5 Dec 2013 23:34:43 +0800 Subject: [PATCH 3/9] Fail quietly when getting null renderer view. It happens when the window is closing. --- renderer/api/atom_api_renderer_ipc.cc | 41 ++++++--------------------- renderer/api/lib/ipc.coffee | 12 ++++---- 2 files changed, 14 insertions(+), 39 deletions(-) diff --git a/renderer/api/atom_api_renderer_ipc.cc b/renderer/api/atom_api_renderer_ipc.cc index ddf16a5b50e7..f5545e03169f 100644 --- a/renderer/api/atom_api_renderer_ipc.cc +++ b/renderer/api/atom_api_renderer_ipc.cc @@ -4,11 +4,9 @@ #include "renderer/api/atom_api_renderer_ipc.h" -#include "base/values.h" #include "common/api/api_messages.h" #include "common/v8_conversions.h" #include "content/public/renderer/render_view.h" -#include "content/public/renderer/v8_value_converter.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebView.h" #include "vendor/node/src/node.h" @@ -26,7 +24,6 @@ namespace { RenderView* GetCurrentRenderView() { WebFrame* frame = WebFrame::frameForCurrentContext(); - DCHECK(frame); if (!frame) return NULL; @@ -34,9 +31,7 @@ RenderView* GetCurrentRenderView() { if (!view) return NULL; // can happen during closing. - RenderView* render_view = RenderView::FromWebView(view); - DCHECK(render_view); - return render_view; + return RenderView::FromWebView(view); } } // namespace @@ -46,22 +41,13 @@ v8::Handle RendererIPC::Send(const v8::Arguments &args) { v8::HandleScope scope; string16 channel; - if (!FromV8Arguments(args, &channel)) + scoped_ptr arguments; + if (!FromV8Arguments(args, &channel, &arguments)) return node::ThrowTypeError("Bad argument"); RenderView* render_view = GetCurrentRenderView(); - - // Convert Arguments to Array, so we can use V8ValueConverter to convert it - // to ListValue. - v8::Local v8_args = v8::Array::New(args.Length() - 1); - for (int i = 0; i < args.Length() - 1; ++i) - v8_args->Set(i, args[i + 1]); - - scoped_ptr converter(V8ValueConverter::create()); - scoped_ptr arguments( - converter->FromV8Value(v8_args, v8::Context::GetCurrent())); - - DCHECK(arguments && arguments->IsType(base::Value::TYPE_LIST)); + if (render_view == NULL) + return v8::Undefined(); bool success = render_view->Send(new AtomViewHostMsg_Message( render_view->GetRoutingID(), @@ -79,22 +65,13 @@ v8::Handle RendererIPC::SendSync(const v8::Arguments &args) { v8::HandleScope scope; string16 channel; - if (!FromV8Arguments(args, &channel)) + scoped_ptr arguments; + if (!FromV8Arguments(args, &channel, &arguments)) return node::ThrowTypeError("Bad argument"); - // Convert Arguments to Array, so we can use V8ValueConverter to convert it - // to ListValue. - v8::Local v8_args = v8::Array::New(args.Length() - 1); - for (int i = 0; i < args.Length() - 1; ++i) - v8_args->Set(i, args[i + 1]); - - scoped_ptr converter(V8ValueConverter::create()); - scoped_ptr arguments( - converter->FromV8Value(v8_args, v8::Context::GetCurrent())); - - DCHECK(arguments && arguments->IsType(base::Value::TYPE_LIST)); - RenderView* render_view = GetCurrentRenderView(); + if (render_view == NULL) + return v8::Undefined(); string16 json; IPC::SyncMessage* message = new AtomViewHostMsg_Message_Sync( diff --git a/renderer/api/lib/ipc.coffee b/renderer/api/lib/ipc.coffee index c54545d8a2a1..545e6a2168a8 100644 --- a/renderer/api/lib/ipc.coffee +++ b/renderer/api/lib/ipc.coffee @@ -4,23 +4,21 @@ ipc = process.atomBinding('ipc') class Ipc extends EventEmitter constructor: -> process.on 'ATOM_INTERNAL_MESSAGE', (args...) => - @emit(args...) + @emit args... window.addEventListener 'unload', (event) -> process.removeAllListeners 'ATOM_INTERNAL_MESSAGE' send: (args...) -> - ipc.send('ATOM_INTERNAL_MESSAGE', 'message', args...) + @sendChannel 'message', args... sendChannel: (args...) -> - ipc.send('ATOM_INTERNAL_MESSAGE', args...) + ipc.send 'ATOM_INTERNAL_MESSAGE', [args...] sendSync: (args...) -> - msg = ipc.sendSync('ATOM_INTERNAL_MESSAGE_SYNC', 'sync-message', args...) - JSON.parse(msg) + @sendSync 'sync-message', args... sendChannelSync: (args...) -> - msg = ipc.sendSync('ATOM_INTERNAL_MESSAGE_SYNC', args...) - JSON.parse(msg) + JSON.parse ipc.sendSync('ATOM_INTERNAL_MESSAGE_SYNC', [args...]) module.exports = new Ipc From 4a1ee39156f1eabdd9da8bdc8e38d63fe914b2e6 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 5 Dec 2013 23:47:07 +0800 Subject: [PATCH 4/9] Get rid of manually converting to base::Value when possible. --- browser/api/atom_api_browser_ipc.cc | 19 ++++--------------- browser/api/atom_api_event_emitter.cc | 6 ++---- browser/api/atom_api_window.cc | 15 ++++----------- browser/api/atom_browser_bindings.cc | 2 -- browser/api/lib/ipc.coffee | 2 +- common/v8_conversions.h | 3 ++- renderer/api/atom_api_renderer_ipc.cc | 1 - renderer/api/atom_renderer_bindings.cc | 2 -- 8 files changed, 13 insertions(+), 37 deletions(-) diff --git a/browser/api/atom_api_browser_ipc.cc b/browser/api/atom_api_browser_ipc.cc index 7e43e1edb667..359743acfd86 100644 --- a/browser/api/atom_api_browser_ipc.cc +++ b/browser/api/atom_api_browser_ipc.cc @@ -4,10 +4,8 @@ #include "browser/api/atom_api_browser_ipc.h" -#include "base/values.h" #include "common/api/api_messages.h" #include "common/v8_conversions.h" -#include "common/v8_value_converter_impl.h" #include "content/public/browser/render_view_host.h" #include "vendor/node/src/node.h" #include "vendor/node/src/node_internals.h" @@ -25,26 +23,17 @@ v8::Handle BrowserIPC::Send(const v8::Arguments &args) { string16 channel; int process_id, routing_id; - if (!FromV8Arguments(args, &channel, &process_id, &routing_id)) + scoped_ptr arguments; + if (!FromV8Arguments(args, &channel, &process_id, &routing_id, &arguments)) return node::ThrowTypeError("Bad argument"); + DCHECK(arguments && arguments->IsType(base::Value::TYPE_LIST)); + RenderViewHost* render_view_host(RenderViewHost::FromID( process_id, routing_id)); if (!render_view_host) return node::ThrowError("Invalid render view host"); - // Convert Arguments to Array, so we can use V8ValueConverter to convert it - // to ListValue. - v8::Local v8_args = v8::Array::New(args.Length() - 3); - for (int i = 0; i < args.Length() - 3; ++i) - v8_args->Set(i, args[i + 3]); - - scoped_ptr converter(new V8ValueConverterImpl()); - scoped_ptr arguments( - converter->FromV8Value(v8_args, v8::Context::GetCurrent())); - - DCHECK(arguments && arguments->IsType(base::Value::TYPE_LIST)); - render_view_host->Send(new AtomViewMsg_Message( routing_id, channel, diff --git a/browser/api/atom_api_event_emitter.cc b/browser/api/atom_api_event_emitter.cc index 48f27cdd2094..3196546adf1e 100644 --- a/browser/api/atom_api_event_emitter.cc +++ b/browser/api/atom_api_event_emitter.cc @@ -7,10 +7,8 @@ #include #include "base/logging.h" -#include "base/memory/scoped_ptr.h" -#include "base/values.h" #include "browser/api/atom_api_event.h" -#include "common/v8_value_converter_impl.h" +#include "common/v8_conversions.h" #include "vendor/node/src/node.h" #include "vendor/node/src/node_internals.h" @@ -49,7 +47,7 @@ bool EventEmitter::Emit(const std::string& name, base::ListValue* args) { // Generate arguments for calling handle.emit. std::vector> v8_args; v8_args.reserve(args->GetSize() + 2); - v8_args.push_back(v8::String::New(name.c_str(), name.size())); + v8_args.push_back(ToV8Value(name)); v8_args.push_back(v8_event); for (size_t i = 0; i < args->GetSize(); i++) { base::Value* value = NULL; diff --git a/browser/api/atom_api_window.cc b/browser/api/atom_api_window.cc index 27cf5706bbbc..bdd4e43edf24 100644 --- a/browser/api/atom_api_window.cc +++ b/browser/api/atom_api_window.cc @@ -5,19 +5,15 @@ #include "browser/api/atom_api_window.h" #include "base/process_util.h" -#include "base/values.h" #include "browser/native_window.h" #include "common/v8_conversions.h" -#include "common/v8_value_converter_impl.h" #include "content/public/browser/navigation_entry.h" #include "content/public/browser/web_contents.h" #include "content/public/browser/render_process_host.h" #include "ui/gfx/point.h" -#include "ui/gfx/rect.h" #include "ui/gfx/size.h" #include "vendor/node/src/node_buffer.h" -using content::V8ValueConverter; using content::NavigationController; using node::ObjectWrap; @@ -105,15 +101,12 @@ v8::Handle Window::New(const v8::Arguments &args) { if (!args.IsConstructCall()) return node::ThrowError("Require constructor call"); - if (!args[0]->IsObject()) - return node::ThrowTypeError("Need options creating Window"); - - scoped_ptr converter(new V8ValueConverterImpl()); - scoped_ptr options( - converter->FromV8Value(args[0], v8::Context::GetCurrent())); + scoped_ptr options; + if (!FromV8Arguments(args, &options)) + return node::ThrowTypeError("Bad argument"); if (!options || !options->IsType(base::Value::TYPE_DICTIONARY)) - return node::ThrowTypeError("Invalid options"); + return node::ThrowTypeError("Options must be dictionary"); new Window(args.This(), static_cast(options.get())); diff --git a/browser/api/atom_browser_bindings.cc b/browser/api/atom_browser_bindings.cc index c2dc9e78cc03..3fb174573161 100644 --- a/browser/api/atom_browser_bindings.cc +++ b/browser/api/atom_browser_bindings.cc @@ -7,10 +7,8 @@ #include #include "base/logging.h" -#include "base/values.h" #include "browser/api/atom_api_event.h" #include "common/v8_conversions.h" -#include "common/v8_value_converter_impl.h" #include "content/public/browser/browser_thread.h" #include "vendor/node/src/node.h" #include "vendor/node/src/node_internals.h" diff --git a/browser/api/lib/ipc.coffee b/browser/api/lib/ipc.coffee index 970709ebc90d..11377ec28300 100644 --- a/browser/api/lib/ipc.coffee +++ b/browser/api/lib/ipc.coffee @@ -8,7 +8,7 @@ sendWrap = (channel, processId, routingId, args...) -> processId = window.getProcessId() routingId = window.getRoutingId() - send channel, processId, routingId, args... + send channel, processId, routingId, [args...] class Ipc extends EventEmitter constructor: -> diff --git a/common/v8_conversions.h b/common/v8_conversions.h index a1800ce25b8c..90ae2fc79d2b 100644 --- a/common/v8_conversions.h +++ b/common/v8_conversions.h @@ -15,6 +15,7 @@ #include "base/values.h" #include "browser/api/atom_api_window.h" #include "common/swap_or_assign.h" +#include "common/v8_value_converter_impl.h" #include "content/public/renderer/v8_value_converter.h" #include "googleurl/src/gurl.h" #include "ui/gfx/rect.h" @@ -66,7 +67,7 @@ struct FromV8Value { operator scoped_ptr() { scoped_ptr converter( - content::V8ValueConverter::create()); + new atom::V8ValueConverterImpl); return scoped_ptr( converter->FromV8Value(value_, v8::Context::GetCurrent())); } diff --git a/renderer/api/atom_api_renderer_ipc.cc b/renderer/api/atom_api_renderer_ipc.cc index f5545e03169f..3600a3923153 100644 --- a/renderer/api/atom_api_renderer_ipc.cc +++ b/renderer/api/atom_api_renderer_ipc.cc @@ -12,7 +12,6 @@ #include "vendor/node/src/node.h" using content::RenderView; -using content::V8ValueConverter; using WebKit::WebFrame; using WebKit::WebView; diff --git a/renderer/api/atom_renderer_bindings.cc b/renderer/api/atom_renderer_bindings.cc index 6609d04eb665..4f54e6548c91 100644 --- a/renderer/api/atom_renderer_bindings.cc +++ b/renderer/api/atom_renderer_bindings.cc @@ -7,10 +7,8 @@ #include #include "base/logging.h" -#include "base/values.h" #include "common/v8_conversions.h" #include "content/public/renderer/render_view.h" -#include "content/public/renderer/v8_value_converter.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebView.h" #include "vendor/node/src/node.h" From e9e90b481a3395bafd99de278346348887a4fd78 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 5 Dec 2013 23:54:57 +0800 Subject: [PATCH 5/9] :lipstick: Fix cpplint warnings. --- script/cpplint.py | 1 + 1 file changed, 1 insertion(+) diff --git a/script/cpplint.py b/script/cpplint.py index 0862f4f80c47..c3d97bc46988 100755 --- a/script/cpplint.py +++ b/script/cpplint.py @@ -16,6 +16,7 @@ IGNORE_FILES = [ 'common/api/api_messages.cc', 'common/api/api_messages.h', 'common/atom_version.h', + 'common/swap_or_assign.h', ] SOURCE_ROOT = os.path.dirname(os.path.dirname(__file__)) From 623e0f3ae4112e40b309a1e8c5f886a0b05e99f1 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 6 Dec 2013 14:44:25 +0800 Subject: [PATCH 6/9] Release render view's remote objects when it's deleted. Privously we release them when the window is unloaded, which is not correct since a render view can have multiple windows (or js contexts) and when the unload event is emitted the render view could already have gone. This PR does the cleaning work purely in browser, so here is no need to worry about renderer's life time. --- browser/api/atom_api_window.cc | 4 ++++ browser/api/atom_api_window.h | 1 + browser/api/lib/browser-window.coffee | 5 +++++ browser/atom/rpc-server.coffee | 9 +++++---- browser/native_window.cc | 9 +++++++++ browser/native_window.h | 1 + browser/native_window_observer.h | 3 +++ renderer/api/lib/remote.coffee | 7 ------- 8 files changed, 28 insertions(+), 11 deletions(-) diff --git a/browser/api/atom_api_window.cc b/browser/api/atom_api_window.cc index bdd4e43edf24..1002aa81bee1 100644 --- a/browser/api/atom_api_window.cc +++ b/browser/api/atom_api_window.cc @@ -76,6 +76,10 @@ void Window::OnRendererResponsive() { Emit("responsive"); } +void Window::OnRenderViewDeleted() { + Emit("render-view-deleted"); +} + void Window::OnRendererCrashed() { Emit("crashed"); } diff --git a/browser/api/atom_api_window.h b/browser/api/atom_api_window.h index 03709dc22a10..334e8f554260 100644 --- a/browser/api/atom_api_window.h +++ b/browser/api/atom_api_window.h @@ -43,6 +43,7 @@ class Window : public EventEmitter, virtual void OnWindowBlur() OVERRIDE; virtual void OnRendererUnresponsive() OVERRIDE; virtual void OnRendererResponsive() OVERRIDE; + virtual void OnRenderViewDeleted() OVERRIDE; virtual void OnRendererCrashed() OVERRIDE; private: diff --git a/browser/api/lib/browser-window.coffee b/browser/api/lib/browser-window.coffee index 3da99104b91b..b95c2e7b73e8 100644 --- a/browser/api/lib/browser-window.coffee +++ b/browser/api/lib/browser-window.coffee @@ -11,6 +11,11 @@ BrowserWindow::_init = -> menu = app.getApplicationMenu() @setMenu menu if menu? + # Tell the rpc server that a render view has been deleted and we need to + # release all objects owned by it. + @on 'render-view-deleted', -> + process.emit 'ATOM_BROWSER_RELEASE_RENDER_VIEW', @getProcessId(), @getRoutingId() + BrowserWindow::toggleDevTools = -> if @isDevToolsOpened() then @closeDevTools() else @openDevTools() diff --git a/browser/atom/rpc-server.coffee b/browser/atom/rpc-server.coffee index 6b56bfca2063..479a85c3fca0 100644 --- a/browser/atom/rpc-server.coffee +++ b/browser/atom/rpc-server.coffee @@ -78,6 +78,11 @@ callFunction = (event, processId, routingId, func, caller, args) -> ret = func.apply caller, args event.returnValue = valueToMeta processId, routingId, ret +# Send by BrowserWindow when its render view is deleted. +process.on 'ATOM_BROWSER_RELEASE_RENDER_VIEW', (processId, routingId) -> + console.log 'ATOM_BROWSER_RELEASE_RENDER_VIEW', processId, routingId + objectsRegistry.clear processId, routingId + ipc.on 'ATOM_BROWSER_REQUIRE', (event, processId, routingId, module) -> try event.returnValue = valueToMeta processId, routingId, require(module) @@ -90,10 +95,6 @@ ipc.on 'ATOM_BROWSER_GLOBAL', (event, processId, routingId, name) -> catch e event.returnValue = errorToMeta e -ipc.on 'ATOM_BROWSER_RELEASE_RENDER_VIEW', (event, processId, routingId) -> - objectsRegistry.clear processId, routingId - event.returnValue = null - ipc.on 'ATOM_BROWSER_CURRENT_WINDOW', (event, processId, routingId) -> try BrowserWindow = require 'browser-window' diff --git a/browser/native_window.cc b/browser/native_window.cc index 16e0891bc8d3..7a2353e574de 100644 --- a/browser/native_window.cc +++ b/browser/native_window.cc @@ -268,6 +268,11 @@ void NativeWindow::NotifyWindowClosed() { if (is_closed_) return; + // The OnRenderViewDeleted is not called when the WebContents is destroyed + // directly (e.g. when closing the window), so we make sure it's always + // emitted to users by sending it before window is closed.. + FOR_EACH_OBSERVER(NativeWindowObserver, observers_, OnRenderViewDeleted()); + is_closed_ = true; FOR_EACH_OBSERVER(NativeWindowObserver, observers_, OnWindowClosed()); @@ -393,6 +398,10 @@ bool NativeWindow::OnMessageReceived(const IPC::Message& message) { return handled; } +void NativeWindow::RenderViewDeleted(content::RenderViewHost*) { + FOR_EACH_OBSERVER(NativeWindowObserver, observers_, OnRenderViewDeleted()); +} + void NativeWindow::RenderViewGone(base::TerminationStatus status) { FOR_EACH_OBSERVER(NativeWindowObserver, observers_, OnRendererCrashed()); } diff --git a/browser/native_window.h b/browser/native_window.h index c7d1169cdc3d..03103f118259 100644 --- a/browser/native_window.h +++ b/browser/native_window.h @@ -182,6 +182,7 @@ class NativeWindow : public brightray::DefaultWebContentsDelegate, virtual void RendererResponsive(content::WebContents* source) OVERRIDE; // Implementations of content::WebContentsObserver. + virtual void RenderViewDeleted(content::RenderViewHost*) OVERRIDE; virtual void RenderViewGone(base::TerminationStatus status) OVERRIDE; virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE; diff --git a/browser/native_window_observer.h b/browser/native_window_observer.h index 5d06f983bd55..20b8ddcc17f0 100644 --- a/browser/native_window_observer.h +++ b/browser/native_window_observer.h @@ -35,6 +35,9 @@ class NativeWindowObserver { // Called when renderer recovers. virtual void OnRendererResponsive() {} + // Called when a render view has been deleted. + virtual void OnRenderViewDeleted() {} + // Called when renderer has crashed. virtual void OnRendererCrashed() {} }; diff --git a/renderer/api/lib/remote.coffee b/renderer/api/lib/remote.coffee index 7eaafc20aa52..07b06001038a 100644 --- a/renderer/api/lib/remote.coffee +++ b/renderer/api/lib/remote.coffee @@ -2,7 +2,6 @@ ipc = require 'ipc' CallbacksRegistry = require 'callbacks-registry' v8Util = process.atomBinding 'v8_util' -currentContextExist = true callbacksRegistry = new CallbacksRegistry # Convert the arguments object into an array of meta data. @@ -82,7 +81,6 @@ metaToValue = (meta) -> # Track delegate object's life time, and tell the browser to clean up # when the object is GCed. v8Util.setDestructor ret, -> - return unless currentContextExist ipc.sendChannel 'ATOM_BROWSER_DEREFERENCE', meta.storeId # Remember object's id. @@ -98,11 +96,6 @@ ipc.on 'ATOM_RENDERER_CALLBACK', (id, args) -> ipc.on 'ATOM_RENDERER_RELEASE_CALLBACK', (id) -> callbacksRegistry.remove id -# Release all resources of current render view when it's going to be unloaded. -window.addEventListener 'unload', (event) -> - currentContextExist = false - ipc.sendChannelSync 'ATOM_BROWSER_RELEASE_RENDER_VIEW' - # Get remote module. # (Just like node's require, the modules are cached permanently, note that this # is safe leak since the object is not expected to get freed in browser) From 0a63395b0f2a9c1c448d8069f5830d233f84b992 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 6 Dec 2013 14:54:29 +0800 Subject: [PATCH 7/9] :lipstick: Restore old settings in app specs. --- browser/atom/rpc-server.coffee | 1 - spec/api/app.coffee | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/browser/atom/rpc-server.coffee b/browser/atom/rpc-server.coffee index 479a85c3fca0..6e072b661864 100644 --- a/browser/atom/rpc-server.coffee +++ b/browser/atom/rpc-server.coffee @@ -80,7 +80,6 @@ callFunction = (event, processId, routingId, func, caller, args) -> # Send by BrowserWindow when its render view is deleted. process.on 'ATOM_BROWSER_RELEASE_RENDER_VIEW', (processId, routingId) -> - console.log 'ATOM_BROWSER_RELEASE_RENDER_VIEW', processId, routingId objectsRegistry.clear processId, routingId ipc.on 'ATOM_BROWSER_REQUIRE', (event, processId, routingId, module) -> diff --git a/spec/api/app.coffee b/spec/api/app.coffee index d2d912db701e..70577e4b7118 100644 --- a/spec/api/app.coffee +++ b/spec/api/app.coffee @@ -11,6 +11,7 @@ describe 'app module', -> assert.equal app.getVersion(), '0.1.0' app.setVersion 'test-version' assert.equal app.getVersion(), 'test-version' + app.setVersion '0.1.0' describe 'app.getName()', -> it 'returns the name field of package.json', -> @@ -21,3 +22,4 @@ describe 'app module', -> assert.equal app.getName(), 'atom-shell-default-app' app.setName 'test-name' assert.equal app.getName(), 'test-name' + app.setName 'atom-shell-default-app' From 844fccc177791dc206e247bacc14604e2a1a415c Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 6 Dec 2013 15:04:51 +0800 Subject: [PATCH 8/9] Use random number as id in CallbacksRegistry. It's very possible that the callbacks got GCed before the render view is closed (like page getting refreshed), so we should not let browser call the wrong callback, instead we should throw error whenever a callback is not found. --- common/api/lib/callbacks-registry.coffee | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/common/api/lib/callbacks-registry.coffee b/common/api/lib/callbacks-registry.coffee index 6a0120bf1d60..b549d17c7285 100644 --- a/common/api/lib/callbacks-registry.coffee +++ b/common/api/lib/callbacks-registry.coffee @@ -1,15 +1,19 @@ module.exports = class CallbacksRegistry constructor: -> - @nextId = 0 + @emptyFunc = -> throw new Error "Browser trying to call a non-exist callback + in renderer, this usually happens when renderer code forgot to release + a callback installed on objects in browser when renderer was going to be + unloaded or released." @callbacks = {} add: (callback) -> - @callbacks[++@nextId] = callback - @nextId + id = Math.random().toString() + @callbacks[id] = callback + id get: (id) -> - @callbacks[id] + @callbacks[id] ? -> call: (id, args...) -> @get(id).call global, args... From 085b1a45ee3412626b7bfb848f9bd9e51eae0d42 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 6 Dec 2013 15:53:40 +0800 Subject: [PATCH 9/9] Report the right render view that is deleted. --- browser/api/atom_api_window.cc | 7 +++++-- browser/api/atom_api_window.h | 2 +- browser/api/lib/browser-window.coffee | 4 ++-- browser/native_window.cc | 11 ++++++++--- browser/native_window_observer.h | 2 +- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/browser/api/atom_api_window.cc b/browser/api/atom_api_window.cc index 1002aa81bee1..ddc5d5cf07e1 100644 --- a/browser/api/atom_api_window.cc +++ b/browser/api/atom_api_window.cc @@ -76,8 +76,11 @@ void Window::OnRendererResponsive() { Emit("responsive"); } -void Window::OnRenderViewDeleted() { - Emit("render-view-deleted"); +void Window::OnRenderViewDeleted(int process_id, int routing_id) { + base::ListValue args; + args.AppendInteger(process_id); + args.AppendInteger(routing_id); + Emit("render-view-deleted", &args); } void Window::OnRendererCrashed() { diff --git a/browser/api/atom_api_window.h b/browser/api/atom_api_window.h index 334e8f554260..92c470203e7a 100644 --- a/browser/api/atom_api_window.h +++ b/browser/api/atom_api_window.h @@ -43,7 +43,7 @@ class Window : public EventEmitter, virtual void OnWindowBlur() OVERRIDE; virtual void OnRendererUnresponsive() OVERRIDE; virtual void OnRendererResponsive() OVERRIDE; - virtual void OnRenderViewDeleted() OVERRIDE; + virtual void OnRenderViewDeleted(int process_id, int routing_id) OVERRIDE; virtual void OnRendererCrashed() OVERRIDE; private: diff --git a/browser/api/lib/browser-window.coffee b/browser/api/lib/browser-window.coffee index b95c2e7b73e8..3d1e818eac8f 100644 --- a/browser/api/lib/browser-window.coffee +++ b/browser/api/lib/browser-window.coffee @@ -13,8 +13,8 @@ BrowserWindow::_init = -> # Tell the rpc server that a render view has been deleted and we need to # release all objects owned by it. - @on 'render-view-deleted', -> - process.emit 'ATOM_BROWSER_RELEASE_RENDER_VIEW', @getProcessId(), @getRoutingId() + @on 'render-view-deleted', (event, processId, routingId) -> + process.emit 'ATOM_BROWSER_RELEASE_RENDER_VIEW', processId, routingId BrowserWindow::toggleDevTools = -> if @isDevToolsOpened() then @closeDevTools() else @openDevTools() diff --git a/browser/native_window.cc b/browser/native_window.cc index 7a2353e574de..b85ca5cb939c 100644 --- a/browser/native_window.cc +++ b/browser/native_window.cc @@ -271,7 +271,10 @@ void NativeWindow::NotifyWindowClosed() { // The OnRenderViewDeleted is not called when the WebContents is destroyed // directly (e.g. when closing the window), so we make sure it's always // emitted to users by sending it before window is closed.. - FOR_EACH_OBSERVER(NativeWindowObserver, observers_, OnRenderViewDeleted()); + FOR_EACH_OBSERVER(NativeWindowObserver, observers_, + OnRenderViewDeleted( + GetWebContents()->GetRenderProcessHost()->GetID(), + GetWebContents()->GetRoutingID())); is_closed_ = true; FOR_EACH_OBSERVER(NativeWindowObserver, observers_, OnWindowClosed()); @@ -398,8 +401,10 @@ bool NativeWindow::OnMessageReceived(const IPC::Message& message) { return handled; } -void NativeWindow::RenderViewDeleted(content::RenderViewHost*) { - FOR_EACH_OBSERVER(NativeWindowObserver, observers_, OnRenderViewDeleted()); +void NativeWindow::RenderViewDeleted(content::RenderViewHost* rvh) { + FOR_EACH_OBSERVER(NativeWindowObserver, observers_, + OnRenderViewDeleted(rvh->GetProcess()->GetID(), + rvh->GetRoutingID())); } void NativeWindow::RenderViewGone(base::TerminationStatus status) { diff --git a/browser/native_window_observer.h b/browser/native_window_observer.h index 20b8ddcc17f0..5d697480050f 100644 --- a/browser/native_window_observer.h +++ b/browser/native_window_observer.h @@ -36,7 +36,7 @@ class NativeWindowObserver { virtual void OnRendererResponsive() {} // Called when a render view has been deleted. - virtual void OnRenderViewDeleted() {} + virtual void OnRenderViewDeleted(int process_id, int routing_id) {} // Called when renderer has crashed. virtual void OnRendererCrashed() {}