291439 - crash [@ js_SetClassPrototype] because proto isn't rooted in JS_InitClass across call to js_DefineFunction

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[timeless]

my tree isn't entirely current, but i'm fairly certain this isn't fixed yet. i
can update early next week.

I'm abusing too_much_gc by restricting the jsenvironment to about 2k.

death is very simple:
    proto = js_NewObject(cx, clasp, parent_proto, obj);
/* creates a happy proto with a map != 0; */
    if (!proto)
        return NULL;

    if (!constructor) {
    } else {
        /* Define the constructor function in obj's scope. */
        fun = js_DefineFunction(cx, obj, atom, constructor, nargs, 0);
/* destroyed the proto, presumably because it wasn't rooted */
        named = (fun != NULL);

+	proto->map	0x00000000 {nrefs=??? ops=??? nslots=??? ...}	JSObjectMap *
+	fun	0x00baf020 {nrefs=0x00000001 object=0x00ba5bf0 {map=0x00bb1198
{nrefs=0x00000001 ops=0x100fd560 _js_ObjectOps nslots=0x00000005 ...}
slots=0x00bac42c } u={native=0x10042550 Function(JSContext *, JSObject *,
unsigned int, long *, long *) script=0x10042550 Function(JSContext *, JSObject
*, unsigned int, long *, long *) } ...}	JSFunction *

the call to:
>	js3250.dll!js_SetClassPrototype(JSContext * cx=0x00bab178, JSObject *
ctor=0x00ba5bf0, JSObject * proto=0x00ba5be8, unsigned int attrs=0x00000006) 
Line 3660	C
crashes at:
    return OBJ_DEFINE_PROPERTY(cx, proto,
                               ATOM_TO_JSID(cx->runtime->atomState
                                            .constructorAtom),
                               OBJECT_TO_JSVAL(ctor),
                               JS_PropertyStub, JS_PropertyStub,
                               0, NULL);
because:
+	proto->map	0x00000000 {nrefs=??? ops=??? nslots=??? ...}	JSObjectMap *
and obj_define_property wants to use (proto)->map->ops->defineProperty which
isn't very reachable

>	js3250.dll!JS_InitClass(JSContext * cx=0x00bab178, JSObject * obj=0x00ba5ba8,
JSObject * parent_proto=0x00000000, JSClass * clasp=0x100ee810, int (JSContext
*, JSObject *, unsigned int, long *, long *)* constructor=0x10042550, unsigned
int nargs=0x00000001, JSPropertySpec * ps=0x100ee4a0, JSFunctionSpec *
fs=0x100ef2e4, JSPropertySpec * static_ps=0x00000000, JSFunctionSpec *
static_fs=0x00000000)  Line 1969	C
 	js3250.dll!js_InitFunctionClass(JSContext * cx=0x00bab178, JSObject *
obj=0x00ba5ba8)  Line 1864 + 0x29	C
 	js3250.dll!InitFunctionAndObjectClasses(JSContext * cx=0x00bab178, JSObject *
obj=0x00ba5ba8)  Line 1135 + 0xd	C
 	js3250.dll!JS_ResolveStandardClass(JSContext * cx=0x00bab178, JSObject *
obj=0x00ba5ba8, long id=0x00ba58a4, int * resolved=0x0012f448)  Line 1422 + 0xb	C
 	xpc3250.dll!SafeGlobalResolve(JSContext * cx=0x00bab178, JSObject *
obj=0x00ba5ba8, long id=0x00ba58a4)  Line 126 + 0x16	C++
 	js3250.dll!js_LookupPropertyWithFlags(JSContext * cx=0x00bab178, JSObject *
obj=0x00ba5ba8, long id=0x00bab808, unsigned int flags=0x00000000, JSObject * *
objp=0x0012f58c, JSProperty * * propp=0x0012f57c)  Line 2563 + 0x44	C
 	js3250.dll!js_LookupProperty(JSContext * cx=0x00bab178, JSObject *
obj=0x00ba5ba8, long id=0x00bab808, JSObject * * objp=0x0012f58c, JSProperty * *
propp=0x0012f57c)  Line 2421 + 0x1b	C
 	js3250.dll!js_GetProperty(JSContext * cx=0x00bab178, JSObject *
obj=0x00ba5ba8, long id=0x00bab808, long * vp=0x0012f5b0)  Line 2706 + 0x19	C
 	xpc3250.dll!XPCWrappedNativeScope::SetGlobal(XPCCallContext & ccx={...},
JSObject * aGlobal=0x00ba5ba8)  Line 190 + 0x20	C++
 	xpc3250.dll!XPCWrappedNativeScope::XPCWrappedNativeScope(XPCCallContext &
ccx={...}, JSObject * aGlobal=0x00ba5ba8)  Line 159	C++
 	xpc3250.dll!XPCWrappedNativeScope::GetNewOrUsed(XPCCallContext & ccx={...},
JSObject * aGlobal=0x00ba5ba8)  Line 119 + 0x23	C++
 	xpc3250.dll!nsXPConnect::InitClasses(JSContext * aJSContext=0x00bab178,
JSObject * aGlobalJSObj=0x00ba5ba8)  Line 435 + 0xd	C++
 	xpc3250.dll!XPCJSContextStack::GetSafeJSContext(JSContext * *
aSafeJSContext=0x0012f708)  Line 157 + 0x1d	C++
 	xpc3250.dll!nsXPCThreadJSContextStackImpl::GetSafeJSContext(JSContext * *
aSafeJSContext=0x0012f708)  Line 333	C++
 	caps.dll!nsScriptSecurityManager::GetSafeJSContext()  Line 229 + 0x1e	C++
 	caps.dll!nsScriptSecurityManager::Init()  Line 2818 + 0x8	C++
 	caps.dll!nsScriptSecurityManager::GetScriptSecurityManager()  Line 2892 + 0x8	C++
 	caps.dll!Construct_nsIScriptSecurityManager(nsISupports * aOuter=0x00000000,
const nsID & aIID={...}, void * * aResult=0x0012f8f4)  Line 344 + 0x5	C++
 	xpcom_core.dll!nsGenericFactory::CreateInstance(nsISupports *
aOuter=0x00000000, const nsID & aIID={...}, void * * aResult=0x0012f8f4)  Line
82 + 0x15	C++
 	xpcom_core.dll!nsComponentManagerImpl::CreateInstanceByContractID(const char *
aContractID=0x00d104c4, nsISupports * aDelegate=0x00000000, const nsID &
aIID={...}, void * * aResult=0x0012f8f4)  Line 1994 + 0x18	C++
 	xpcom_core.dll!nsComponentManagerImpl::GetServiceByContractID(const char *
aContractID=0x00d104c4, const nsID & aIID={...}, void * * result=0x0012f95c) 
Line 2421 + 0x32	C++
 	xpcom_core.dll!CallGetService(const char * aContractID=0x00d104c4, const nsID
& aIID={...}, void * * aResult=0x0012f95c)  Line 93	C++
 	xpcom_core.dll!nsGetServiceByContractID::operator()(const nsID & aIID={...},
void * * aInstancePtr=0x0012f95c)  Line 276 + 0x13	C++
 
xpc3250.dll!nsCOMPtr<nsIScriptSecurityManager>::assign_from_gs_contractid(nsGetServiceByContractID
gs={...}, const nsID & aIID={...})  Line 1272 + 0x11	C++
 
xpc3250.dll!nsCOMPtr<nsIScriptSecurityManager>::nsCOMPtr<nsIScriptSecurityManager>(nsGetServiceByContractID
gs={...})  Line 678	C++
 	xpc3250.dll!mozJSComponentLoader::ReallyInit()  Line 433	C++
 	xpc3250.dll!mozJSComponentLoader::ModuleForLocation(const char *
registryLocation=0x00bd07f8, nsIFile * component=0x00000000)  Line 829 + 0x17	C++
 	xpc3250.dll!mozJSComponentLoader::GetFactory(const nsID & aCID={...}, const
char * aLocation=0x00bd07f8, const char * aType=0x00bcbcf0, nsIFactory * *
_retval=0x0012fb84)  Line 384 + 0xe	C++
 	xpcom_core.dll!nsFactoryEntry::GetFactory(nsIFactory * * aFactory=0x0012fb84,
nsComponentManagerImpl * mgr=0x0038f380)  Line 302 + 0x3a	C++
 	xpcom_core.dll!nsComponentManagerImpl::CreateInstanceByContractID(const char *
aContractID=0x00b9f8e8, nsISupports * aDelegate=0x00000000, const nsID &
aIID={...}, void * * aResult=0x0012fbe4)  Line 1989 + 0x10	C++
 	xpcom_core.dll!nsComponentManagerImpl::GetServiceByContractID(const char *
aContractID=0x00b9f8e8, const nsID & aIID={...}, void * * result=0x0012fc4c) 
Line 2421 + 0x32	C++
 	xpcom_core.dll!CallGetService(const char * aContractID=0x00b9f8e8, const nsID
& aIID={...}, void * * aResult=0x0012fc4c)  Line 93	C++
 	xpcom_core.dll!nsGetServiceByContractIDWithError::operator()(const nsID &
aIID={...}, void * * aInstancePtr=0x0012fc4c)  Line 286 + 0x13	C++
 	xpcom_core.dll!nsCOMPtr_base::assign_from_gs_contractid_with_error(const
nsGetServiceByContractIDWithError & gs={...}, const nsID & iid={...})  Line
141 + 0x10	C++
 	xpcom_core.dll!nsCOMPtr<nsISupports>::nsCOMPtr<nsISupports>(const
nsGetServiceByContractIDWithError & gs={...})  Line 1007	C++
 	xpcom_core.dll!NS_CreateServicesFromCategory(const char * category=0x004ddbc0,
nsISupports * origin=0x00000000, const char * observerTopic=0x004ddbb0)  Line
817	C++
 	xpcom_core.dll!NS_InitXPCOM2_P(nsIServiceManager * * result=0x0012ff40,
nsIFile * binDirectory=0x00000000, nsIDirectoryServiceProvider *
appFileLocationProvider=0x00000000)  Line 683 + 0x11	C++
 	xpcshell.exe!main(int argc=0x00000001, char * * argv=0x00387040, char * *
envp=0x00382f88)  Line 1499 + 0x22	C++
 	xpcshell.exe!mainCRTStartup()  Line 398 + 0x11	C
 	kernel32.dll!_BaseProcessStart@4()  + 0x23

Comment 1

[timeless]

actual death is caused by:
 	js3250.dll!js_GC(JSContext * cx=0x00bab178, unsigned int gcflags=0x00000005) 
Line ...	C
 	js3250.dll!js_NewGCThing(JSContext * cx=0x00bab178, unsigned int
flags=0x00000004, unsigned int nbytes=0x00000018)  Line 637 + 0xb	C
 	js3250.dll!AllocSlots(JSContext * cx=0x00bab178, long * slots=0x00000000,
unsigned long nslots=0x00000005)  Line 1797 + 0xf	C
 	js3250.dll!js_NewObject(JSContext * cx=0x00bab178, JSClass * clasp=0x100ee810,
JSObject * proto=0x00000000, JSObject * parent=0x00ba5ba8)  Line 1916 + 0xf	C
 	js3250.dll!js_NewFunction(JSContext * cx=0x00bab178, JSObject *
funobj=0x00000000, int (JSContext *, JSObject *, unsigned int, long *, long *)*
native=0x10042550, unsigned int nargs=0x00000001, unsigned int flags=0x00000000,
JSObject * parent=0x00ba5ba8, JSAtom * atom=0x00bab678)  Line 1909 + 0x14	C
 	js3250.dll!js_DefineFunction(JSContext * cx=0x00bab178, JSObject *
obj=0x00ba5ba8, JSAtom * atom=0x00bab678, int (JSContext *, JSObject *, unsigned
int, long *, long *)* native=0x10042550, unsigned int nargs=0x00000001, unsigned
int attrs=0x00000000)  Line 1971 + 0x1f	C
>	js3250.dll!JS_InitClass(JSContext * cx=0x00bab178, JSObject * obj=0x00ba5ba8,
JSObject * parent_proto=0x00000000, JSClass * clasp=0x100ee810, int (JSContext
*, JSObject *, unsigned int, long *, long *)* constructor=0x10042550, unsigned
int nargs=0x00000001, JSPropertySpec * ps=0x100ee4a0, JSFunctionSpec *
fs=0x100ef2e4, JSPropertySpec * static_ps=0x00000000, JSFunctionSpec *
static_fs=0x00000000)  Line 1968 + 0x1b	C

Comment 2

[timeless]

Attachment: i couldn't find anything to root proto

Comment 3

[Brendan Eich [:brendan]]

I had a patch for this bug at one point.  Ah, there it is in attachment 120665 [details] [diff] [review],
from bug 123668:

@@ -1833,20 +1858,30 @@ JS_InitClass(JSContext *cx, JSObject *ob
 
     if (!constructor) {
         /* Lacking a constructor, name the prototype (e.g., Math). */
         named = OBJ_DEFINE_PROPERTY(cx, obj, (jsid)atom, OBJECT_TO_JSVAL(proto),
                                     NULL, NULL, 0, NULL);
         if (!named)
             goto bad;
         ctor = proto;
     } else {
-        /* Define the constructor function in obj's scope. */
+        /*
+         * Define the constructor function in obj's scope.  The object newborn
+         * root pigeon-hole in cx will be overwritten with fun->object, so we
+         * must mark proto in case the GC-private allocation of fun->object's
+         * slots causes a last-ditch GC.
+         * YYY remove this when initial slots are fused with fun->object in one
+         *     extended GC-thing allocation
+         */
+        uint8 *flagp = js_GetGCThingFlags(proto);
+        *flagp |= GCF_MARK;
         fun = js_DefineFunction(cx, obj, atom, constructor, nargs, 0);
+        *flagp &= ~GCF_MARK;
         named = (fun != NULL);
         if (!fun)
             goto bad;
 
         /*
          * Remember the class this function is a constructor for so that
          * we know to create an object of this class when we call the
          * constructor.
          */

It's a hack, but it's cheaper and more isolated than pushing a local root scope.
Shaver, what do you think?

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

Can js_DefineFunction, perhaps with TOO_MUCH_GC in play, cause more than one GC?
 (I'm thinking of a callout to a script-running debugger, or even perhaps some
clever getters and setters in the obj on which we're defining.)

I agree that it's better than pushing a scope, but wonder if it's sufficient.

Comment 5

[timeless]

    fun = (JSFunction *) js_NewGCThing(cx, GCX_PRIVATE, sizeof(JSFunction));
/* ... if (!funobj) */
        funobj = js_NewObject(cx, &js_FunctionClass, NULL, parent);

that looks like two gc's.  for kicks, js_NewObject does:
    obj = (JSObject *) js_NewGCThing(cx, GCX_OBJECT, sizeof(JSObject));
/* ... */
    newslots = AllocSlots(cx, NULL, nslots);
And AllocSlots can trigger a GC, so i count at least three.

Comment 6

[Brendan Eich [:brendan]]

Right, that won't work! ;-)  Old patch, obsoleted; nm.

timeless's patch is pretty good, although awful is too much for me to swallow. 
I'll take it, though, and r/a=me check it in.  Thanks, timeless!

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 181529 [details] [diff] [review]
i couldn't find anything to root proto

As promised.

/be

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 181529 [details] [diff] [review]
i couldn't find anything to root proto

>+    js_LeaveLocalRootScope(cx);
...
>+    JS_LeaveLocalRootScope(cx);

Is the js vs. JS difference intentional?

Comment 9

[Brendan Eich [:brendan]]

Fixed (I renamed awful to bad2, boring but consonant; I fixed the JS_ to be js_).

/be