299205 - [FIX]JS_ASSERT(flags != GCF_FINAL) hit in UnmarkedGCThingFlags() on startup with TOO_MUCH_GC defined

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Johnny Stenback (:jst)]

Running with TOO_MUCH_GC defined I crash (fatal assert) on startup at:

JS_Assert(const char * s=0x100f1a60, const char * file=0x100f1a2c, int
ln=0x00000408) Line 155	C
UnmarkedGCThingFlags(void * thing=0x02f5c1c8) Line 1032 + 0x1f	C
NextUnmarkedGCThing(long * vp=0x02c57c58, long * end=0x02c57c5c, void * *
thingp=0x0012cdc0, unsigned char * * flagpp=0x0012cdd0) Line 1056 + 0x9	C
MarkGCThing(JSContext * cx=0x02a43160, void * thing=0x02f5c1b8, unsigned char *
flagp=0x02f5ae47) Line 1202 + 0x18	C
js_MarkGCThing(JSContext * cx=0x02a43160, void * thing=0x02f5c0b8, void *
arg=0x00000000) Line 1438 + 0x11	C
JS_MarkGCThing(JSContext * cx=0x02a43160, void * thing=0x02f5c0b8, const char *
name=0x0254e2b4, void * arg=0x00000000) Line 1827 + 0xf	C
nsDOMClassInfo::MarkReachablePreservedWrappers(nsIDOMNode * aDOMNode=0x02de612c,
JSContext * cx=0x02a43160, void * arg=0x00000000) Line 4641 + 0x17	C++
nsNodeSH::Mark(nsIXPConnectWrappedNative * wrapper=0x02f69060, JSContext *
cx=0x02a43160, JSObject * obj=0x02f5b0e8, void * arg=0x00000000, unsigned int *
_retval=0x0012ce7c) Line 5589 + 0x16	C++
XPC_WN_Helper_Mark(JSContext * cx=0x02a43160, JSObject * obj=0x02f5b0e8, void *
arg=0x00000000) Line 936	C++
js_Mark(JSContext * cx=0x02a43160, JSObject * obj=0x02f5b0e8, void *
arg=0x00000000) Line 4053 + 0x12	C
MarkGCThing(JSContext * cx=0x02a43160, void * thing=0x02f5b0e8, unsigned char *
flagp=0x02f5ac2d) Line 1138 + 0x23	C
js_MarkGCThing(JSContext * cx=0x02a43160, void * thing=0x02f5b0e8, void *
arg=0x00000000) Line 1438 + 0x11	C
gc_root_marker(JSDHashTable * table=0x00b65834, JSDHashEntryHdr *
hdr=0x02f97014, unsigned long num=0x00000000, void * arg=0x02a43160) Line 1477
+ 0x12	C
JS_DHashTableEnumerate(JSDHashTable * table=0x00b65834, JSDHashOperator
(JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *)* etor=0x10047370,
void * arg=0x02a43160) Line 618 + 0x19	C
js_GC(JSContext * cx=0x02a43160, unsigned int gcflags=0x00000005) Line 1693 + 0x18	C
js_NewGCThing(JSContext * cx=0x02a43160, unsigned int flags=0x00000000, unsigned
int nbytes=0x00000008) Line 571 + 0xb	C
js_NewObject(JSContext * cx=0x02a43160, JSClass * clasp=0x02a9f4a4, JSObject *
proto=0x02c64a68, JSObject * parent=0x02f5c0b8) Line 1855 + 0xd	C
JS_NewObject(JSContext * cx=0x02a43160, JSClass * clasp=0x02a9f4a4, JSObject *
proto=0x02c64a68, JSObject * parent=0x02f5c0b8) Line 2247 + 0x15	C
XPCWrappedNative::Init(XPCCallContext & ccx={...}, JSObject * parent=0x02f5c0b8,
const XPCNativeScriptableCreateInfo * sci=0x0012d21c) Line 764 + 0x1b	C++
XPCWrappedNative::GetNewOrUsed(XPCCallContext & ccx={...}, nsISupports *
Object=0x02fd56d4, XPCWrappedNativeScope * Scope=0x02a43590, XPCNativeInterface
* Interface=0x00ba4710, XPCWrappedNative * * resultWrapper=0x0012d2c4) Line
391 + 0x1b	C++
XPCConvert::NativeInterface2JSObject(XPCCallContext & ccx={...},
nsIXPConnectJSObjectHolder * * dest=0x0012d3d0, nsISupports * src=0x02fd56d4,
const nsID * iid=0x0244e9e4, JSObject * scope=0x01b56dc8, int
allowNativeWrapper=0x00000000, unsigned int * pErr=0x0012d310) Line 1064 + 0x1e	C++
nsXPConnect::WrapNative(JSContext * aJSContext=0x02a43160, JSObject *
aScope=0x01b56dc8, nsISupports * aCOMObj=0x02fd56d4, const nsID & aIID={...},
nsIXPConnectJSObjectHolder * * _retval=0x0012d3d0) Line 582 + 0x22	C++
nsDOMClassInfo::WrapNative(JSContext * cx=0x02a43160, JSObject *
scope=0x01b56dc8, nsISupports * native=0x02fd56d4, const nsID & aIID={...}, long
* vp=0x0012ddf4) Line 1281 + 0x3a	C++
nsArraySH::GetProperty(nsIXPConnectWrappedNative * wrapper=0x02feca50, JSContext
* cx=0x02a43160, JSObject * obj=0x02f5c0f0, long id=0x00000003, long *
vp=0x0012ddf4, int * _retval=0x0012d43c) Line 6018 + 0x2a	C++
XPC_WN_Helper_GetProperty(JSContext * cx=0x02a43160, JSObject * obj=0x02f5c0f0,
long idval=0x00000003, long * vp=0x0012ddf4) Line 851 + 0x2f	C++
js_GetProperty(JSContext * cx=0x02a43160, JSObject * obj=0x02f5c0f0, long
id=0x00000003, long * vp=0x0012ddf4) Line 2750 + 0x13b	C
js_Interpret(JSContext * cx=0x02a43160, unsigned char * pc=0x02f92fbc, long *
result=0x0012de6c) Line 3312 + 0x74e	C

