Closed Bug 67258 Opened 24 years ago Closed 24 years ago

Components.results sometimes has no properties

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: jband_mozilla, Assigned: jband_mozilla)

Details

Attachments

(2 files)

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!
r/sr=brendan@mozilla.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
Sure, good patterns should spread, replacing less good ones.
r/sr=brendan@mozilla.org.

/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
Closed: 24 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.

Attachment

General

Creator:
Created:
Updated:
Size: