The nsbeta2+ bugs that vishy and pavlov own should depend on this bug, so it can inherit nsbeta2+ goodness. /be
Testing this out with a spanking clean build; I ran into some startup trouble testing it in my current build. Will check in thereafter.
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Thanks Mike, commercial build AIM dependencies will go in today too. Vishy
Very curious. Dave Baron and Bruce both congratulated me on getting rid of a 5k/page leak just after I checked this in; looks like a Components object was leaking. I have no idea why this fixed it, or if the accusations are substantiated. CCing jband to celebrate.
Hm. Actually, CCing shaver too, in case the JS Component loader contributed to hanging on to the global. And dbaron, in case we need him for details.
Crash-on-shutdown, seems like it could be a consequence of this fix. Stack from Waterson: js_LockRuntime(JSRuntime * 0xdddddddd) line 622 + 3 bytes js_DestroyContext(JSContext * 0x010f09c0, int 2) line 140 + 9 bytes JS_DestroyContext(JSContext * 0x010f09c0) line 787 + 11 bytes xpcPerThreadData::Cleanup() line 224 + 13 bytes xpcPerThreadData::CleanupAllThreads() line 403 + 8 bytes nsXPConnect::~nsXPConnect() line 197 nsXPConnect::`scalar deleting destructor'(unsigned int 1) + 15 bytes nsXPConnect::Release(nsXPConnect * const 0x010e51e0) line 42 + 129 bytes nsXPConnect::ReleaseXPConnectSingleton() line 279 + 12 bytes xpcModuleDtor(nsIModule * 0x00cf1030) line 64 nsGenericModule::Shutdown() line 143 + 10 bytes nsGenericModule::~nsGenericModule() line 123 nsGenericModule::`scalar deleting destructor'(unsigned int 1) + 15 bytes nsGenericModule::Release(nsGenericModule * const 0x00cf1030) line 125 + 127 bytes nsDll::Shutdown() line 443 + 18 bytes nsFreeLibrary(nsDll * 0x00c7b9d0, nsIServiceManager * 0x00000000, int 3) line 380 nsFreeLibraryEnum(nsHashKey * 0x00c7b710, void * 0x00c7b9d0, void * 0x0012fe14) line 428 + 64 bytes _hashEnumerate(PLHashEntry * 0x00c7b6d0, int 0, void * 0x0012fdfc) line 99 + 26 bytes PL_HashTableEnumerateEntries(PLHashTable * 0x00c67530, int (PLHashEntry *, int, void *)* 0x10019c70 _hashEnumerate(PLHashEntry *, int, void *), void * 0x0012fdfc) line 413 + 15 bytes nsHashtable::Enumerate(int (nsHashKey *, void *, void *)* 0x10060540 nsFreeLibraryEnum(nsHashKey *, void *, void *), void * 0x0012fe14) line 237 + 20 bytes nsNativeComponentLoader::UnloadAll(nsNativeComponentLoader * const 0x00c66220, int 3) line 986 nsComponentManagerImpl::UnloadLibraries(nsIServiceManager * 0x00000000, int 3) line 1910 + 22 bytes nsComponentManagerImpl::Shutdown() line 307 NS_ShutdownXPCOM(nsIServiceManager * 0x00000000) line 622 + 11 bytes main(int 1, char * * 0x00c64720) line 1193 + 8 bytes mainCRTStartup() line 338 + 17 bytes KERNEL32! 77f1b304()
Fixed the problem by adding an mSafeContextIsFromSetter boolean to the per-thread data, setting it when the setter is called, and consulting it on cleanup to decide whether the context is ours to delete. Thereby muddying the ownership model. This seems like more evidence that this is a hack! Is there: a) Another way to solve the original problem? b) A way to pass in a *fresh* DOM-chocolate context, so we can at least keep the ownership model uniform?
Duh! Sorry, mccabe -- shoulda seen that coming (JSContexts are manually created and destroyed). I don't think the flag is a hack; it's a fine boolean that helps us conserve "chocolate" (DOM-like) safe contexts. Otherwise, we'd end up with a modular form of the first solution I hacked with pavlov: call NS_NewScriptGlobalObject and pass its result to NS_CreateScriptEnvironment, all to make something just like the hidden window's context/global. I say we should avoid such silly duplication of this roach-motel of a context, and just use the hidden window's context. Since it is not ref-counted, let's cope with your "preset, please don't destroy" flag, and stipulate that the preset context's lifetime exceeds the XPConnect refs to it. Maybe it is a hack. But so is duplicating the hidden window's context, even if done from DOM primitives, without reference to the hidden window. I'd rather save the RAM. Comments? /be
This touches on a question I've always had... why are we so careful about creating new JSContexts? It seems like context management has been a big headache in the history of the product. Just how expensive are they? (I know that magic imputed to the context private is another reason we need to track them carefully, so creation-expense isn't the whole story. But...) In this case, is the cost of a singleton context worth the engineering cost of creating a mixed ownership model? One answer I was looking for to a) is - is there some completely different way to make the DOM-to-xpc conversion succeed? Well, what we have works! :) Hm. I just realized - we probably leak a context now in the case when there was already a safe context there when we set the new one. The "SetSafeJSContext called too late!" assert was firing whenever we registered JS Components.
JSContexts are cheap enough, but nsJSContexts are less so, and don't forget the GlobalWindowImpl, which IIRC drags in a whole set of standard classes, as well as DOM classes and XPConnect classes. That lot is not cheap. /be
mccabe, we need to fix that !mSaveJSContext leak. New bug, or this one? /be
So we need to destory the context leaked when the context setter replaces one that has already been created. We can do this on the spot, in the setter, or tuck it away and wait until it would have been destroyed before. If we destroy it on the spot, I'm worried about js components that may have already been created under that context. I guess this depends on whether xpconnect has some other reference to the context, but in general - is it OK to execute functions from a js object in a new context when js from that object has already run in an existing context?
Argh, we need an open bug to thrash this out. mccabe, can you reopen this or, if you prefer, open a new one and cc: me on it? Need shaver too, to tell us about whether a JS component might come to depend on the standard class prototypes of its loading context's global object. Apart from onload-type scripted code that runs and calls constructors, there are both RegExp and function "literals" that the compiler creates, prototyped by the compile-time context's scope-chain's last object's standard class prototypes. More in the (new or reopened) bug. /be
Reopening bug. A conservative approach (that costs us the size of a safe context for the lifetime of the application) is to save off the already-created safe context into a member, and destroy it at cleanup time, giving it the same lifetime as before.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I fixed instanceof (well, fixed may be too strong) to cope with multiple class prototypes, one used at compile time for literals (regexps, functions) and the other at runtime, by having it compare native method or underlying JSScript*, and if identical for two constructors (one named by the right operand, the other found somewhere in the left's prototype chain, and named 'constructor'), declare instance-of. So maybe we don't care about keeping the mozJSComponentLoader global and its standard classes around. What other dependency could there be? Shaver, are there global variables bound at load time in the top-level object, which need to be found later? /be
CCing shaver. Shaver, could you peek at past comments?
I'm unclear where we are now on this issue. It sounds like we've landed a fix, resolved some jscontext issues and still have open questions. Brendan: can you affix a status update, please?
Whiteboard: [need info]
This ball is in mccabe's court. Mike, did you find out who was initializing the safe context for the main thread earlier than CreateHiddenWindow? If it is in fact the JS component loader, we should consider reordering hidden window creation. /be
I haven't heard any resolution of the safety of destroying the possibly-in-use js components context... and I also think that case only happens when new js components are registered, so I'm going to go ahead with the conservative solution. I'll save off any already-created js component context when setting a DOM context, and destroy the saved-off context at the usual time in the destructor.
Mike, is this fixed enough that it should no longer be a beta2 stopper?
Whiteboard: [need info] → [NEED INFO]
It's now merely a leak.
Whiteboard: [NEED INFO] → [nsbeta2-] [NEED INFO]
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago → 18 years ago
Resolution: --- → FIXED
Added a check to the previous patch to make sure that SetSafeJSContext can't set the safe context to null, ensuring that the XPC-specific safe context is only created exactly once, and keeping state consistent. Fix checked in, closing.
You need to log in before you can comment on or make changes to this bug.