317304 - Implement weak references for JS

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Alex Fritze]

Patch coming up.

Comment 1

[Alex Fritze]

Attachment: first attempt

This patch implements two new methods for weak reference support in JS: Components.utils.getWeakRef() and Components.utils.forceGC().
- forceGC() forces a JS garbage collection cycle.
- getWeakRef() works as follows:
> var a = { foo: 1, bar: "baz" };
[object Object]
> var b = Components.utils.getWeakRef(a);
[xpconnect wrapped xpcIJSWeakRef ...]
> b.get();
[object Object]
> b.get().foo;
1
> Components.utils.forceGC();

> b.get();
[object Object]
> a = null;
null
> b.get();
[object Object]
> Components.utils.forceGC();

>b.get();
null

There is probably a more direct way of implementing this in the JS engine. Any hints welcome.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 203809 [details] [diff] [review]
first attempt

>+    /*
>+     * To be called from JS only.
>+     *
>+     * Return a weak reference for the given JS object.
>+     */
>+    xpcIJSWeakRef getWeakRef(/* in JSObject obj */);

I'd prefer that we spell out "Reference" here in the type and method names.

>+    /*
>+     * To be called from JS only.
>+     *
>+     * Force an immediate garbage collection cycle.
>+     */
>+    void forceGC();
> };

Not 100% sold on this, but we sure get a lot of requests for it, so let's
go for it.

>+ * The Original Code is Mozilla code.
>+ *
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2004
>+ * the Initial Developer. All Rights Reserved.

Missing some part of the boiler plate there, and it should say you
rather than NSCP.

>+/* xpcIJSWeakRef getWeakRef (); */
>+NS_IMETHODIMP
>+nsXPCComponents_Utils::GetWeakRef(xpcIJSWeakRef **_retval)
>+{
>+    nsRefPtr<xpcJSWeakRef> ref(new xpcJSWeakRef());
>+    if (!ref) return NS_ERROR_FAILURE;

NS_ERROR_OUT_OF_MEMORY, and please put the then-clause on its
own line.

>+    // get the xpconnect native call context
>+    nsCOMPtr<nsIXPCNativeCallContext> cc;
>+    xpc->GetCurrentNativeCallContext(getter_AddRefs(cc));
>+    if (!cc)
>+        return NS_ERROR_FAILURE;

Might we get a more informative error code from GCNCC here,
which we could propagate?  You do so in nsJSWeakRef::Get!

>+ * The Original Code is Mozilla Communicator client code, released
>+ * March 31, 1998.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998
>+ * the Initial Developer. All Rights Reserved.

More boilerplate oops here. (We really need some elisp macros for
doing this correctly, ahem.)

>+    nsRefPtr<nsXPCWrappedJS> wrapped;
>+    nsXPCWrappedJS::GetNewOrUsed(ccx,
>+                                 obj,
>+                                 NS_GET_IID(nsISupports),
>+                                 nsnull,
>+                                 getter_AddRefs(wrapped));
>+    if (!wrapped) {
>+        NS_ERROR("can't get nsISupportsWeakReference wrapper for obj");
>+        return NS_ERROR_FAILURE;
>+    }

Error propagation here?  If null is unambiguous, it means OOM, so we should
use that unless we can get a real rv from GNOU.

>+    return ((nsISupportsWeakReference*)wrapped)->GetWeakReference(getter_AddRefs(mWrappedJSObject));

Excess parenthesization.  I know it seems that there is a limitless supply, but
conservation is nonetheless a virtue.

r+sr=shaver with those nits picked.

Comment 3

[Jonathan Watt [:jwatt]]

Looks like Alex had too much on his plate and this fell through the cracks. I've updated the patch to address the review comments and checked it in. I also added a unit test for added bonus points.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch as checked in

Comment 5

[Jesse Ruderman]

Right after this patch was checked in, Tp spiked on Linux, and then the Linux machine was swapped out.  I think this patch should be backed out for a few hours and then landed again to make sure it doesn't regress Tp on Linux.

Comment 6

[Jonathan Watt [:jwatt]]

The patch adds a couple of methods to Components.utils which nobody currently calls. I can't see how it could possibly cause a Tp spike, but if you _really_ want I can back it out tomorrow.