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