Bug 1187552 - Make NativeJSContainer use direct ownership model; r=snorp
authorJim Chen <nchen@mozilla.com>
Tue, 04 Aug 2015 17:47:28 -0400
changeset 287894 6696b897b06e06a743ce6f78ce72dc74408c56f7
parent 287893 20a9ea2255877e3697c4838eb372b3fe8bc8fc54
child 287895 5a5b8c2939064f79c1289f36d19a18f47f71ebaa
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssnorp
bugs1187552
milestone42.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1187552 - Make NativeJSContainer use direct ownership model; r=snorp Make NativeJSContainer/NativeJSObject Java objects own their corresponding C++ objects directly, to reduce an extra allocation/deallocation for each object and to simplify code.
mobile/android/tests/browser/robocop/testEventDispatcher.java
widget/android/NativeJSContainer.cpp
--- a/mobile/android/tests/browser/robocop/testEventDispatcher.java
+++ b/mobile/android/tests/browser/robocop/testEventDispatcher.java
@@ -31,16 +31,17 @@ public class testEventDispatcher extends
     private static final String TEST_JS = "testEventDispatcher.js";
     private static final String GECKO_EVENT = "Robocop:TestGeckoEvent";
     private static final String GECKO_RESPONSE_EVENT = "Robocop:TestGeckoResponse";
     private static final String NATIVE_EVENT = "Robocop:TestNativeEvent";
     private static final String NATIVE_RESPONSE_EVENT = "Robocop:TestNativeResponse";
     private static final String NATIVE_EXCEPTION_EVENT = "Robocop:TestNativeException";
 
     private JavascriptBridge js;
+    private NativeJSObject savedMessage;
 
     @Override
     public void setUp() throws Exception {
         super.setUp();
         js = new JavascriptBridge(this);
 
         EventDispatcher.getInstance().registerGeckoThreadListener(
                 (GeckoEventListener) this, GECKO_EVENT, GECKO_RESPONSE_EVENT);
@@ -67,16 +68,24 @@ public class testEventDispatcher extends
 
         js.syncCall("send_test_message", GECKO_EVENT);
         js.syncCall("send_message_for_response", GECKO_RESPONSE_EVENT, "success");
         js.syncCall("send_message_for_response", GECKO_RESPONSE_EVENT, "error");
         js.syncCall("send_test_message", NATIVE_EVENT);
         js.syncCall("send_message_for_response", NATIVE_RESPONSE_EVENT, "success");
         js.syncCall("send_message_for_response", NATIVE_RESPONSE_EVENT, "error");
         js.syncCall("send_test_message", NATIVE_EXCEPTION_EVENT);
+
+        fAssertNotSame("Should have saved a message", null, savedMessage);
+        try {
+            savedMessage.toString();
+            fFail("Using NativeJSContainer should throw after disposal");
+        } catch (final NullPointerException e) {
+        }
+
         js.syncCall("finish_test");
     }
 
     @Override
     public void handleMessage(final String event, final JSONObject message) {
         ThreadUtils.assertOnGeckoThread();
 
         try {
@@ -170,16 +179,19 @@ public class testEventDispatcher extends
             }
 
             try {
                 message.getString("int");
                 fFail("Wrong property type should throw InvalidPropertyException");
             } catch (final NativeJSObject.InvalidPropertyException e) {
             }
 
+            // Save this message for post-disposal check.
+            savedMessage = message;
+
             // Save this test for last; make sure EventDispatcher catches InvalidPropertyException.
             message.getString("nonexistent_string");
             fFail("EventDispatcher should catch InvalidPropertyException");
 
         } else {
             fFail("Event type should be valid: " + event);
         }
     }
--- a/widget/android/NativeJSContainer.cpp
+++ b/widget/android/NativeJSContainer.cpp
@@ -9,17 +9,16 @@
 
 #include "Bundle.h"
 #include "GeneratedJNINatives.h"
 #include "MainThreadUtils.h"
 #include "jsapi.h"
 #include "nsJSUtils.h"
 
 #include <mozilla/Vector.h>
