nsXPCComponents_Results stores state about the wrapper around itself. That state can be wrong if the wrapper is gc'd and a new wrapper created in its place. dmose tripped over this bug in his twisted ldap work. Commented patch to follow.
I pulled the tip version of xpccomponents.cpp, applied this patch, and the problem seems fixed. Thanks John!
firstname.lastname@example.org, but dmose's code is still twisted :-) /be
http://lxr.mozilla.org/seamonkey/search?string=mCacheFilled nsJSIID seems to use the same pattern, could it (in any way) be suffering from the same problem as nsXPCComponents_Results?
jag: It turns out not to matter for the nsJSIID objects. They represent the "nsIFoo" in "Components.interfaces.nsIFoo" and the properties on them that we are adding represent the 'const' members declared in idl. The nsJSIID objects themselves are rooted because their wrappers' JSObjects are added as properties of the 'interfaces' JSObject. In the case where the 'interfaces' JSObject itself is collected then we would also end up lazily recreating the nsJSIID objects anyway if and when a new 'interfaces' object where created. So, we never hit a case where the properties of the nsJSIID JSObject go away without the nsJSIID C++ object itself also going away. An xpcshell test case for both of these is: print(Components.results.NS_OK); print(Components.interfaces.nsICategoryManager.FALLBACK); gc(); print(Components.results.NS_OK); print(Components.interfaces.nsICategoryManager.FALLBACK); This should print something like: 0 1 before 6309, after 5598, break 00000000 0 1 Without the patch I get: 0 1 before 6309, after 5598, break 00000000 undefined 1 Still, it is perfectly reasonable to apply the same (obj != mObj) test in nsJSIID. There is certainly nothing wrong with this safer pattern and it might save us if the system changes a bit in the future. I have to admit that I failed to think of this case and only discovered that it was not a problem by testing it and *then* thinking through why. Thanks! John.
Status: NEW → ASSIGNED
Created attachment 24111 [details] [diff] [review] patch that applies the same pattern to nsJSIID too
Sure, good patterns should spread, replacing less good ones. email@example.com. /be
Well, I understand the changes you've made, it seems you've correctly changed all related code, and I'll just take your word for it that |obj| is the right one :-) r=jag, fwiw.
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.