578821 - Rooting of jetpack handles is too simplistic

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

The original design of jetpack handles was simple, but won't meet their needs. It rooted both sides of the handle, and some code was responsible for cleaning up the handle when it was no longer needed.

This won't work, because there are a fair number of cases where a handle needs to live as long as the client holds it, e.g. the handle for an XMLHttpRequest object needs to live until the client throws it away. Otherwise, we'd have to eagerly send over every possible bit of data that the client might want from the XMLHttpRequest, which is a fair bit of data that most clients won't need.

So I'm proposing the following change to the handle API:

// Make a handle live only until it is no longer reachable from the calling code.
handle.unroot();

// Register a callback which is notified when a handle bmecomes invalid, either
// through an explicit invalidate() call or being unrooted and then unreachable.
// This method is only called on the *other* side of a conversation.
handle.onInvalidate(fn);

When a handle is created, it is rooted. Handles can always reach their parent.

Comment 1

[Benjamin Smedberg]

Attachment: Add an .isRooted property, true by default, and .onInvalidate callback function, rev. 1

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 460628 [details] [diff] [review]
Add an .isRooted property, true by default, and .onInvalidate callback function, rev. 1

>+    , mRooted(true)

Hm, this isn't really true... Why not make it false to begin with?

>+    JS_AddNamedObjectRoot(mCx, &mObj, "Jetpack Handle");

That can fail, theoretically. Do you care?

>+        NS_ASSERTION(mRooted, "Initialized incorrectly");
>+        JS_AddNamedObjectRoot(mCx, &mObj, "Jetpack Handle");

If you make mRooted default to false then you could just call Root() here.

>+        if (JS_HasProperty(mCx, obj.object(), "onInvalidate",

DOM uses all lowercase (e.g. "onload") for event handlers... Do you want to follow that convention here?

>+          JS_CallFunctionName(mCx, obj.object(), "onInvalidate", 0, NULL,
>+                              r.addr());

The handler could throw an exception and screw you up later so I think you either want to report any pending exceptions or clear them here.

>+  SetIsRooted(JSContext* cx, JSObject* obj, jsval, jsval* vp) {
> ...
>+    else
>+      self->Unroot();

It's possible to have a null self here isn't it? This could crash.

>+JetpackChild::GC(JSContext* cx, uintN argc, jsval *vp)

Do you want this in non-test builds too? Maybe #ifdef the impl?

>+JetpackChild::GCZeal(JSContext* cx, uintN argc, jsval *vp)

Hm, JS_SetGCZeal isn't always available... You need some ifdefs in the impl somewhere.

Comment 3

[Benjamin Smedberg]

Attachment: Add an .isRooted property, true by default, and .onInvalidate callback function, rev. 2

Made the changes, including removing the weird const/mutable stuff from Handle.h. cjones for the ipc/ipdl changes, bent for the code, and bwarner for the API.

I left onInvalidated interCaps because that's the common style in jetpack-land.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 461532 [details] [diff] [review]
Add an .isRooted property, true by default, and .onInvalidate callback function, rev. 2

Looks good!

Comment 5

[Brian Warner [:warner :bwarner]]

Comment on attachment 461532 [details] [diff] [review]
Add an .isRooted property, true by default, and .onInvalidate callback function, rev. 2

Atul is writing the jetpack-SDK -side code for this, so I think he's in a better position to review it than me.

Comment 6

[Atul Varma [:atul]]

Comment on attachment 461532 [details] [diff] [review]
Add an .isRooted property, true by default, and .onInvalidate callback function, rev. 2

This is perfect, just the kind of functionality we need for implementing our APIs.

Thanks also for the gc()/gczeal() globals, they'll be useful for our own unit tests too.

Comment 7

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/383c1d348414

Comment 8

[Atul Varma [:atul]]

I've documented these changes here:

  https://developer.mozilla.org/en/Jetpack_Processes