Components.results sometimes has no properties

VERIFIED FIXED

Status

()

VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: jband_mozilla, Assigned: jband_mozilla)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

18 years ago
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.
(Assignee)

Comment 1

18 years ago
Created attachment 24042 [details] [diff] [review]
proposed fix for r= and sr=

Comment 2

18 years ago
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

Comment 4

18 years ago
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?
(Assignee)

Comment 5

18 years ago
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
(Assignee)

Comment 6

18 years ago
Created attachment 24111 [details] [diff] [review]
patch that applies the same pattern to nsJSIID too
Sure, good patterns should spread, replacing less good ones.
r/sr=brendan@mozilla.org.

/be

Comment 8

18 years ago
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.
(Assignee)

Comment 9

18 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 10

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