Closed Bug 418982 Opened 16 years ago Closed 10 years ago

js_InitFunctionAndObjectClasses recursion causes assert and dies.

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED INVALID
mozilla1.9

People

(Reporter: MikeM, Unassigned)

Details

Attachments

(2 files, 1 obsolete file)

When lazy resolving standard classes using a resolver hook on the global object resolving the name "Function" causes recursion and death by JS_Assert.

JS_ASSERT(!entry->key.obj && entry->flags == 0);

JSCLASS_GLOBAL_FLAGS is NOT enabled for the global due to bug# 418264 which has not been resolved yet (no pun intended).

See callstack below:
--------------------------------
JS_Assert(const char * s=0x101cd004, const char * file=0x101ccff8, int ln=1192)  Line 59	C
js_InitFunctionAndObjectClasses(JSContext * cx=0x031c8b18, JSObject * obj=0x03d12020)  Line 1192 + 0x28 bytes	C
js_GetClassObject(JSContext * cx=0x031c8b18, JSObject * obj=0x03d12020, JSProtoKey key=JSProto_Object, JSObject * * objp=0x03d0e610)  Line 2631 + 0xb bytes	C
js_FindClassObject(JSContext * cx=0x031c8b18, JSObject * start=0x00000000, long id=3, long * vp=0x03d0e638)  Line 2685 + 0x15 bytes	C
js_GetClassPrototype(JSContext * cx=0x031c8b18, JSObject * scope=0x03d12020, long id=3, JSObject * * protop=0x03d0e690)  Line 4526 + 0x15 bytes	C
JS_InitClass(JSContext * cx=0x031c8b18, JSObject * obj=0x03d12020, JSObject * parent_proto=0x00000000, JSClass * clasp=0x101d2ac8, int (JSContext *, JSObject *, unsigned int, long *, long *)* constructor=0x100c1f70, unsigned int nargs=1, JSPropertySpec * ps=0x101d2790, JSFunctionSpec * fs=0x101d2bd0, JSPropertySpec * static_ps=0x00000000, JSFunctionSpec * static_fs=0x00000000)  Line 2654 + 0x1b bytes	C
js_InitFunctionClass(JSContext * cx=0x031c8b18, JSObject * obj=0x03d12020)  Line 1941 + 0x29 bytes	C
js_InitFunctionAndObjectClasses(JSContext * cx=0x031c8b18, JSObject * obj=0x03d12020)  Line 1211 + 0xd bytes	C
JS_ResolveStandardClass(JSContext * cx=0x031c8b18, JSObject * obj=0x03d12020, long id=64041068, int * resolved=0x03d0e730)  Line 1516 + 0xf bytes	C
request_global_resolve(JSContext * cx=0x031c8b18, JSObject * __formal=0x03d12000, long id=64041068, unsigned int flags=16, JSObject * * objp=0x03d0e7b8)  Line 103 + 0x15 bytes	C++
js_LookupPropertyWithFlags(JSContext * cx=0x031c8b18, JSObject * obj=0x03d12000, long id=64041068, unsigned int flags=16, JSObject * * objp=0x03d0e800, JSProperty * * propp=0x03d0e7f0)  Line 3291 + 0x17 bytes	C
js_FindClassObject(JSContext * cx=0x031c8b18, JSObject * start=0x00000000, long id=64041068, long * vp=0x03d0e82c)  Line 2696 + 0x1b bytes	C
js_GetClassPrototype(JSContext * cx=0x031c8b18, JSObject * scope=0x03d12000, long id=5, JSObject * * protop=0x03d0e85c)  Line 4526 + 0x15 bytes	C
js_NewObject(JSContext * cx=0x031c8b18, JSClass * clasp=0x101d2ac8, JSObject * proto=0x00000000, JSObject * parent=0x03d12000)  Line 2423 + 0x15 bytes	C
js_CloneFunctionObject(JSContext * cx=0x031c8b18, JSObject * funobj=0x03d12600, JSObject * parent=0x03d12000)  Line 2083 + 0x14 bytes	C
js_Interpret(JSContext * cx=0x031c8b18, unsigned char * pc=0x031cdd58, long * result=0x03d0eefc)  Line 5438 + 0x14 bytes	C
js_Execute(JSContext * cx=0x031c8b18, JSObject * chain=0x03d12000, JSScript * script=0x031cdc90, JSStackFrame * down=0x00000000, unsigned int flags=0, long * result=0x03d0f0e8)  Line 1649 + 0x13 bytes	C
JS_ExecuteScript(JSContext * cx=0x031c8b18, JSObject * obj=0x03d12000, JSScript * script=0x031cdc90, long * rval=0x03d0f0e8)  Line 4823 + 0x19 bytes	C
Can anyone comment on what this ASSERT means?
Should I produce a patch which just cuts out the offending line?
I assume its there for a good reason.
This is still broken.

