491013 - "Assertion failure: OBJ_SCOPE(obj)->object == pobj, at ../jsobj.cpp" or "Assertion failure: OBJ_IS_CLONED_BLOCK(obj), at ../jsobj.cpp"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

let(x) ((function(){for( y in [0, function(){}, 0, 0]) x = x})())


asserts debug js shell without -j at Assertion failure: OBJ_SCOPE(obj)->object == pobj, at ../jsobj.cpp:4069

let(x) ((function(){for( y in [0, function(){}]) x = x})())


asserts debug js shell without -j at Assertion failure: OBJ_IS_CLONED_BLOCK(obj), at ../jsobj.cpp:2445

Both seem related, as autoBisect shows they are probably related to bug 490666 :

The first bad revision is:
changeset:   27505:3519ddacbe2e
user:        Andreas Gal
date:        Thu Apr 30 15:52:13 2009 -0700
summary:     We don't cache access to shared properties in the property cache (490666, r=igor,brendan).

Comment 1

[Andreas Gal :gal]

No JIT involvement. 

#0  JS_Assert (s=0x1ad3f8 "OBJ_IS_CLONED_BLOCK(obj)", file=0x1ac55a "../jsobj.cpp", ln=2445) at ../jsutil.cpp:69
#1  0x000adf29 in js_PutBlockObject (cx=0x30bc80, normalUnwind=1) at ../jsobj.cpp:2445
#2  0x00092a78 in js_Interpret (cx=0x30bc80) at ../jsinterp.cpp:6955
#3  0x00096a29 in js_Execute (cx=0x30bc80, chain=0x2ac000, script=0x30d8d0, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1633
#4  0x00010d3a in JS_ExecuteScript (cx=0x30bc80, obj=0x2ac000, script=0x30d8d0, rval=0x0) at ../jsapi.cpp:5056
#5  0x0000a206 in Process (cx=0x30bc80, obj=0x2ac000, filename=0xbffff991 "x.js", forceTTY=0) at ../../shell/js.cpp:412
#6  0x0000af1a in ProcessArgs (cx=0x30bc80, obj=0x2ac000, argv=0xbffff898, argc=1) at ../../shell/js.cpp:806
#7  0x0000b2e7 in main (argc=1, argv=0xbffff898, envp=0xbffff8a0) at ../../shell/js.cpp:4728
(gdb) 

No idea what happens here. Jim?

Comment 2

[Igor Bukanov]

The reason for the bug is js_GetMutableScope call is JSOP_SETPROP/JSOP_SETNAME. Now, since we cache the shared properties, it is wrong to do that unconditionally.

Comment 3

[Igor Bukanov]

Attachment: v1

The essence of the patch is to make sure that the property cache code for SETPROP/SETNAME does not create scopes for shared properties. 

Another set of changes is to remove useless property-less prototypes for anonymous objects consistently using JS_NewObjectWithGivenProto to create prototype-less Block, NoSuchMethod and XMLFilter objects. That allowed to simplifu code and provide better assert coverage to ensure that no scope is ever created for cloned block objects.

Comment 4

[Jim Blandy :jimb]

Separately from any cache issues, a block object created by js_GetBlockObject
should always share its scope with its prototype --- the immutable object built
by the compiler.  So I don't think we should ever be applying
js_GetMutableScope to a block... unless perhaps the compiler needs to do so.

Comment 5

[Igor Bukanov]

Attachment: v2

The testing with the try server revealed that the Block object prototype was used to xdr scripts. The new version fixes that using direct calls to Block, Function or RegExp functions in js_XDRScript. It generates strictly less code than keeping the Block prototype initialization code.

Comment 6

[Igor Bukanov]

