67258 - Components.results sometimes has no properties

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[John Bandhauer]

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.

Comment 1

[John Bandhauer]

Attachment: proposed fix for r= and sr=

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

I pulled the tip version of xpccomponents.cpp, applied this patch, and the
problem seems fixed.  Thanks John!

Comment 3

[Brendan Eich [:brendan]]

r/sr=[e-mail], but dmose's code is still twisted :-)

/be

Comment 4

[Peter "jag" Annema]

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?

Comment 5

[John Bandhauer]

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.

Comment 6

[John Bandhauer]

Attachment: patch that applies the same pattern to nsJSIID too

Comment 7

[Brendan Eich [:brendan]]

Sure, good patterns should spread, replacing less good ones.
r/sr=[e-mail].

/be

Comment 8

[Peter "jag" Annema]

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.

Comment 9

[John Bandhauer]

fix checked in

Comment 10

[Phil Schwartau]

Marking Verified -