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/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..ddc5d5cf07e1 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; @@ -80,6 +76,13 @@ void Window::OnRendererResponsive() { Emit("responsive"); } +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() { Emit("crashed"); } @@ -105,15 +108,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_api_window.h b/browser/api/atom_api_window.h index 03709dc22a10..92c470203e7a 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(int process_id, int routing_id) OVERRIDE; virtual void OnRendererCrashed() OVERRIDE; private: 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/browser-window.coffee b/browser/api/lib/browser-window.coffee index 3da99104b91b..3d1e818eac8f 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', (event, processId, routingId) -> + process.emit 'ATOM_BROWSER_RELEASE_RENDER_VIEW', processId, routingId + BrowserWindow::toggleDevTools = -> if @isDevToolsOpened() then @closeDevTools() else @openDevTools() 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/browser/atom/rpc-server.coffee b/browser/atom/rpc-server.coffee index 6b56bfca2063..6e072b661864 100644 --- a/browser/atom/rpc-server.coffee +++ b/browser/atom/rpc-server.coffee @@ -78,6 +78,10 @@ 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) -> + objectsRegistry.clear processId, routingId + ipc.on 'ATOM_BROWSER_REQUIRE', (event, processId, routingId, module) -> try event.returnValue = valueToMeta processId, routingId, require(module) @@ -90,10 +94,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..b85ca5cb939c 100644 --- a/browser/native_window.cc +++ b/browser/native_window.cc @@ -268,6 +268,14 @@ 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( + GetWebContents()->GetRenderProcessHost()->GetID(), + GetWebContents()->GetRoutingID())); + is_closed_ = true; FOR_EACH_OBSERVER(NativeWindowObserver, observers_, OnWindowClosed()); @@ -393,6 +401,12 @@ bool NativeWindow::OnMessageReceived(const IPC::Message& message) { return handled; } +void NativeWindow::RenderViewDeleted(content::RenderViewHost* rvh) { + FOR_EACH_OBSERVER(NativeWindowObserver, observers_, + OnRenderViewDeleted(rvh->GetProcess()->GetID(), + rvh->GetRoutingID())); +} + 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..5d697480050f 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(int process_id, int routing_id) {} + // Called when renderer has crashed. virtual void OnRendererCrashed() {} }; 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... 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..90ae2fc79d2b 100644 --- a/common/v8_conversions.h +++ b/common/v8_conversions.h @@ -11,7 +11,12 @@ #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 "common/v8_value_converter_impl.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 +65,13 @@ struct FromV8Value { width->IntegerValue(), height->IntegerValue()); } + operator scoped_ptr() { + scoped_ptr converter( + new atom::V8ValueConverterImpl); + 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 +196,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 +236,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; } diff --git a/renderer/api/atom_api_renderer_ipc.cc b/renderer/api/atom_api_renderer_ipc.cc index d12634797226..3600a3923153 100644 --- a/renderer/api/atom_api_renderer_ipc.cc +++ b/renderer/api/atom_api_renderer_ipc.cc @@ -4,17 +4,14 @@ #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" using content::RenderView; -using content::V8ValueConverter; using WebKit::WebFrame; using WebKit::WebView; @@ -26,7 +23,6 @@ namespace { RenderView* GetCurrentRenderView() { WebFrame* frame = WebFrame::frameForCurrentContext(); - DCHECK(frame); if (!frame) return NULL; @@ -34,9 +30,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 @@ -45,23 +39,14 @@ RenderView* GetCurrentRenderView() { v8::Handle RendererIPC::Send(const v8::Arguments &args) { v8::HandleScope scope; - if (!args[0]->IsString()) + string16 channel; + scoped_ptr arguments; + if (!FromV8Arguments(args, &channel, &arguments)) 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 - // 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(), @@ -78,24 +63,14 @@ 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; + scoped_ptr arguments; + if (!FromV8Arguments(args, &channel, &arguments)) 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); - 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, context)); - - 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/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" 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 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) 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__)) 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'