Closed
Bug 66950
Opened 24 years ago
Closed 24 years ago
xpconnect wrappedJS should implement nsISupportsWeakReference
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
VERIFIED
FIXED
People
(Reporter: jband_mozilla, Assigned: jband_mozilla)
References
Details
Attachments
(2 files)
7.83 KB,
patch
|
Details | Diff | Splinter Review | |
19.76 KB,
patch
|
Details | Diff | Splinter Review |
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•24 years ago
|
||
Assignee | ||
Comment 2•24 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.
Comment 3•24 years ago
|
||
+ + 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•24 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
Comment 5•24 years ago
|
||
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•24 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•24 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•24 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 :))
Comment 9•24 years ago
|
||
Or (backwards compatible, I hope) add a new addObserverRef method that takes a STRONG or WEAK enum. /be
Assignee | ||
Comment 10•24 years ago
|
||
Assignee | ||
Comment 11•24 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.
Comment 12•24 years ago
|
||
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•24 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.
Comment 14•24 years ago
|
||
r/sr=brendan@mozilla.org -- who's your other reviewer? I cc'd shaver and rogerl. /be
Comment 15•24 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]
Comment 16•24 years ago
|
||
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
Comment 17•24 years ago
|
||
No wonder I didn't see anything going on with this bug... cc'ing.
Assignee | ||
Comment 18•24 years ago
|
||
fix checked in. Thanks!
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•