Note: There are a few cases of duplicates in user autocompletion which are being worked on.

xpconnect wrappedJS should implement nsISupportsWeakReference

VERIFIED FIXED

Status

()

Core
XPConnect
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: John Bandhauer, Assigned: John Bandhauer)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

17 years ago
With some changes to the JS engine we can track lifetime of interesting
JSObjects. With some bigger changes to wrappedJS wrappers we can support
nsISupportsWeakReference.

See news://news.mozilla.org/3A720CD4.E076E7%40netscape.com (and the followups)
for discussion on the Js engine changes.
(Assignee)

Comment 1

17 years ago
Created attachment 23770 [details] [diff] [review]
First part of the fix
(Assignee)

Comment 2

17 years ago
I attached a patch that implements the js/src and dom/src/base parts of the fix.
Some of the xpconnect part is in place. I also added support to the
nsSupportsWeakReference mixin class for finding out if the given object has a
weak reference.

With positive reviews I'd check this independent patch in.
+
+    if (!(gcflags & GC_LAST_CONTEXT) && rt->gcCallback)
+        (void) rt->gcCallback(cx, JSGC_MARK_END);
 
No need to test GC_LAST_CONTEXT -- the test for JSGC_BEGIN is necessary because
the callback's return value can cause the GC not to run (by returning early),
and the last GC must always run.

Looks good otherwise, r/sr=brendan@mozilla.org.

/be
(Assignee)

Comment 4

17 years ago
brendan: you test GC_LAST_CONTEXT for both the JSGC_BEGIN and JSGC_END events.
This would be the only event going to the gcCallback in the last JSContext case.
I was thinking that the context might be considered to be in a questionable
state at this point. Maybe not. I note that finalizers get called as usual.
Nevertheless, it is fine with me to go ahead and remove the flag check.

Thanks.

rogerl or shaver: r= ?
Status: NEW → ASSIGNED
I test for BEGIN and END so as not to have an END without a BEGIN (can't call
BEGIN for the last context and respect a false return, so I decided not to call
it -- hmm, could call it but ignore the r.v.).

This is crucial: when the last context in the runtime is going away, do we need
to call your MARK_END callback in order to free things?  If so, then it seems
better to invoke all the callbacks (BEGIN, MARK_END, END) even for the last
context, and simply to ignore BEGIN's return code.  Care to work that into a patch?

/be

Comment 6

17 years ago
adding myself to the CC because I have concerns about this. 

There are mechanisms (specifically the nsIObserverService) that have a policy of
holding weak references where possible, and holding strong refs the rest of the
time.

If this bug is a blanket fix for all XPConnected objects from JS, I think we may
end up with certain objects silently disappearing out from under us.

