nsXPCThreadJSContextStackImpl static mSingleton not getting cleared on destruction

RESOLVED FIXED

Status

()

Core
XPConnect
P3
normal
RESOLVED FIXED
19 years ago
10 months ago

People

(Reporter: rginda, Assigned: John Bandhauer)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

19 years ago
mSingleton was a static member of the GetSingleton method, and was not getting
cleared when the object was destroyed.  Second attempts to create the object
failed.

Index: xpcprivate.h
===================================================================
RCS file: /cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v
retrieving revision 1.19
diff -u -r1.19 xpcprivate.h
--- xpcprivate.h        1999/07/15 05:42:49     1.19
+++ xpcprivate.h        1999/07/31 08:17:48
@@ -860,6 +860,8 @@
     // hide ctor and dtor
     nsXPCThreadJSContextStackImpl();
     virtual ~nsXPCThreadJSContextStackImpl();
+    static nsXPCThreadJSContextStackImpl* mSingleton;
+
 };

Index: xpcthreadcontext.cpp
===================================================================
RCS file: /cvsroot/mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp,v
retrieving revision 1.2
diff -u -r1.2 xpcthreadcontext.cpp
--- xpcthreadcontext.cpp        1999/07/15 05:42:50     1.2
+++ xpcthreadcontext.cpp        1999/07/31 08:21:32
@@ -71,19 +71,24 @@
     NS_ADDREF_THIS();
 }

-nsXPCThreadJSContextStackImpl::~nsXPCThreadJSContextStackImpl() {}
+nsXPCThreadJSContextStackImpl::~nsXPCThreadJSContextStackImpl()
+{
+    mSingleton = nsnull;
+}

 static NS_DEFINE_IID(knsXPCThreadJSContextStackImplIID,
NS_IJSCONTEXTSTACK_IID);
 NS_IMPL_ISUPPORTS(nsXPCThreadJSContextStackImpl,
knsXPCThreadJSContextStackImplIID);

+nsXPCThreadJSContextStackImpl* nsXPCThreadJSContextStackImpl::mSingleton =
nsnull;
+
 //static
 nsXPCThreadJSContextStackImpl*
 nsXPCThreadJSContextStackImpl::GetSingleton()
 {
-    static nsXPCThreadJSContextStackImpl* singleton = NULL;
-    if(!singleton)
-        singleton = new nsXPCThreadJSContextStackImpl();
-    return singleton;
+    if(!mSingleton)
+        mSingleton =
+            new nsXPCThreadJSContextStackImpl();
+    return mSingleton;
 }

 /* readonly attribute PRInt32 Count; */

Updated

19 years ago
Assignee: dp → mccabe
Component: XPCOM → XPConnect
(Reporter)

Comment 1

19 years ago
This patch doesn't cover it.  I'm seeing failures while invoking
QueryInterface on the 'second' instance.

Program received signal SIGSEGV, Segmentation fault.
0x0 in ?? ()
Current language:  auto; currently c
(gdb) where
#0  0x0 in ?? ()
#1  0x401088e8 in XPTC_InvokeByIndex (that=0x8074490, methodIndex=0,
paramCount=2,
    params=0xbfffdd04) at xptcinvoke_unixish_x86.cpp:160
#2  0x402d1bef in nsXPCWrappedNativeClass::CallWrappedMethod (this=0x808a918,
cx=0x8062fa8,
    wrapper=0x81369e8, desc=0x80929e0, callMode=CALL_METHOD, argc=1,
argv=0x80814f8, vp=0xbfffdee8)
    at xpcwrappednativeclass.cpp:511
#3  0x402d34ef in WrappedNative_CallMethod (cx=0x8062fa8, obj=0x810b4d8, argc=1,
argv=0x80814f8,
    vp=0xbfffdee8) at xpcwrappednativejsops.cpp:129
#4  0x40043f03 in js_Invoke (cx=0x8062fa8, argc=1, flags=0) at jsinterp.c:654
...
#12 0x804aa85 in main (argc=0, argv=0xbffff858) at xpcshell.cpp:568
(Assignee)

Updated

19 years ago
Assignee: mccabe → jband
(Assignee)

Comment 2

19 years ago
reassigning to me.

I'm a little confused, but interested...

I'm not clear how or why you are accessing this particular class via JS. Its
interface is (intentionally) not marked as scriptable. Are you confusing this
with something else? What JS code is running when you hit the error you show?

This service holds an important state even if no one is using it at a given
point in time. I don't want to have the service manager release it on a whim.
So, I will add an extra addref in xpconnect to keep it from being deleted unless
someone convinces me otherwise.
(Reporter)

