1022769 - NativeJSObject does not root objects correctly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

From bug 984458,

(In reply to Jon Coppeard (:jonco) from comment #18)
> (In reply to Jim Chen [:jchen :nchen] from comment #17)
> 
> > That's possible. How should JSObject* be rooted when it's on the heap?
> 
> Every pointer to a GC thing must be traced, and there's a few ways this can
> happen, in order of decreasing preference:
> 
>  1) Traced as part of a cycle collected object
>  2) Rooted by wrapping in a PersistentRooted<>
>  3) Rooted by calling JS_Add/RemoveObjectRoot(), although this is deprecated
> in favour of 2.
>  4) Rooted by adding a tracer with JS_Add/RemoveExtraGCRootsTracer()
> 
> I'm not familiar with this code, but it seems that NativeJSContainer objects
> are created and passed to the Java side over JNI and that there are no
> references to them on the Gecko side.  Is that correct?  If so it makes it
> hard to trace them using the cycle collector. 
> 
> Also, I think the lifetime is determined by the Java side calling through
> into DisposeInstance().  Is that hooked up to the Java GC or does it get
> called directly?
> 
> Option 2 sounds like a good idea except it doesn't work with a Vector of
> JSObject*, both for efficiency reasons and because it doesn't compile
> (possibly we should fix this, but Vectors of PersistentRooted things aren't
> really the way to go anyway).
> 
> Option 3 also doesn't work for elements in a Vector as they can get moved in
> memory if the Vector is resized.
> 
> So that leaves us with adding a custom roots tracer for this.  This isn't
> ideal, but I think it's good for now and it we can see if it fixes the
> crashes.  I've got a patch that does this.
> 
> In the longer term, we could look at adding an API to better support
> situations like this.  Or maybe it is possible to rearchitect things to
> trace these via the cycle collector somehow.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Use PersistentRooted to root NativeJSContainer objects

