Closed Bug 984458 Opened 6 years ago Closed 6 years ago

Investigate using a JNI/JS-API implementation of JSONObject

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(6 files, 4 obsolete files)

16.10 KB, patch
blassey
: review+
Details | Diff | Splinter Review
8.92 KB, patch
blassey
: review+
blassey
: feedback+
Details | Diff | Splinter Review
10.02 KB, patch
blassey
: review+
Details | Diff | Splinter Review
19.06 KB, patch
blassey
: review+
Details | Diff | Splinter Review
11.03 KB, patch
blassey
: review+
Details | Diff | Splinter Review
20.98 KB, patch
blassey
: review+
Details | Diff | Splinter Review
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.
Depends on: 987281
Depends on: 987284
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.
Attachment #8395849 - Attachment is obsolete: true
Attachment #8396638 - Flags: review?(blassey.bugs)
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.
Attachment #8396654 - Flags: review?(blassey.bugs)
Assignee: nobody → nchen
Status: NEW → ASSIGNED
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.
Attachment #8396638 - Flags: review?(blassey.bugs) → review+
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.
Attachment #8396654 - Flags: review?(blassey.bugs) → feedback+
(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.
This patch implements some of the NativeJSObject functionality, including toString() which returns the JSON of the object.
Attachment #8398059 - Flags: review?(blassey.bugs)
Adds most of the get* methods.
Attachment #8395850 - Attachment is obsolete: true
Attachment #8395851 - Attachment is obsolete: true
Attachment #8398062 - Flags: review?(blassey.bugs)
Attachment #8398059 - Attachment description: Bug 984458 - c. Add skeletal NativeJSObject implementation; → c. Add skeletal NativeJSObject implementation; (v1)
Add getObject()
Attachment #8398065 - Flags: review?(blassey.bugs)
Attachment #8398059 - Flags: review?(blassey.bugs) → review+
Attachment #8398062 - Flags: review?(blassey.bugs) → review+
Attachment #8398065 - Flags: review?(blassey.bugs) → review+
Attachment #8396654 - Flags: review?(blassey.bugs)
Attachment #8396654 - Flags: review?(blassey.bugs) → review+
This patch implements the opt* methods from the JSONObject interface.
Attachment #8399601 - Flags: review?(blassey.bugs)
Attachment #8399601 - Flags: review?(blassey.bugs) → review+
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.
Flags: needinfo?(blassey.bugs)
(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.
Flags: needinfo?(blassey.bugs)
(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?
Flags: needinfo?(jcoppeard)
(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.
Flags: needinfo?(jcoppeard)
Attached patch add-NativeJSContainer-tracing (obsolete) — Splinter Review
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.
Attachment #8436477 - Flags: review?(terrence)
Attachment #8436477 - Flags: review?(nchen)
It seems this patch does not fix bug 1011474.
Comment on attachment 8436477 [details] [diff] [review]
add-NativeJSContainer-tracing

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

Works for me.
Attachment #8436477 - Flags: review?(terrence) → review+
Sorry, can we work on this in bug 1022769? This bug is resolved and regressions should go in separate bugs.
Depends on: 1022769
Attachment #8436477 - Flags: review?(nchen)
Comment on attachment 8436477 [details] [diff] [review]
add-NativeJSContainer-tracing

Work on this has moved to bug 1022769.
Attachment #8436477 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.