984458 - Investigate using a JNI/JS-API implementation of JSONObject

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

There have been concerns about JSON being inefficient when used for Java-JS events. Without switching to another system, we can provide our own JSONObject implementation that uses the SpiderMonkey JS API internally through JNI. This way we don't ever convert between JS objects and JSON. Instead of passing JSON, we simply pass a SpiderMonkey object wrapped in our JSONObject.

Comment 1

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

Attachment: Bug 984458 - Add NativeJSContainer implementation;

Comment 2

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

Attachment: Bug 984458 - Add skeletal NativeJSObject implementation;

Comment 3

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

Attachment: Bug 984458 - Add NativeJSObject property getters;

Comment 4

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

Attachment: Add NativeJSContainer implementation (v1)

This patch adds an empty NativeJSObject class, which will become a JNI wrapper around JSObject, using an API similar to JSONObject.

The patch also implements a basic NativeJSContainer. NativeJSContainer inherits from NativeJSObject, but also adds additional memory management features. In particular, it has a dispose() method that, once called, will release the wrapped JSObject for GC.

Comment 5

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

Attachment: Add threading support to NativeJSContainer (v1)

Because JSRuntime is single-threaded, we must make sure we only access our wrapped JSObject from its own thread. This patch adds more stringent checks to make sure this happens.

The patch also adds a mechanism to pass a NativeJSContainer to another thread by implementing a clone() method. This works by serializing the wrapped JSObject internally, then constructing a new NativeJSContainer from the serialized data. When another thread first uses the new NativeJSContainer, we deserialize the data on the new thread and the new NativeJSContainer becomes attached to that thread.

This mechanism is not needed right now, but may become useful if we ever decide we need to use NativeJSContainer on the UI thread, for example.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8396638 [details] [diff] [review]
Add NativeJSContainer implementation (v1)

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

::: mobile/android/base/util/NativeJSContainer.java
@@ +23,5 @@
> +    /**
> +     * Dispose all associated native objects. Subsequent use of any objects derived from
> +     * this container will throw a NullPointerException.
> +     */
> +    public native void dispose();

I'd still like to figure out a better way to handle the life cycle of these, but this is fine for now.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8396654 [details] [diff] [review]
Add threading support to NativeJSContainer (v1)

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

f+, but I want to see a new patch that throws if used on the wrong thread

::: mobile/android/base/util/NativeJSContainer.java
@@ +12,5 @@
>   * access Javascript objects in Java.
> + *
> + * A container must only be used on the thread it is attached to. To use it on another
> + * thread, call {@link #clone()} to make a copy, and use the copy on the other thread.
> + * When a copy is first used, it becomes attached to the thread using it.

Let's check the thread in the native code and throw an exception if you're not on the right thread.

Comment 8

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

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> Comment on attachment 8396654 [details] [diff] [review]
> Add threading support to NativeJSContainer (v1)
> 
> Let's check the thread in the native code and throw an exception if you're
> not on the right thread.

Yep, we do check it in NativeJSContainer::EnsureObject, which will be used in other patches whenever the object is accessed.

Comment 9

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

Attachment: c. Add skeletal NativeJSObject implementation; (v1)

This patch implements some of the NativeJSObject functionality, including toString() which returns the JSON of the object.

Comment 10

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

Attachment: d. Add NativeJSObject property getters (v1)

Adds most of the get* methods.

Comment 11

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

Attachment: e. Add NativeJSObject object getter (v1)

Add getObject()

Comment 12

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

Attachment: f. Add NativeJSObject opt getters (v1)

This patch implements the opt* methods from the JSONObject interface.

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7821e5836e71
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3bfc6b95ea3
https://hg.mozilla.org/integration/mozilla-inbound/rev/93f0f89e30da
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9bea24490d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7f49db0b09
https://hg.mozilla.org/integration/mozilla-inbound/rev/99a8ea57ec65

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7821e5836e71
https://hg.mozilla.org/mozilla-central/rev/c3bfc6b95ea3
https://hg.mozilla.org/mozilla-central/rev/93f0f89e30da
https://hg.mozilla.org/mozilla-central/rev/a9bea24490d6
https://hg.mozilla.org/mozilla-central/rev/1c7f49db0b09
https://hg.mozilla.org/mozilla-central/rev/99a8ea57ec65

Comment 15

[Jon Coppeard (:jonco) (PTO until 27th Jan)]

Comment on attachment 8396638 [details] [diff] [review]
Add NativeJSContainer implementation (v1)

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

::: widget/android/NativeJSContainer.cpp
@@ +78,5 @@
> +    }
> +
> +    JSContext* const mContext;
> +    // Root JS object
> +    const JS::Heap<JSObject*> mJSObject;

How does the GC rooting work here?  JS::Heap<> does not root its contents.

I'm worried this might be the cause of crashes like bug 1011474.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Jon Coppeard (:jonco) from comment #15)
> Comment on attachment 8396638 [details] [diff] [review]
> Add NativeJSContainer implementation (v1)
> 
> Review of attachment 8396638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/NativeJSContainer.cpp
> @@ +78,5 @@
> > +    }
> > +
> > +    JSContext* const mContext;
> > +    // Root JS object
> > +    const JS::Heap<JSObject*> mJSObject;
> 
> How does the GC rooting work here?  JS::Heap<> does not root its contents.
> 
> I'm worried this might be the cause of crashes like bug 1011474.

Sorry, I don't know.

Comment 17

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

(In reply to Jon Coppeard (:jonco) from comment #15)
> Comment on attachment 8396638 [details] [diff] [review]
> Add NativeJSContainer implementation (v1)
> 
> Review of attachment 8396638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/NativeJSContainer.cpp
> @@ +78,5 @@
> > +    }
> > +
> > +    JSContext* const mContext;
> > +    // Root JS object
> > +    const JS::Heap<JSObject*> mJSObject;
> 
> How does the GC rooting work here?  JS::Heap<> does not root its contents.
> 
> I'm worried this might be the cause of crashes like bug 1011474.

That's possible. How should JSObject* be rooted when it's on the heap?

Comment 18

[Jon Coppeard (:jonco) (PTO until 27th Jan)]

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

[Jon Coppeard (:jonco) (PTO until 27th Jan)]

Attachment: add-NativeJSContainer-tracing

Here's a patch to add tracing for JSObject*s held by a NativeJSContainer.

This adds a GC roots tracer for every object.  This is not ideal, but the best available option at the moment as described in the previous comment.

Comment 20

[Jon Coppeard (:jonco) (PTO until 27th Jan)]

It seems this patch does not fix bug 1011474.

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 8436477 [details] [diff] [review]
add-NativeJSContainer-tracing

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

Works for me.

Comment 22

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

Sorry, can we work on this in bug 1022769? This bug is resolved and regressions should go in separate bugs.

Comment 23

[Jon Coppeard (:jonco) (PTO until 27th Jan)]

Comment on attachment 8436477 [details] [diff] [review]
add-NativeJSContainer-tracing

Work on this has moved to bug 1022769.