Last Comment Bug 578821 - Rooting of jetpack handles is too simplistic
: Rooting of jetpack handles is too simplistic
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: Benjamin Smedberg [:bsmedberg]
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-14 14:53 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2010-09-20 15:06 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta4+


Attachments
Add an .isRooted property, true by default, and .onInvalidate callback function, rev. 1 (17.67 KB, patch)
2010-07-27 13:22 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Splinter Review
Add an .isRooted property, true by default, and .onInvalidate callback function, rev. 2 (19.11 KB, patch)
2010-07-30 06:51 PDT, Benjamin Smedberg [:bsmedberg]
bent.mozilla: review+
cjones.bugs: review+
avarma: review+
Details | Diff | Splinter Review

Description Benjamin Smedberg [:bsmedberg] 2010-07-14 14:53:42 PDT
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 [:bsmedberg] 2010-07-27 13:22:43 PDT
Created attachment 460628 [details] [diff] [review]
Add an .isRooted property, true by default, and .onInvalidate callback function, rev. 1
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-07-29 14:12:41 PDT
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 [:bsmedberg] 2010-07-30 06:51:47 PDT
Created attachment 461532 [details] [diff] [review]
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!) 2010-08-09 12:18:25 PDT
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] 2010-08-09 16:45:14 PDT
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] 2010-08-09 17:02:39 PDT
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 [:bsmedberg] 2010-08-10 08:25:25 PDT
http://hg.mozilla.org/mozilla-central/rev/383c1d348414
Comment 8 Atul Varma [:atul] 2010-09-20 15:06:26 PDT
I've documented these changes here:

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

Note You need to log in before you can comment on or make changes to this bug.