Apparently js_InitFunctionAndObjectClasses() is getting called twice.
It looks like a pretty basic case. 
Not so obscure Brendan :-)

Any ideas on a work around or fix?

The assertion is there for a reason. js_InitFunctionAndObjectClasses is meant to be called recursively, so that's not it. This looks like a bug to do with your not using JSCLASS_GLOBAL_FLAGS (you are not using those flags, right?). Need a test set-up like yours. Can you provide a minimal embedding program, or maybe a patch to js.c? Thanks,

/be
Yes I'm not using JSCLASS_GLOBAL_FLAGS for the global in this case.
Does the call stack provided help?

Also could you explain what that Assert does? I might find the bug myself quicker if I knew what was actually going on.

FYI. Release builds run fine no crashes or weird behavior.
Standard class init is resolve-based when lazy, so it must avoid recurring to death when the resolve hook wants to lookup the same id it's resolving, or a different id that's being resolved under it on the stack. So here:

        key.id = ATOM_TO_JSID(rt->atomState.classAtoms[JSProto_Function]);
        entry = (JSResolvingEntry *)
                JS_DHashTableOperate(table, &key, JS_DHASH_ADD);

We lookup-and-add-if-not-present an entry in cx->resolvingTable for 'Function'.

        if (entry && entry->key.obj && (entry->flags & JSRESFLAG_LOOKUP)) {
            /* Already resolving Function, record Object too. */
            JS_ASSERT(entry->key.obj == obj);
            key.id = ATOM_TO_JSID(rt->atomState.classAtoms[JSProto_Object]);
            entry = (JSResolvingEntry *)
                    JS_DHashTableOperate(table, &key, JS_DHASH_ADD);

This all should be clear from the comment and the similar code: if resolving 'Function' led to js_InitFunctionAndObjectClasses being called, then we put 'Object' in cx->resolvingTable too, since both classes are handled by the one js_InitFunctionAndObjectClasses helper.

        }
        if (!entry) {
            JS_ReportOutOfMemory(cx);
            return NULL;
        }

Common OOM check.

        JS_ASSERT(!entry->key.obj && entry->flags == 0);
        entry->key = key;
        entry->flags = JSRESFLAG_LOOKUP;

If the JSDHashTable has no entry for the given key, given the JS_DHASH_ADD op, it will create a new entry, initialize its keyHash member, and leave the rest zeroed. The assertion is insisting on this.

What values (hex is good) do you see in *entry at the assertbotch point?

/be
Heres *entry all expanded out from the watch window:
---------------------------------------------------------

- *entry	{hdr={...} key={...} flags=1 }	JSResolvingEntry
- hdr	{keyHash=3035604832 }	JSDHashEntryHdr
    keyHash	3035604832	unsigned long
- key	{obj=0x03d11020 id=64041060 }	JSResolvingKey
-   obj	0x03d11020 {map=0x034daf00 fslots=0x03d11024 dslots=0x034daf7c }	JSObject *
-     map	0x034daf00 {nrefs=1 ops=0x101cc610 freeslot=37 }	JSObjectMap *
        nrefs	1	long
+	ops	0x101cc610 _js_ObjectOps {newObjectMap=0x100c6f30 destroyObjectMap=0x100c6f60 lookupProperty=0x100c93a0 ...}	JSObjectOps *
	freeslot	37	unsigned long
-	fslots	0x03d11024	long [6]
	  [0]	0	long
	  [1]	0	long
	  [2]	270241857	long
	  [3]	-2147483647	long
	  [4]	-2147483647	long
	  [5]	-2147483647	long