An example: say I have an observer:
obs.prototype = {
    Observe: function(a,b,c) { dump("Observed.\"); }
}

and in a function I say:
function addObserver() {
     var observer = new obs;
     observerService.addObserver("topic", obs);
}

In the current world, the JS object does not implement a weak reference, so the
observer service will hold a strong ref to it... 

if this bug is fixed, when the function exits, won't this object get cleaned up
because there are no references to it?
(Assignee)

Comment 7

17 years ago
Good point Alec. I was marching ahead blindly.

In this situation C++ observers control whether they will be held with a weak or
strong reference by either implementing or not implementing
nsISupportsWeakReference (nsISWR for short). My proposal as written does not
give the JS objects the same sort of control. I can fix that by decreeing that
the wrapper will only claim to support nsISWR if the underlying JSObject claims
to support it. So if you want that support then add to your object something like:

    QueryInterface: function (iid) {
        if (iid.equals(Components.interfaces.nsISupportsWeakReference)) {
            return this;
        }
        throw Components.results.NS_ERROR_NO_INTERFACE;
    },

xpconnect won't call any methods of nsISupportsWeakReference; this is just a
mechaism for giving the JSObject some control.

Alternately, we could say that the pattern used by nsObserverService is broken.
The decision to use a weak or strong reference should not be based only on the
observer impementing a given interface or not (regardless of language). Instead
the caller to AddObserver should pass a param that explicitly specifies whether
the particular reference is to be weak or strong. I think this is a much more
reasonable level of control over the ugliness of refcounting. But, perhaps such
a change is not in the cards.

Thoughts?

Comment 8

17 years ago
Actually, I do agree that the decision about whether or not to hold strong/weak
refs should not simply be whether or not the observer itself implements it, but
it's darn convenient to support strong and weak refs in the observer service... 

I think architecturally  it makes the most sense to do as you suggest and add a
parameter to AddObserver. Given the amount of work this might entail, I would be
satisified with the QI solution you just gave. (but I'll gladly review anyone
fixing ALL the callers to the observer service :))
Or (backwards compatible, I hope) add a new addObserverRef method that takes a
STRONG or WEAK enum.

/be
(Assignee)

Comment 10

17 years ago
Created attachment 23913 [details] [diff] [review]
full proposed fix
(Assignee)

Comment 11

17 years ago
I attached a patch that includes the previous patch with changes suggested by 
brendan. And, it includes the xpconnect part of the support for weak references. 
I did as I suggested (and alecf agreed) and made wrappers only support weak 
references when the JSObject claims to implement the interface.

I checked in a test:
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/tests/js/old/xpctest_ob
server.js

This shows one observer that supports weak references and one that does not. The 
output I get looks like:

observer1 notified for: xpctest_observer_topic with: notification 1
observer2 notified for: xpctest_observer_topic with: notification 1
before 6777, after 5886, break 00000000
observer1 notified for: xpctest_observer_topic with: notification 2
observer2 notified for: xpctest_observer_topic with: notification 2
before 5985, after 5877, break 00000000
observer2 notified for: xpctest_observer_topic with: notification 3

If you look at the testcase you'll see that this indicates that for the 
weak-ref-supporting object, after the JSObject is collected the observer wrapper 
is not called. While, for the version that does not support the weak ref, a 
strong ref is held and the JSObject is not collected and is still called.

The debug instrumentation indicates that this is not leaking any wrappers or 
roots at shutdown and afaict everything else is running as before.

Looking for r= and sr=

Thanks,

John.
Remove the CHECK_REQUEST(cx) from JS_IsAboutToBeFinalized -- the GC need not run
in a request.

Style nit: either cuddle the chained ifs into one if using &&, or brace the
inner if (as a dangling-else warding sign) instead of

+    if (rt->gcCallback)
+        if (!rt->gcCallback(cx, JSGC_BEGIN) && !(gcflags & GC_LAST_CONTEXT))
             return;
-    }

School me: I don't see how mMapLock has anything to do with mWrappedJSMap.

/be
(Assignee)

Comment 13

17 years ago
> Remove the CHECK_REQUEST(cx) from JS_IsAboutToBeFinalized -- the GC need not > 
> run in a request.

Done. I forgot what that cehck did and stupidly copied it.

> Style nit: either cuddle the chained ifs into one if using &&, or brace the
> inner if (as a dangling-else warding sign) instead of

I just added the braces. I started out with '&&' and decided the nested ifs was 
more readable.

> School me: I don't see how mMapLock has anything to do with mWrappedJSMap.

We always hold that lock when searching or modifying the mWrapperJSMap (usually 
via accessors to the map and lock from another class); e.g.
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappedjs.cpp#22
6

I fear the threadsafety is a bit imperfect in xpconnect in general. It has not 
been pushed hard enough. But, I think this case makes sense.
r/sr=brendan@mozilla.org -- who's your other reviewer?  I cc'd shaver and rogerl.

/be

Comment 15

17 years ago
r=rogerl on JS bits. i do not understand the XP bits so someone else should 
look. [is it typical to have function declarations in jsapi.h as well as 
other header files? I didn't find other cases in a very cursory scan]
rogerl: the jsapi.h prototype is for JS_IsAboutToBeFinalized, the jsgc.h one is
for its internal js_... counterpart, so that gc_find_flags need not be exported
(very big intra-library modularity deal! :-).

/be

Updated

17 years ago
Blocks: 59246

Comment 17

17 years ago
No wonder I didn't see anything going on with this bug... cc'ing.
(Assignee)

Comment 18

17 years ago
fix checked in. Thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 19

17 years ago
Marking Verified - 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.