What I've learned so far is that it in a call to JS_MarkGCThing() for a
XULElement, but somewhere in the marking code we end up at a XPCWrappedNative
for a nsPrefBranch and it seems like we're hitting the assert while looping over
the slots on the wrapper.

FWIW, this happens both with and w/o fastback enabled.

Comment 1

[Boris Zbarsky [:bzbarsky]]

It looks like this started happening with my checkin for bug 295983....  Looking
into what could be going on now.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the change I made to EvaluateStringWithValue seems to be the problem.  In
particular, just locking and then unlocking the GCThing is enough to cause this
bug.  So if I comment out the ScriptEvaluated call there, we still hit this
assert.  Simply locking on its own is not enough.  Unlocking is required.

I tried replacing lock/unlock with JS_AddNamedRoot/JS_RemoveRoot.  That still
has the same issue.  Simply adding and immediately removing the root causes us
to assert sometime later.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

OK, so the story, per Brendan is that locking/unlocking or rooting/unrooting
pokes the gcpoke thingie, which means we gc at the _next_ available
opportunity... and that JS_DefineUCProperty can GC as it atomizes the prop
name.

This makes us just root the jval* we pass to EvaluateStringWithValue.  Also
adds some rooting of a function object where I think it's needed.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 195065 [details] [diff] [review]
Patch

+// Struct to handle automatic rooting and unrooting
+struct nsAutoGCRoot {
+  // aPtr should be the pointer to the thing we wanr to protect (so a
+  // jsval* or a JSObject** or something along those lines).
+  nsAutoGCRoot(void* aPtr, nsresult* aResult) :
+    mPtr(aPtr)
+  {
+    mResult = *aResult =
+      nsXBLProtoImplMember::AddJSGCRoot(mPtr, "nsAutoGCRoot");
+  }

How about making this more type safe by providing one ctor that takes a jsval*
and one that takes a JSObject*?

r=jst with that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Sure, I can do that...

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 195065 [details] [diff] [review]
Patch

>-  jsval rval;
>+  jsval rval = JSVAL_NULL;
>+  if (!::JS_AddNamedRoot(cx, &rval, "NPN_evaluate")) {
>+    return false;
>+  }
>+    
>   nsresult rv = scx->EvaluateStringWithValue(utf16script, obj, principal,
>                                              nsnull, 0, nsnull, &rval, nsnull);
>+
>+  ::JS_RemoveRoot(cx, &rval);
>+  
>   NS_ENSURE_SUCCESS(rv, false);
> 
>   if (result) {
>     return JSValToNPVariant(npp, cx, rval, result);
>   }

Just in case JSValToNPVariant calls a JS API entry point that allocates or
otherwise might collect, we should not remove rval as a root till after this
point -- taking care to avoid early returns hidden in evil macros, of course!

>   // compile the literal string 
>-  jsval result = nsnull;
>+  jsval result = JSVAL_NULL;

How about an extra newline here for readability?

>+  // JS_DefineUCProperty can and EvaluateStringWithValue can both

Nit: "can" doubled wrongly.

Looks good otherwise, sr=me with these fixed.

/be

Comment 7

[Johnny Stenback (:jst)]

bz, you want me to make the suggested changes here, or you want to take this bug
and do that yourself?

Comment 8

[Johnny Stenback (:jst)]

Oh, and I just realized that we'd probably want something like this for the code
that was changed in the fix for bug 307040 as well, right?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Oh, and I just realized that we'd probably want something like this for the code
> that was changed in the fix for bug 307040 as well, right?

No, that patch is good -- the newborn function object is protected by its
newborn root, which won't be displaced until it is homed to the slot in the
property being defened (before any addProperty call-out from
js_DefineNativeProperty, underneath JS_DefineUCProperty).

Atomizing the property name will not disturb atoms or pre-empt any root other
than a string root, and even then you'd have to GC after that displacement via
another path to the allocator under *Define*Property, which does not exist.

Contrast to this bug's patch, where arbitrary layering of C++ interface and
implementation on top of the JS API (used deep in the implementation, with
ScriptEvaluated call-outs and such interleaved!) requires the rval to be rooted.
 Who knows what might GC?  But in the simple straight-line JS API use-cases for
which newborn rooting was designed, such as New => Define, it's enough.

/be

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed trunk and branch.

Comment 12

[Boris Zbarsky [:bzbarsky]]

It looks like comment 10 is not quite right.  :(  See the stack in bug 327712 comment 2.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> It looks like comment 10 is not quite right.  :(  See the stack in bug 327712
> comment 2.

That comment seems wildly optimistic to me now.  Forget it.

/be