52591 - JS Loader needs to call nsIXPConnect::InitClasses for all globals

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P3)

Description

[John Bandhauer]

Shaver's JS Loader uses a super global and only calls nsIXPConnect::InitClasses 
for the super global - not for each global object. He talked me into it at the 
time, but it turns out this causes problems.

nsIXPConnect::InitClasses creates a Components object and attaches it as a 
property of the object passed in. It also establishes a 'scope' used to find or 
make new wrappers.

When xpconnect needs to find or create a wrapper for a given native object a 
JSObject is passed in to use for finding the correct scope. The steps are to get 
the ultimate ancestor of that JSObject, do a lookup for "Components" on that 
ancestor, and then ask the native Components object for its native scope 
opbject. New wrappers are constructed using that ancestor as the wrapper's 
JSObject's parent.

So... We were ending up with the super global being the parent of *all* the 
wrappers' JSObjects for all components loaded by the JS loader. 

The thing this breaks is rginda's subscript loader which needs to find an object 
which it will compile and evaluate scripts against. It defaults to using the 
ultimate ancestor of the wrapper's JSObject - which is the super global! This is 
bad since his loaded scripts then do not share the same scope as the code that 
asked them to be loaded. *and* the scripts loadef from different JS components 
can write on each other's stuff.

My proposed fix is to follow the existing...

    /* fix the scope and prototype links for cloning and scope containment */
    if (!JS_SetPrototype(mContext, obj, mSuperGlobal) ||
        !JS_SetParent(mContext, obj, nsnull))
        return nsnull;

...with the new...

    rv = xpc->InitClasses(mContext, obj);
    if (NS_FAILED(rv))
        return nsnull;

This will give us a Components object for each global. At some future point we 
may optimize the Components object to share its bulk via refactoring and the use 
of __proto__. For now we need to get this right. Sharing one Components object 
for all JS Components is broken.

thoughts?

Comment 1

[Brendan Eich [:brendan]]

Could you not use a resolve hook to lazily define Components in each global?

Otherwise, worse is better, but leave an XXX comment in the patch.

/be

Comment 2

[Mike Shaver (:shaver emeritus)]

Lazy definition seems to not be a win, since I predict that the first use of any
component's script will involve a Components reference in the _very_ early
going.

I think rginda's ultimate-ancestor test should use the scope link, though.  Why
does it not?  Where can I see this test?

Comment 3

[John Bandhauer]

This is exactly why I was not suggesting lazy. I do think we can get to the 
point where the only thing specialized for each global is a plain JSObject whose 
private tells xpconnect what it needs to know and who does everything 
interesting via proto delegation to the shared object with the arrays of 
interfaces and classes and all that jazz.

grinda's stuff *does* follow the parent chain. But, by default the near-end of 
this chain is the JSObject of the wrapper and for all wrappers in the current 
setup the far-end of the chain is the super global *because* there is only one 
Components object for all JS components.

Ask rginda if you want to see his source.

Comment 4

[Robert Ginda]

My ancestor test looks like this (error checks omitted)...

    nsCOMPtr<nsIXPCNativeCallContext> cc;
    rv = xpc->GetCurrentNativeCallContext(getter_AddRefs(cc));

    nsCOMPtr<nsIXPConnectWrappedNative> wn;
    rv = cc->GetCalleeWrapper (getter_AddRefs(wn));

    rv = wn->GetJSObject (&target_obj);

    JSObject *maybe_glob = JS_GetParent (cx, target_obj);
    while (maybe_glob != nsnull)
    {
        target_obj = maybe_glob;
        maybe_glob = JS_GetParent (cx, maybe_glob);
    }

    /* target_obj holds global object */

I'll attach a tarball of everything needed to reproduce the situation if you
want to see it.

Comment 5

[John Bandhauer]

I checked in my fixes for this. The rest of the threadsafety issues here fall 
into the lap of the component manager.

Comment 6

[Phil Schwartau]

Marking Verified -