40406 - xpconnect needs DOM-ish default context to allow making windows from js components

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[Mike McCabe]

Vishy has a JS component that he wants to make a UI window from.  He gets it
through XPConnect, but can't convert the window itself to an xpconnected object.

Vishy:

var windowManager =
Components.classes['component://netscape/rdf/datasource?name=window-mediator'].getService(); 
var windowManagerInterface = windowManager.QueryInterface(
Components.interfaces.nsIWindowMediator); 
var topWindow = windowManagerInterface.getMostRecentWindow(null); 
dump("FOUR \n"); 

On doing the statement before dump(FOUR), I get 

************************************************************ 
** NOTE: This report will only be printed in DEBUG builds.** 
* Call to xpconnect wrapped JSObject produced this error:  * 
[Exception... "Can not get JavaScript object for DOM object arg 1
[nsIWindowMediator.getMostRecentWindow]"  nsresult: "0x8057002a
(NS_ERROR_XPC_CANT_GET_JSOBJECT_OF_DOM_OBJECT)"  location: "JS frame ::
D:\may18\ns\dist\WIN32_D.OBJ\bin\components\nsJSAimChatRendezvous.js ::
anonymous :: line 38"  data: no] 
************************************************************ 

Coincidentally, Pavlov was doing almost the same thing and ran into the same
problem.

Brendan-investigation showed that the conversion needs a magic JSContext with
special DOM innards, but in the js component case was using an internal default
minimal context.

Fix seems to be to give XPConnect a setter for the default context to use for a
given thread; patch in hand to do this.

Comment 1

[Mike McCabe]

Attachment: add cx setter, courtesy Brendan

Comment 2

[Brendan Eich [:brendan]]

The nsbeta2+ bugs that vishy and pavlov own should depend on this bug, so it can 
inherit nsbeta2+ goodness.

/be

Comment 3

[Mike McCabe]

Testing this out with a spanking clean build; I ran into some startup trouble
testing it in my current build.  Will check in thereafter.

Comment 4

[Mike McCabe]

Fix checked in.

Comment 5

[Viswanath Ramachandran]

Thanks Mike, commercial build AIM dependencies will go in today too. Vishy

Comment 6

[Mike McCabe]

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.

Comment 7

[Mike McCabe]

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.

Comment 8

[Mike McCabe]

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()

Comment 9

[Mike McCabe]

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?

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Mike McCabe]

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.

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Brendan Eich [:brendan]]

mccabe, we need to fix that !mSaveJSContext leak.  New bug, or this one?

/be

Comment 14

[Mike McCabe]

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?

Comment 15

[Brendan Eich [:brendan]]

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

Comment 16

[Mike McCabe]

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.

Comment 17

[Brendan Eich [:brendan]]

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

Comment 18

[Mike McCabe]

CCing shaver.  Shaver, could you peek at past comments?

Comment 19

[rickg]

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?

Comment 20

[Brendan Eich [:brendan]]

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

Comment 21

[Mike McCabe]

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.

Comment 22

[leger]

Mike, is this fixed enough that it should no longer be a beta2 stopper?

Comment 23

[Mike McCabe]

It's now merely a leak.

Comment 24

[Peter Trudelle]

[nsbeta2-]

Comment 25

[Mike McCabe]

Attachment: Safe fix for JSContext leak

Comment 26

[Mike McCabe]

Attachment: Another try.

Comment 27

[Mike McCabe]

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.