-#include <mozilla/WeakPtr.h>
 #include <mozilla/jni/Accessors.h>
 #include <mozilla/jni/Refs.h>
 #include <mozilla/jni/Utils.h>
 
 /**
  * NativeJSContainer.cpp implements the native methods in both
  * NativeJSContainer and NativeJSObject, using JSAPI to retrieve values from a
  * JSObject and using JNI to return those values to Java code.
@@ -99,18 +98,17 @@ public:
     size_t Length() const {
         return static_cast<size_t>(mEnv->GetStringLength(mJNIString.Get()));
     }
 };
 
 } // namepsace
 
 class NativeJSContainerImpl final
-    : public SupportsWeakPtr<NativeJSContainerImpl>
-    , public NativeJSObject::Natives<NativeJSContainerImpl>
+    : public NativeJSObject::Natives<NativeJSContainerImpl>
     , public NativeJSContainer::Natives<NativeJSContainerImpl>
 {
     typedef NativeJSContainerImpl Self;
     typedef NativeJSContainer::Natives<NativeJSContainerImpl> ContainerBase;
     typedef NativeJSObject::Natives<NativeJSContainerImpl> ObjectBase;
 
     typedef JS::PersistentRooted<JSObject*> PersistentObject;
 
@@ -571,65 +569,64 @@ class NativeJSContainerImpl final
         }
 
         if (!CheckProperty(Prop::InValue, val)) {
             return typename Prop::NativeType();
         }
         return (this->*Prop::FromValue)(val);
     }
 
+    NativeJSObject::LocalRef CreateChild(JS::HandleObject object)
+    {
+        auto instance = NativeJSObject::New();
+        mozilla::UniquePtr<NativeJSContainerImpl> impl(
+                new NativeJSContainerImpl(instance.Env(), mJSContext, object));
+
+        ObjectBase::AttachNative(instance, mozilla::Move(impl));
+        mChildren.append(NativeJSObject::GlobalRef(instance));
+        return instance;
+    }
+
     NativeJSContainerImpl(JNIEnv* env, JSContext* cx, JS::HandleObject object)
         : mEnv(env)
         , mJSContext(cx)
         , mJSObject(cx, object)
     {}
 
+public:
     ~NativeJSContainerImpl()
     {
         // Dispose of all children on destruction. The children will in turn
         // dispose any of their children (i.e. our grandchildren) and so on.
-        NativeJSObject::LocalRef childObj(mEnv);
+        NativeJSObject::LocalRef child(mEnv);
         for (size_t i = 0; i < mChildren.length(); i++) {
-            childObj = mChildren[i];
-            Self* const child = ObjectBase::GetNative(childObj);
-            child->ObjectBase::DisposeNative(childObj);
-            delete child;
+            child = mChildren[i];
+            ObjectBase::GetNative(child)->ObjectBase::DisposeNative(child);
         }
     }
 
-    NativeJSObject::LocalRef CreateChild(JS::HandleObject object)
-    {
-        auto instance = NativeJSObject::New();
-        (new NativeJSContainerImpl(instance.Env(), mJSContext, object))
-                ->ObjectBase::AttachNative(instance);
-        mChildren.append(NativeJSObject::GlobalRef(instance));
-        return instance;
-    }
-
-public:
     static NativeJSContainer::LocalRef
     CreateInstance(JSContext* cx, JS::HandleObject object)
     {
         auto instance = NativeJSContainer::New();
-        (new NativeJSContainerImpl(instance.Env(), cx, object))
-                ->ContainerBase::AttachNative(instance);
+        mozilla::UniquePtr<NativeJSContainerImpl> impl(
+                new NativeJSContainerImpl(instance.Env(), cx, object));
+
+        ContainerBase::AttachNative(instance, mozilla::Move(impl));
         return instance;
     }
 
-    MOZ_DECLARE_WEAKREFERENCE_TYPENAME(NativeJSContainerImpl)
-
     // NativeJSContainer methods
 
     void DisposeNative(const NativeJSContainer::LocalRef& instance)
     {
         if (!CheckThread()) {
             return;
         }
         ContainerBase::DisposeNative(instance);
-        delete this;
     }
 
     NativeJSContainer::LocalRef Clone()
     {
         if (!CheckThread()) {
             return nullptr;
         }
         return CreateInstance(mJSContext, mJSObject);