(In reply to comment #4)
> So I don't think we should ever be applying
> js_GetMutableScope to a block... unless perhaps the compiler needs to do so.

This is what the patch asserts - it insists that js_GetMutableScope is not feed with a block object.

Comment 7

[Igor Bukanov]

Comment on attachment 375439 [details] [diff] [review]
v2

Asking for a review as the patch has passed the try server. For details, see the comments above.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 375439 [details] [diff] [review]
v2

>+                            /*
>+                             * Fastest path: check if the cached sprop is
>+                             * already in scope to call NATIVE_GET and break

s/to call/and call/

>+                             * to get out of the do-while(0). But we only can
>+                             * call NATIVE_GET if obj owns scope or sprop is

s/But we only can call NATIVE_GET if/But we can call NATIVE_GET only if/

> js_NewBlockObject(JSContext *cx)
> {
>     /*
>      * Null obj's proto slot so that Object.prototype.* does not pollute block
>+     * scopes and to give own scope to the block object.

s/own scope to the block object/the block object its own scope/

>@@ -65,18 +65,24 @@ js_GetMutableScope(JSContext *cx, JSObje
> {
>     JSScope *scope, *newscope;
>     JSClass *clasp;
>     uint32 freeslot;
> 
>     scope = OBJ_SCOPE(obj);
>     JS_ASSERT(JS_IS_SCOPE_LOCKED(cx, scope));
>     if (scope->object == obj)
>         return scope;
>+
>+    /*
>+     * Only a compile-time block object has own scope which is created
>+     * together with the object.

How about "Compile-time block objects each have their own scope, created at birth, and runtime clone of a block objects are never mutated" instead?

>+        uint32 kind;
>+        if (xdr->mode == JSXDR_ENCODE) {
>+            JSClass *clasp = STOBJ_GET_CLASS(*objp);
>+            if (clasp == &js_FunctionClass) {
>+                kind = 0;
>+            } else {
>+                JS_ASSERT(clasp == &js_BlockClass);
>+                kind = 1;

Use an enum for 0 and 1?

/be

Comment 9

[Igor Bukanov]

Attachment: v3

The new version of the patch addresses the nits above. 

In js_XDRScript I replaced kind with the isBlock pseudo-boolean. The reason I do not want to have an enumeration is that it suggests that more objects, besides Block and Function, may come in future. But I suspect those new objects, if any, will endup using separated layout and there would be no need to use any enums to distinguish them. Case in point is the regular expressions that are stored and serialized separately.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 375478 [details] [diff] [review]
v3

>+                            /*
..12345678901234567890123456789012345678901234567890123456789012345678901234567890
>+                             * Fastest path: check if the cached sprop is

Sorry, final nit: s/if/whether/ -- should fit.

>+                             * already in scope and call NATIVE_GET and break
>+                             * to get out of the do-while(0). But we can call
>+                             * NATIVE_GET only if obj owns scope or sprop is
>+                             * shared.
>+                             */

r=me with that, thanks.

/be

Comment 11

[Kent James (:rkent)]

I was experiencing this bug reproducibly when I opened an IMAP folder in my custom debug build of TB on 1.9.1. I applied the patch v2 to mozilla 1.9.1 (with a small manual correction due to an unsuccessful hunk) and the crash went away.

It would be useful if you would post the 1.9.1 version of the new patch as well, since those of us doing TB development are typically using 1.9.1, may be halted by this bug, and need to apply it locally even if you decide for some reason to delay the application to 1.9.1

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #11)
> I was experiencing this bug reproducibly when I opened an IMAP folder in my
> custom debug build of TB on 1.9.1. I applied the patch v2 to mozilla 1.9.1
> (with a small manual correction due to an unsuccessful hunk) and the crash went
> away.
> 
> It would be useful if you would post the 1.9.1 version of the new patch as
> well, since those of us doing TB development are typically using 1.9.1, may be
> halted by this bug, and need to apply it locally even if you decide for some
> reason to delay the application to 1.9.1

See bug 490128 which is already marked as a bug that this bug blocks.

Comment 13

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/6534f8b9aa74 - landed with the nit from the comment 10 addressed

Comment 14

[Kent James (:rkent)]

I spoke too soon in comment 11. Today, in the same circumstance of comment 11 (after applying v2 of the patch to 1.9.1) I am getting this error reproducibly followed by a crash:

Assertion failure: STOBJ_GET_CLASS(obj) != &js_BlockClass, at c:/tb/test/src/moz
illa/js/src/jsscope.cpp:79

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> I spoke too soon in comment 11. Today, in the same circumstance of comment 11
> (after applying v2 of the patch to 1.9.1) I am getting this error reproducibly
> followed by a crash:
> 
> Assertion failure: STOBJ_GET_CLASS(obj) != &js_BlockClass, at
> c:/tb/test/src/moz
> illa/js/src/jsscope.cpp:79

What is the stack trace for the error?

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #15)
> (In reply to comment #14)
> > I spoke too soon in comment 11. Today, in the same circumstance of comment 11
> > (after applying v2 of the patch to 1.9.1) I am getting this error reproducibly
> > followed by a crash:
> > 
> > Assertion failure: STOBJ_GET_CLASS(obj) != &js_BlockClass, at
> > c:/tb/test/src/moz
> > illa/js/src/jsscope.cpp:79
> 
> What is the stack trace for the error?

Kent, besides backtrace (for js assertions),

(gdb) bt

one can also do:

(gdb) frame 1
(gdb) call JS_GetFramePC(cx, cx->fp)
(gdb) call JS_PCToLineNumber(cx, cx->fp->script, $1)
(gdb) p cx->fp->script->filename

for more useful information (see bug 490128 comment #0 near the bottom) if you are using gdb.

Comment 17

[Andreas Gal :gal]

Assertion failure: isBlock == 1, at ../../../js/src/jsscript.cpp:614

Program received signal SIGTRAP, Trace/breakpoint trap.
JS_Assert (s=0x45 <Address 0x45 out of bounds>, file=0x45 <Address 0x45 out of bounds>, ln=69) at ../../../js/src/jsutil.cpp:69
69	    abort();
(gdb) bt
#0  JS_Assert (s=0x45 <Address 0x45 out of bounds>, file=0x45 <Address 0x45 out of bounds>, ln=69) at ../../../js/src/jsutil.cpp:69
#1  0x0028456f in js_XDRScript (xdr=0x65b310, scriptp=0xbfffe724, hasMagic=0x0) at ../../../js/src/jsscript.cpp:614
#2  0x0028e004 in JS_XDRScript (xdr=0x65b310, scriptp=0xbfffe724) at ../../../js/src/jsxdrapi.cpp:690
#3  0x111d45b8 in ReadScriptFromStream [inlined] () at ../../../../../js/src/xpconnect/loader/mozJSComponentLoader.cpp:443
#4  0x111d45b8 in mozJSComponentLoader::ReadScript (this=0x622350, flSvc=0x655700, nativePath=0x657428 "file:///Users/gal/workspace/tracemonkey-repository/debug-build/dist/MinefieldDebug.app/Contents/MacOS/components/fuelApplication.js", uri=0x65d6d0, cx=0x862600, script=0xbfffe724) at ../../../../../js/src/xpconnect/loader/mozJSComponentLoader.cpp:1069

Comment 18

[Andreas Gal :gal]

Backed out due to assert during startup in debug builds.

http://hg.mozilla.org/tracemonkey/rev/38512deaca7e

Comment 19

[Igor Bukanov]

(In reply to comment #17)
> Assertion failure: isBlock == 1, at ../../../js/src/jsscript.cpp:614

Hm, it seems as if the system has not picked up XDR version change. Have you tried an optimized build?

Comment 20

[Igor Bukanov]

Attachment: v4

Given that the backed out version of the patch has passed all tinderbox tests and my local debug builds of the browser on Linux runs just fine it is unclear what caused the assert on Mac. The fact that the code around the asserts is heavily executed on the startup adds even more fog.

So to make sure that this is not caused by some wired implementation of boolean-to-unsigned cast I updates the patch to use an explicit check, not implicit conversion in js_XDRScript:

         if (xdr->mode == JSXDR_ENCODE) {
             JSClass *clasp = STOBJ_GET_CLASS(*objp);
             JS_ASSERT(clasp == &js_FunctionClass ||
                       clasp == &js_BlockClass);
-            isBlock = (clasp == &js_BlockClass);
+            isBlock = (clasp == &js_BlockClass) ? 1 : 0;
          }
         if (!JS_XDRUint32(xdr, &isBlock))
             goto error;
         if (isBlock == 0) {
             if (!js_XDRFunctionObject(xdr, objp))
                 goto error;
         } else {
             JS_ASSERT(isBlock == 1);
  
I plan to try to land the new patch on Tuesday.

Comment 21

[Kent James (:rkent)]

Here's my stack trace. I am running on Windows XP (Visual Studio, not gdb).:

	js3250.dll!JS_Assert(const char * s=0x00691734, const char * file=0x00691708, int ln=0x0000004f)  Line 65	C++
 	js3250.dll!js_GetMutableScope(JSContext * cx=0x02ff1600, JSObject * obj=0x02463740)  Line 79 + 0x26 bytes	C++
 	js3250.dll!js_SetPropertyHelper(JSContext * cx=0x02ff1600, JSObject * obj=0x02463740, int id=0x047438a4, int cacheResult=0x00000001, int * vp=0x0012ec58)  Line 4594 + 0xd bytes	C++
 	js3250.dll!js_Interpret(JSContext * cx=0x02ff1600)  Line 4729 + 0x1a bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x02ff1600, unsigned int argc=0x00000003, int * vp=0x05a45100, unsigned int flags=0x00000000)  Line 1352 + 0x9 bytes	C++
 	js3250.dll!array_extra(JSContext * cx=0x02ff1600, ArrayExtraMode mode=FOREACH, unsigned int argc=0x00000003, int * vp=0x05a450dc)  Line 3051 + 0x13 bytes	C++
 	js3250.dll!array_forEach(JSContext * cx=0x02ff1600, unsigned int argc=0x00000001, int * vp=0x05a450dc)  Line 3107 + 0x13 bytes	C++
 	js3250.dll!js_Interpret(JSContext * cx=0x02ff1600)  Line 5088 + 0x17 bytes	C++
 	js3250.dll!js_Execute(JSContext * cx=0x02ff1600, JSObject * chain=0x04540be0, JSScript * script=0x0889e1f0, JSStackFrame * down=0x00000000, unsigned int flags=0x00000000, int * result=0x00000000)  Line 1578 + 0x9 bytes	C++
 	js3250.dll!JS_EvaluateUCScriptForPrincipals(JSContext * cx=0x02ff1600, JSObject * obj=0x04540be0, JSPrincipals * principals=0x01ef9fbc, const unsigned short * chars=0x0313cf00, unsigned int length=0x00000029, const char * filename=0x08a38b00, unsigned int lineno=0x000000d1, int * rval=0x00000000)  Line 5143 + 0x19 bytes	C++
 	gklayout.dll!nsJSContext::EvaluateString(const nsAString_internal & aScript={...}, void * aScopeObject=0x04540be0, nsIPrincipal * aPrincipal=0x01ef9fb8, const char * aURL=0x08a38b00, unsigned int aLineNo=0x000000d1, unsigned int aVersion=0x00000000, nsAString_internal * aRetValue=0x00000000, int * aIsUndefined=0x0012f6f8)  Line 1603 + 0x42 bytes	C++
 	gklayout.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout=0x08a38b58)  Line 7763 + 0xb3 bytes	C++
 	gklayout.dll!nsGlobalWindow::TimerCallback(nsITimer * aTimer=0x08a38bc8, void * aClosure=0x08a38b58)  Line 8119	C++
 	xpcom_core.dll!nsTimerImpl::Fire()  Line 420 + 0xe bytes	C++
 	xpcom_core.dll!nsTimerEvent::Run()  Line 514	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012f850)  Line 511	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00cc22b8, int mayWait=0x00000001)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 170 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 193 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=0x00000002, char * * argv=0x00c9cac0, const nsXREAppData * aAppData=0x00c9cfc8)  Line 3298 + 0x25 bytes	C++
 	thunderbird.exe!NS_internal_main(int argc=0x00000002, char * * argv=0x00c9cac0)  Line 103 + 0x12 bytes	C++
 	thunderbird.exe!wmain(int argc=0x00000002, unsigned short * * argv=0x00c9c9f8)  Line 110 + 0xd bytes	C++
 	thunderbird.exe!__tmainCRTStartup()  Line 583 + 0x19 bytes	C
 	thunderbird.exe!wmainCRTStartup()  Line 403	C
 	kernel32.dll!7c817077()

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #20)
> Created an attachment (id=375755) [details]
> v4