Comment 3

19 years ago
I've got class and interface enumeration working in JavaScript on my
local tree, and I've written a test script that creates an instance of
every class, and querys it for every interface.  After each successful
QueryInterface, it enumerates all properties on the new object, and then
releases the instance.  This has flushed quite a few basic create/destroy
without calling Init() and create/destroy/create bugs.
So... I'm not really using nsXPCThreadJSContextStackImpl for any purpose
other than smoke testing.
Maybe the real bug is that this class is even showing up in JavaScript.
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

19 years ago
I should have figured that this was what you were up to. Very cool!

When are you going to submit this stuff so that mccabe and I can get our grubby
hands on it?

Your patch in this instance would have been good except that, once created, I
*really* don't want the object to ever be deleted in the lifetime of the
process. The bug was that nsXPCThreadJSContextStackImpl::GetSingleton was not
doing an addref. When accessed only as a service this was only getting called
once and the addref from the ctor made it look ok. But, with multiple calls the
reference counts were getting unbalanced. Even with your patch callers ended up
calling Release on an object that has already deleted itself becasue its
refcount had not previously been pushed high enough. Very much my fault. Thanks.

I just checked in...

Index: xpcthreadcontext.cpp
===================================================================
RCS file: /cvsroot/mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp,v
retrieving revision 1.2
diff -r1.2 xpcthreadcontext.cpp
67a68,75
> /*
> * This object holds state that we don't want to lose!
> *
> * The plan is that once created this object never goes away. We do an
> * intentional extra addref at construction to keep it around even if no one
> * is using it.
> */
>
85a94,95
>     if(singleton)
>         NS_ADDREF(singleton);

... you should abandon your suggested patch and update from the trunck.

As to this *very* interesting issues of calling CreateInstance on every
registered component and QIing each for all known interfaces...

I suppose there is (now!) always the chance that someone might do this.

First off, this is not really a huge security issue because we can restrict it
using the nsIXPCSecurityManager interface in places (like content) where this
isn't to be allowed.

I had completely overlooked the fact that *all* xpcom objects are at least a
little bit scriptable because they all expose a scriptable QueryInterface via
inheritence. This is an important thing to remember.

If we wanted to we could invent 'nsISupportsNoScript'. People could declare it
as an interface's base in their idl files. The typelib system would show a
non-scriptable QI and we could jigger the C++ headers with something like:

#define nsISupportsNoScript nsISupports

Or use a typedef. Other language mappings might not be so happy!

This is probably moot. I think that the interesting 'damage' is already done in
cases where components get created using CreateInstance where they expect to
only be services, or vica versa. This remains a broken part of xpcom - objects
can't enforce the 'mode' of their creation and use. If they are inherently
singletons then their factories can always return the same instance, but this
really breaks the semantics of CreateInstance. This comes up now and then and
none of us has an answer that we are prepared to impose on the system.

Or bad things can happen if the object is a 'private' (though registered) object
and expects some special handshake that is never given after creation when
created by 'outsiders'.

I don't think that we can do anything about all this except tell people to not
write bad components. If you register a factory for a thing with the component
manager, then you are in the fish bowl (or fish bowel on a bad day:)

The thing to do is exactly what rginda@ix.netcom.com is doing -- exercise the
heck out of things and see what breaks then fix those things.

Thanks! (now show us the code! :)

John.

P.S. rginda, I'm not marking this bug fixed. Please check that the crash is gone
when you can and mark it 'fixed' if you think that appropriate. Thanks.
(Reporter)

Comment 5

19 years ago
What if CreateInstance were to return NS_ERROR_NO_PUBLIC_CREATE (or some such
error) for classes that are only meant to be created by 'friends.'  Components
'in the know' would create the class using a static NS_NewSekretObject
(nsISekret **retval) constructor.  The nsISekret object could still be used as
a retval from some other publicly creatable object, and would be able to enjoy
all the benifits of being scriptable, without the creation issues.

From my *gasp* VB experience, I recall setting a boolean property described as
"Publicly Creatable" on classes.  So I assume MS-COM already has some way of
doing this.  I will create a small VB test case using this property and see of
I can't get some more details.

BTW, I will retest this fix when I get my tree back in order :)
(Reporter)

Updated

19 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED

Updated

19 years ago
QA Contact: beppe → cbegle

Comment 6

19 years ago
setting QA contact to Christine Begle -- here ya go
You need to log in before you can comment on or make changes to this bug.