(In reply to Jon Coppeard (:jonco) from comment #18)
>
> I'm not familiar with this code, but it seems that NativeJSContainer objects
> are created and passed to the Java side over JNI and that there are no
> references to them on the Gecko side.  Is that correct?  If so it makes it
> hard to trace them using the cycle collector. 

That's correct.

> Also, I think the lifetime is determined by the Java side calling through
> into DisposeInstance().  Is that hooked up to the Java GC or does it get
> called directly?

dispose() is called explicitly by Java code.

> Option 2 sounds like a good idea except it doesn't work with a Vector of
> JSObject*, both for efficiency reasons and because it doesn't compile
> (possibly we should fix this, but Vectors of PersistentRooted things aren't
> really the way to go anyway).

Can you explain more why Vectors of PersistentRooted things aren't the way to go? To get around Vector not liking PersistentRooted, we can allocate PersistentRooted separately and store a pointer in the Vector. I attached a patch that does that.

But since I'm not experienced in GC rooting, if you think JS_Add/RemoveExtraGCRootsTracer is the better way. Feel free to land your patch in this bug :)  I'll post my review of your patch soon.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

r+ on your patch with the change below. Thanks!

>diff --git a/widget/android/NativeJSContainer.cpp b/widget/android/NativeJSContainer.cpp
>--- a/widget/android/NativeJSContainer.cpp
>+++ b/widget/android/NativeJSContainer.cpp
>@@ -138,17 +143,22 @@ public:
> 
>         JSAutoStructuredCloneBuffer buffer;
>         if (!buffer.write(cx, JS::RootedValue(cx, JS::ObjectValue(*object)))) {
>             AndroidBridge::ThrowException(env,
>                 "java/lang/UnsupportedOperationException",
>                 "Cannot serialize object");
>             return nullptr;
>         }
>-        return CreateInstance(env, new NativeJSContainer(cx, Move(buffer)));
>+        NativeJSContainer* newContainer = new NativeJSContainer(cx, Move(buffer));
>+        if (!newContainer->RegisterGCTracer()) {

We should move this RegisterGCTracer call to EnsureObject(), after the SwitchContextToCurrentThread() call [1]. The reason is, in this code path, we are constructing NativeJSContainer to be *not* attached to a particular JSRuntime. We later bind to a JSRuntime in EnsureObject(), so we should register the tracer there.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/NativeJSContainer.cpp#223

>+            delete newContainer;
>+            return nullptr;
>+        }
>+        return CreateInstance(env, newContainer);
>     }
> 
>     static jobject GetInstanceFromObject(JNIEnv* env, jobject object) {
>         MOZ_ASSERT(object);
> 
>         const jobject instance = env->GetObjectField(object, jObjectContainer);
>         MOZ_ASSERT(instance);
>         return instance;

>@@ -94,17 +94,22 @@ public:
>         MOZ_ASSERT(jObject);
>         jString = AndroidBridge::GetClassGlobalRef(
>             env, "java/lang/String");
>         MOZ_ASSERT(jString);
>     }
> 
>     static jobject CreateInstance(JNIEnv* env, JSContext* cx,
>                                   JS::HandleObject object) {
>-        return CreateInstance(env, new NativeJSContainer(cx, object));
>+        NativeJSContainer *container = new NativeJSContainer(cx, object);
>+        if (!container->RegisterGCTracer()) {

This other RegisterGCTracer call can stay because in this code path, we bind to the current JSRuntime immediately. However, I'd prefer that the call go inside the constructor, and we assert it always returns true.

>+            delete container;
>+            return nullptr;
>+        }
>+        return CreateInstance(env, container);
>     }
> 
>     static NativeJSContainer* FromInstance(JNIEnv* env, jobject instance) {
>         MOZ_ASSERT(instance);
> 
>         const jlong fieldValue =
>             env->GetLongField(instance, jContainerNativeObject);
>         NativeJSContainer* const nativeObject =

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Jim Chen [:jchen :nchen] from comment #1)
> Can you explain more why Vectors of PersistentRooted things aren't the way
> to go? To get around Vector not liking PersistentRooted, we can allocate
> PersistentRooted separately and store a pointer in the Vector. I attached a
> patch that does that.

PersistentRooted<JSObject*> is a linked list element containing the JSObject* so overhead for the previous and next pointers makes this three times the size of the just the pointer.  Also, when it comes to marking it's faster to iterate through a linear array than a linked list.  Having said that, if you know the list is going to be small then it doesn't matter much.

Do you know the likely expected size of this list?

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8437379 [details] [diff] [review]
Use PersistentRooted to root NativeJSContainer objects

Review of attachment 8437379 [details] [diff] [review]:
-----------------------------------------------------------------

I like this approach.  If the size of the vector is always going to be small, then let's do this.

::: widget/android/NativeJSContainer.cpp
@@ +197,5 @@
>              return nullptr;
>          }
>          size_t newIndex = container->mRootedObjects.length();
> +        PersistentObjectPtr rootedJSObject(new PersistentObject(cx, jsObject));
> +        if (!container->mRootedObjects.append(Move(rootedJSObject))) {

It's a shame we have to construct PersistentObjectPtr on the stack first but this is another symptom of the fact that these don't work well with Vectors.

@@ +286,5 @@
>      PRThread* mThread;
>      // Context that the object is valid in
>      JSContext* mThreadContext;
>      // Deserialized object, or nullptr if object is in serialized form
> +    PersistentObjectPtr mJSObject;

It isn't necessary to make this a pointer to a PersistentRooted<>.  Just PersistentObject works here.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8437379 [details] [diff] [review]
Use PersistentRooted to root NativeJSContainer objects

Review of attachment 8437379 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jon Coppeard (:jonco) from comment #4)
> Comment on attachment 8437379 [details] [diff] [review]
> Use PersistentRooted to root NativeJSContainer objects
> 
> Review of attachment 8437379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like this approach.  If the size of the vector is always going to be
> small, then let's do this.

Yup, the vector is empty in most cases and small in other cases.

> @@ +286,5 @@
> >      PRThread* mThread;
> >      // Context that the object is valid in
> >      JSContext* mThreadContext;
> >      // Deserialized object, or nullptr if object is in serialized form
> > +    PersistentObjectPtr mJSObject;
> 
> It isn't necessary to make this a pointer to a PersistentRooted<>.  Just
> PersistentObject works here.

We need a pointer here to implement late-binding to a different JSRuntime (see the first paragraph in comment 2). nullptr here means we haven't bound to a JSRuntime yet.

Thanks!

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8437379 [details] [diff] [review]
Use PersistentRooted to root NativeJSContainer objects

Review of attachment 8437379 [details] [diff] [review]:
-----------------------------------------------------------------

In that case, great.  r=me.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Use PersistentRooted to root NativeJSContainer objects (v1.1)

For checkin. Try, https://tbpl.mozilla.org/?tree=Try&rev=aeabdca25642

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e31a31b2148f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e31a31b2148f