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

RESOLVED FIXED

Status

()

Core
XPConnect
P3
normal
RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: Mike McCabe, Assigned: Mike McCabe)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta2-] [NEED INFO])

Attachments

(3 attachments)

(Assignee)

Description

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

Comment 1

18 years ago
Created attachment 9064 [details] [diff] [review]
add cx setter, courtesy Brendan
The nsbeta2+ bugs that vishy and pavlov own should depend on this bug, so it can 
inherit nsbeta2+ goodness.

/be
Keywords: nsbeta2
(Assignee)

Comment 3

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

Comment 4

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

Updated

18 years ago
Blocks: 27867
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

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

Comment 11

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

Comment 14

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

Comment 16

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

Comment 18

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

Comment 19

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

Comment 21

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

18 years ago
Mike, is this fixed enough that it should no longer be a beta2 stopper?
Whiteboard: [need info] → [NEED INFO]
(Assignee)

Comment 23

18 years ago
It's now merely a leak.

Comment 24

18 years ago
[nsbeta2-]
Whiteboard: [NEED INFO] → [nsbeta2-] [NEED INFO]
(Assignee)

Comment 25

18 years ago
Created attachment 9579 [details] [diff] [review]
Safe fix for JSContext leak
(Assignee)

Comment 26

18 years ago
Created attachment 9580 [details] [diff] [review]
Another try.
(Assignee)

Updated

18 years ago
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 27

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