I couldn't apply this patch to mozilla-1.9.1 to test rkent's assertion:

$ patch -p1 --dry-run < ~/Desktop/attachment.cgi 
patching file js/src/jsapi.cpp
patching file js/src/jsfun.cpp
patching file js/src/jsfun.h
patching file js/src/jsinterp.cpp
Hunk #1 succeeded at 922 (offset -34 lines).
Hunk #2 succeeded at 973 (offset -34 lines).
Hunk #3 FAILED at 4571.
1 out of 3 hunks FAILED -- saving rejects to file js/src/jsinterp.cpp.rej
patching file js/src/jsobj.cpp
Hunk #1 succeeded at 2390 (offset -16 lines).
Hunk #2 succeeded at 2415 (offset -16 lines).
Hunk #3 succeeded at 2543 (offset -16 lines).
Hunk #4 succeeded at 2657 (offset -16 lines).
Hunk #5 succeeded at 4236 (offset -10 lines).
Hunk #6 succeeded at 4270 (offset -10 lines).
patching file js/src/jsobj.h
Hunk #1 succeeded at 423 (offset -6 lines).
Hunk #2 succeeded at 467 (offset -6 lines).
patching file js/src/jsproto.tbl
patching file js/src/jsregexp.cpp
Hunk #1 succeeded at 4533 (offset -21 lines).
Hunk #2 succeeded at 4573 (offset -21 lines).
Hunk #3 succeeded at 4597 (offset -21 lines).
patching file js/src/jsregexp.h
Hunk #1 succeeded at 167 (offset -8 lines).
patching file js/src/jsscope.cpp
patching file js/src/jsscript.cpp
patching file js/src/jsxdrapi.h
patching file js/src/jsxml.cpp
patching file js/src/jsxml.h
Hunk #1 succeeded at 260 (offset 1 line).

Comment 23

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/69bd30151a50

Comment 24

[Igor Bukanov]

(In reply to comment #22)
> (In reply to comment #20)
> > Created an attachment (id=375755) [details] [details]
> > v4
> 
> I couldn't apply this patch to mozilla-1.9.1 to test rkent's assertion:

I presume this comes from the absence of the bug 490666 on 1.9.1.

Comment 25

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/69bd30151a50

Comment 26

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/95efce4f46a4