Merge pull request #139 from atom/browser-cleaner

Do not rely on renderer to report the deletion of render view

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',

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<v8::Value> BrowserIPC::Send(const v8::Arguments &args) {
 
   string16 channel;
   int process_id, routing_id;
-  if (!FromV8Arguments(args, &channel, &process_id, &routing_id))
+  scoped_ptr<base::Value> 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::Array> 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<V8ValueConverter> converter(new V8ValueConverterImpl());
-  scoped_ptr<base::Value> 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,

browser/api/atom_api_event_emitter.cc

@@ -7,10 +7,8 @@
 #include <vector>
 
 #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::Handle<v8::Value>> 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;

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<v8::Value> 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<V8ValueConverter> converter(new V8ValueConverterImpl());
-  scoped_ptr<base::Value> options(
-      converter->FromV8Value(args[0], v8::Context::GetCurrent()));
+  scoped_ptr<base::Value> 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<base::DictionaryValue*>(options.get()));
 

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:

browser/api/atom_browser_bindings.cc

@@ -7,10 +7,8 @@
 #include <vector>
 
 #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"

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()
 

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: ->

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'

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());
 }

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;
 

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() {}
 };

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...

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 <typename T>
+class HasSwapMethod {
+  typedef char one;
+  typedef long two;
+  template <typename C> static one test(char[sizeof(&C::swap)]) ;
+  template <typename C> static two test(...);
+ public:
+  enum { value = sizeof(test<T>(0)) == sizeof(char) };
+};
+
+template<bool B, class T = void>
+struct enable_if {};
+
+template<class T>
+struct enable_if<true, T> { typedef T type; };
+
+template<typename T>
+typename enable_if<HasSwapMethod<T>::value>::type SwapOrAssign(
+    T& v1, const T& v2) {
+  v1.swap(const_cast<T&>(v2));
+}
+
+template<typename T>
+typename enable_if<!HasSwapMethod<T>::value>::type SwapOrAssign(
+    T& v1, const T& v2) {
+  v1 = v2;
+}
+
+}  // namespace internal
+
+#endif  // ATOM_COMMON_SWAP_OR_ASSIGN_H_

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<base::Value>() {
+    scoped_ptr<content::V8ValueConverter> converter(
+        new atom::V8ValueConverterImpl);
+    return scoped_ptr<base::Value>(
+        converter->FromV8Value(value_, v8::Context::GetCurrent()));
+  }
+
   operator std::vector<std::string>() {
     std::vector<std::string> array;
     v8::Handle<v8::Array> v8_array = v8::Handle<v8::Array>::Cast(value_);
@@ -184,6 +196,12 @@ bool V8ValueCanBeConvertedTo<gfx::Rect>(v8::Handle<v8::Value> value) {
   return value->IsObject();
 }
 
+template<> inline
+bool V8ValueCanBeConvertedTo<scoped_ptr<base::Value>>(
+    v8::Handle<v8::Value> value) {
+  return value->IsObject();
+}
+
 template<> inline
 bool V8ValueCanBeConvertedTo<std::vector<std::string>>(
     v8::Handle<v8::Value> value) {
@@ -218,7 +236,8 @@ template<typename T1> inline
 bool FromV8Arguments(const v8::Arguments& args, T1* value, int index = 0) {
   if (!V8ValueCanBeConvertedTo<T1>(args[index]))
     return false;
-  *value = static_cast<const T1&>(FromV8Value(args[index]));
+  internal::SwapOrAssign(*value,
+                         static_cast<const T1&>(FromV8Value(args[index])));
   return true;
 }
 

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<v8::Value> RendererIPC::Send(const v8::Arguments &args) {
   v8::HandleScope scope;
 
-  if (!args[0]->IsString())
+  string16 channel;
+  scoped_ptr<base::Value> 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::Array> 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<V8ValueConverter> converter(V8ValueConverter::create());
-  scoped_ptr<base::Value> 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<v8::Value> RendererIPC::Send(const v8::Arguments &args) {
 v8::Handle<v8::Value> RendererIPC::SendSync(const v8::Arguments &args) {
   v8::HandleScope scope;
 
-  if (!args[0]->IsString())
+  string16 channel;
+  scoped_ptr<base::Value> arguments;
+  if (!FromV8Arguments(args, &channel, &arguments))
     return node::ThrowTypeError("Bad argument");
 
-  v8::Handle<v8::Context> 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::Array> 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<V8ValueConverter> converter(V8ValueConverter::create());
-  scoped_ptr<base::Value> 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(

renderer/api/atom_renderer_bindings.cc

@@ -7,10 +7,8 @@
 #include <vector>
 
 #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"

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

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)

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__))

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'