-	dslots	0x034daf7c	long *
		-2147483647	long
	id	64041060	long
     flags	1	unsigned long
More info:

On 1st pass through the assert passes fine.
The line right after the ASSERT is where the non-null entry->key.obj is coming from. Both the obj and id of the key are zeros until that point in the code.
Then flags is set too as you can see...
2nd pass appears to be trying to resolve the prototype of "Function" and trips after what pass #1 did below.
----------------
        JS_ASSERT(!entry->key.obj && entry->flags == 0);
        entry->key = key;
        entry->flags = JSRESFLAG_LOOKUP;
-----------------

The reason key.obj is non-null is because it was explicitly set 16 lines above the ASSERT:

------------------
 rt = cx->runtime;
    key.obj = obj;   <-- SET HERE
    if (resolving) {
------------------

So the assert looks bogus to me.
Scratch that last comment.  Its wrong and shows incomplete understanding!

Here's the bug from what I can see:

The code assumes that if "Function" was just added to the table it should add "Object" too (on the 2nd pass).

But what if "Object" was ALREADY a live entry in the table?

In that case entry->key.obj would have a non-null value and the ASSERT will fire.

Possible fix is to check for live entry before setting the entry-key??
See below:

-----------------
if(!ENTRY_IS_LIVE) {
        JS_ASSERT(!entry->key.obj && entry->flags == 0);
        entry->key = key;
        entry->flags = JSRESFLAG_LOOKUP;
}
-----------------
That possible fix above doesn't work and makes the recursion worse.

The way js_InitFunctionAndObjectClasses() is written forces "Object" to not actually get added until during the call to js_InitFunctionClass() which again calls InitFunctionAndObjectClasses() which it shouldn't.

It needs to be done right away before anybody else trys to add "Object" to the table before we do.

FYI here was the call stack which adds "Object" to the table unexpectedly:

JS_DHashTableOperate(JSDHashTable * table=0x034b95c8, const void * key=0x03d0e5c4, JSDHashOperator op=JS_DHASH_ADD)  Line 601	C
js_StartResolving(JSContext * cx=0x031caa60, JSResolvingKey * key=0x03d0e5c4, unsigned long flag=1, JSResolvingEntry * * entryp=0x03d0e5b8)  Line 555 + 0xd bytes	C
js_GetClassObject(JSContext * cx=0x031caa60, JSObject * obj=0x03d11020, JSProtoKey key=JSProto_Object, JSObject * * objp=0x03d0e608)  Line 2619 + 0x13 bytes	C
js_FindClassObject(JSContext * cx=0x031caa60, JSObject * start=0x00000000, long id=3, long * vp=0x03d0e630)  Line 2685 + 0x15 bytes	C
js_GetClassPrototype(JSContext * cx=0x031caa60, JSObject * scope=0x03d11020, long id=3, JSObject * * protop=0x03d0e688)  Line 4526 + 0x15 bytes	C
JS_InitClass(JSContext * cx=0x031caa60, JSObject * obj=0x03d11020, JSObject * parent_proto=0x00000000, JSClass * clasp=0x101cbad8, int (JSContext *, JSObject *, unsigned int, long *, long *)* constructor=0x100be8c0, unsigned int nargs=1, JSPropertySpec * ps=0x101cb7a0, JSFunctionSpec * fs=0x101cbbe0, JSPropertySpec * static_ps=0x00000000, JSFunctionSpec * static_fs=0x00000000)  Line 2659 + 0x1b bytes	C
js_InitFunctionClass(JSContext * cx=0x031caa60, JSObject * obj=0x03d11020)  Line 1941 + 0x29 bytes	C
js_InitFunctionAndObjectClasses(JSContext * cx=0x031caa60, JSObject * obj=0x03d11020)  Line 1216 + 0xd bytes	C
JS_ResolveStandardClass(JSContext * cx=0x031caa60, JSObject * obj=0x03d11020, long id=64041068, int * resolved=0x03d0e72c)  Line 1521 + 0xf bytes	C
request_global_resolve(JSContext * cx=0x031caa60, JSObject * __formal=0x03d11000, long id=64041068, unsigned int flags=16, JSObject * * objp=0x03d0e7b4)  Line 103 + 0x15 bytes	C++

Inside js_GetClassObject() the following code exists:
---------------------
rkey.obj = obj;
    rkey.id = ATOM_TO_JSID(cx->runtime->atomState.classAtoms[key]);
    if (!js_StartResolving(cx, &rkey, JSRESFLAG_LOOKUP, &rentry))
        return JS_FALSE;
    if (!rentry) {
        /* Already caching key in obj -- suppress recursion. */
        *objp = NULL;
        return JS_TRUE;
    }
    generation = cx->resolvingTable->generation;

    cobj = NULL;
    init = lazy_prototype_init[key];
-------------------------

Essentially js_StartResolving() and init(cx, obj) were both trying to add "Object" to the table.

Patch to follow....
Attached patch Patch v1 (obsolete) — Splinter Review
This code fixes the bug as far as I can tell.  No other regression testing done.
It's a simple fix and I like it.
Attachment #307624 - Flags: review?(brendan)
I found another way to break it.  Patch is bad.  Will upload a replacement when I figure out how to fix it.
Attached patch Patch V2Splinter Review
This patch attempts to avoid the ASSERT unless checking new entries in the table.
This is because between the 1st and 2nd pass into this function somebody else has added the "Object" to the resolver table.

If we call JS_DHashTableOperate() a second time when JSProto_Function now exists it is possible that the entry will come back with entry->key.obj && entry->flags both set in which case we do not want to assert. 

Fixes the 1st assert but now there is an assert in
js_stopResolving():

   JS_ASSERT(JS_DHASH_ENTRY_IS_BUSY(&entry->hdr));

This dance is too tricky for me, I can't get inside the authors head.
Ok.. I admit defeat :-<
Attachment #307624 - Attachment is obsolete: true
Attachment #307662 - Flags: review?(brendan)
Attachment #307624 - Flags: review?(brendan)
Comment on attachment 307662 [details] [diff] [review]
Patch V2

Let's debug first, patch some code (probably not this code) after we know what is going wrong.

/be
Attachment #307662 - Flags: review?(brendan) → review-
Brendan,
PLEASE READ THIS.

Here's the basic flow and problem.  (a few frames omitted for brevity)

1) request_global_resolve
2) JS_ResolveStandardClasses
3) js_InitFunctionAndObjectClasses
4) JS_DHASH_ADD on JSProto_Function this works fine
5) js_InitFunctionClass() is called after Function added to table (Object NOT added yet)
6) JS_InitClass
7) js_GetClassPrototype on JSProto_Object
8) js_GetClassObject on JSProto_Object
9) js_StartResolving on key JSProto_Object
10)   JS_DHASH_ADD on JSProto_Object (1st time add) Object now in table.
11)   lazy_prototype_init finds init function js_InitFunctionAndObjectClasses
12)   js_InitFunctionAndObjectClasses is called again
13)      JS_DHASH_ADD on JSProto_Function. Function already exists
14)      JS_DHASH_ADD on JSProto_Object since Function already in table.
15)      ASSERT fails because code doesn't consider if Object already in table
         and assumes entry->key.obj must be null
Thanks, this is indeed a regression to embeddings that do not use JSCLASS_GLOBAL_FLAGS -- IIRC you don't use those flags to avoid another bug. If you could try them just to confirm they fix this symptom, that would be great. I will fix this bug independently of those flags, so you won't have to set them if you don't want to.

/be
Assignee: general → brendan
Priority: -- → P2
Target Milestone: --- → mozilla1.9
If I re-enable JSCLASS_GLOBAL_FLAGS as a workaround then I run into bug# 418264. So I can't. At least not until that one is fixed.
Here is a set of files which will reproduce the ASSERT for this bug.
The script being run in completely meaningless.
This was taken from an older test case.  Ignore cruft and everything except the ASSERT.
Stealing from Brendan.  This should block a 1.8src release bug; bclary: is there one?
Assignee: brendan → crowder
MikeM:  still having trouble with this?
I uploaded a test code for the bug.  Have you run this yet?
Mass unowning JS bugs...  I'm not likely to be able to move these forward.
Assignee: crowder → general
Marking as INVALID based on the embedding API having changed completely and the test case being woefully out of date. Also, the way the resolve stuff works has changed quite drastically, so it's very, very likely that this bug is either plain fixed, or has very different characteristics.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: