350793 - for-in loops must be yieldable (was "Assertion failure: map->nrefs > 0" [@ js_DropObjectMap] during GC)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

I hit this while running the fuzzer in bug 349611, but I don't have a testcase.

Assertion failure: map->nrefs > 0, 
at jsobj.c:2207 [@ js_DropObjectMap]

The output just before the assertion may or may not have been relevant:

9016: "var \u3059 =  (get-=function(id) { return id }) (\nfunction  () { true; })((window(<><x><y/></x></>)));"
Running threw: TypeError: get is not a function

Comment 1

[Jesse Ruderman]

It happened again, and this time I got a stack trace.

(gdb) bt
#0  0x01108db8 in JS_Assert (s=0x113bd80 "map->nrefs > 0", file=0x113b96c "/Users/admin/trunk/mozilla/js/src/jsobj.c", ln=2207) at /Users/admin/trunk/mozilla/js/src/jsutil.c:58
#1  0x010ad2e4 in js_DropObjectMap (cx=0x22d1ea90, map=0x24bf9e60, obj=0x2e97b50) at /Users/admin/trunk/mozilla/js/src/jsobj.c:2207
#2  0x010af63c in js_FinalizeObject (cx=0x22d1ea90, obj=0x2e97b50) at /Users/admin/trunk/mozilla/js/src/jsobj.c:2672
#3  0x0106f6fc in js_GC (cx=0x22d1ea90, gckind=GC_NORMAL) at /Users/admin/trunk/mozilla/js/src/jsgc.c:2779
#4  0x010199f0 in JS_GC (cx=0x22d1ea90) at /Users/admin/trunk/mozilla/js/src/jsapi.c:1938
#5  0x01019a9c in JS_MaybeGC (cx=0x22d1ea90) at /Users/admin/trunk/mozilla/js/src/jsapi.c:2010
#6  0x08f0e124 in nsJSContext::DOMBranchCallback (cx=0x22d1ea90, script=0x24ba9d60) at /Users/admin/trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:689
#7  0x01077944 in js_Interpret (cx=0x22d1ea90, pc=0x24ba9da1 "\005??`\003c\nc\020", result=0xbfffe2c4) at /Users/admin/trunk/mozilla/js/src/jsinterp.c:2338
#8  0x01074010 in js_Invoke (cx=0x22d1ea90, argc=1, flags=2) at /Users/admin/trunk/mozilla/js/src/jsinterp.c:1369
#9  0x010744ac in js_InternalInvoke (cx=0x22d1ea90, obj=0x2c3cbf0, fval=47781440, flags=0, argc=1, argv=0x24bfed00, rval=0xbfffe4fc) at /Users/admin/trunk/mozilla/js/src/jsinterp.c:1448
#10 0x01024210 in JS_CallFunctionValue (cx=0x22d1ea90, obj=0x2c3cbf0, fval=47781440, argc=1, argv=0x24bfed00, rval=0xbfffe4fc) at /Users/admin/trunk/mozilla/js/src/jsapi.c:4419
#11 0x08f12be8 in nsJSContext::CallEventHandler (this=0x227ff8f0, aTarget=0x24b03f70, aScope=0x2c3cbf0, aHandler=0x2d91640, aargv=0x22fea134, arv=0xbfffe648) at /Users/admin/trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:1740
#12 0x08f41114 in nsGlobalWindow::RunTimeout (this=0x24b03f70, aTimeout=0x24bc3bd0) at /Users/admin/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:6573
#13 0x08f416a8 in nsGlobalWindow::TimerCallback (aTimer=0x24bc3c10, aClosure=0x24bc3bd0) at /Users/admin/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:6900
#14 0x013bbb6c in nsTimerImpl::Fire (this=0x24bc3c10) at /Users/admin/trunk/mozilla/xpcom/threads/nsTimerImpl.cpp:383
#15 0x013bbdd4 in nsTimerEvent::Run (this=0x22fff0e0) at /Users/admin/trunk/mozilla/xpcom/threads/nsTimerImpl.cpp:456
#16 0x013b6134 in nsThread::ProcessNextEvent (this=0x2114740, mayWait=1, result=0xbfffe8d8) at /Users/admin/trunk/mozilla/xpcom/threads/nsThread.cpp:482
#17 0x013358f4 in NS_ProcessNextEvent_P (thread=0x2114740, mayWait=1) at nsThreadUtils.cpp:225
#18 0x05d8a3d8 in nsBaseAppShell::Run (this=0x21393a0) at /Users/admin/trunk/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:153
#19 0x069b2828 in nsAppStartup::Run (this=0x214e870) at /Users/admin/trunk/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:171
#20 0x002103b4 in XRE_main (argc=1, argv=0xbfffee68, aAppData=0x3070) at /Users/admin/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:2465
#21 0x00002c78 in main (argc=1, argv=0xbfffee68) at /Users/admin/trunk/mozilla/browser/app/nsBrowserApp.cpp:61

Comment 2

[Jesse Ruderman]

Attachment: small testcase that crashes or asserts during GC about 40% of the time

Comment 3

[Jesse Ruderman]

I'm not sure this is the smallest possible testcase, but it should be small enough.

Comment 4

[Robert Sayre]

Attachment: stack when windows complains about heap corruption

My Windows debug build repeatedly asserts with heap corruption in JS_Free() at this line:

> LOCKED_OBJ_GET_CLASS(obj)->finalize(cx, obj);

"HEAP CORRUPTION DETECTED at ... [memory addresses]"
"CRT detected that the application wrote to memory after end of heap buffer."

This is just above the line in Jesse's stack.

Comment 5

[Robert Sayre]

Ugh. In generator_finalize, it looks like map.nslots and map.freeslot are wrong. VS claims there are only 5 slots.

> js3250.dll!generator_finalize(JSContext * cx=0x0433f3f0, JSObject * obj=JSObject [5 slots])  Line 589 + 0xd bytes	C

		JSGEN_CLOSED	4	int
		JSGEN_NEWBORN	0	int
		JSGEN_OPEN	1	int
+		cx	0x0433f3f0 {links={...} interpLevel=0 stackLimit=718568 ...}	JSContext *
-		gen	0x05da6618 {next=0x00000000 obj=JSObject [5 slots] state=JSGEN_CLOSED ...}	JSGenerator *
+		next	0x00000000 {next=??? obj=null state=??? ...}	JSGenerator *
-		obj	JSObject [5 slots]	JSObject *
-		[raw members]	{map=0x046bf940 slots=0x04454994 }	JSObject
-		map	0x046bf940 {nrefs=2 ops=0x00554518 nslots=12 ...}	JSObjectMap *
		nrefs	2	long
+		ops	0x00554518 _js_ObjectOps {newObjectMap=0x00421eb5 destroyObjectMap=0x004214ba lookupProperty=0x00421b45 ...}	JSObjectOps *
		nslots	12	unsigned long
		freeslot	9	unsigned long
-		slots	0x04454994	long *
			67503408	long
		[0]	67503408	long
		[1]	66135688	long
		[2]	5579297	long
		[3]	98199065	long
		[4]	-2147483647	long
		state	JSGEN_CLOSED	JSGeneratorState
+		frame	{callobj=null argsobj=null varobj=null ...}	JSStackFrame
+		arena	{next=0x05359ed8 base=98199184 limit=98199240 ...}	JSArena
+		stack	0x05da6690	long [1]
		gen->state	JSGEN_CLOSED	JSGeneratorState

Comment 6

[Brendan Eich [:brendan]]

Robert, can you reproduce this at will? If so, find me on IRC in a half hour or whenever you have time, and let's debug.

/be

Comment 7

[Robert Sayre]

(In reply to comment #6)
> Robert, can you reproduce this at will?

Yeah, I can at least get the heap corruption warnings at will with an 09/07 Windows build. Refreshing the test case on Windows and Mac seems to get it to trigger pretty easily.

Comment 8

[Mike Moening]

I can reproduce at will too.
The heap corruption is occuring in generator_finalize on this line:

        JS_free(cx, gen);

That's at least according to boundschecker. I'm not seeing a double free or anything obvious. I'll debug some more later and see if I can find it.

Comment 9

[Mike Moening]

More info from boundschecker...

Apparently inside do_forinloop:

the "flags" variable is not intialized before this call in jsinterp.c:

flags |= js_GetNativeIteratorFlags(cx, iterobj);

I doubt this intented. I'm betting its part of the problem.
If anyone can suggest the best place to initialize the variable I'll give it a try and see if makes any difference.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> More info from boundschecker...
> 
> Apparently inside do_forinloop:
> 
> the "flags" variable is not intialized before this call in jsinterp.c:
> 
> flags |= js_GetNativeIteratorFlags(cx, iterobj);

Which line is that on?

flags is initialized to 0 by JSOP_STARTITER, before the loop.  If a for each loop is used, it's reset to JSITER_FOREACH every time through the loop, just before the JSOP_FOR* opcode.

> I doubt this intented. I'm betting its part of the problem.
> If anyone can suggest the best place to initialize the variable I'll give it a
> try and see if makes any difference.

Does boundschecker say anything else?  Can you break in the debugger when flags seems to boundschecker to be uninitialized, and display script->filename, also script->lineno and pc - script->main?  Thanks.

I can't reproduce this in the js shell.

/be

Comment 11

[Brendan Eich [:brendan]]

Oh for crying out loud!  Generators can be resumed with flags unrestored.

/be

Comment 12

[Brendan Eich [:brendan]]

Reproduced it in the shell just now...  Fix today.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: fix

The JSOP_FOR{IN,EACH,EACHKEYVAL} prefix bytecodes appear at the loop-closing jump target, the top of the loop in code generation terms.  There's no way to yield in between one of these prefixes and the JOF_FOR-formatted opcode that immediately follows it (JSOP_FOR{NAME,ARG,VAR,LOCAL,PROP,ELEM}).

/be

Comment 14

[Brendan Eich [:brendan]]

Fixed on trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.204; previous revision: 3.203
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.284; previous revision: 3.283
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.70; previous revision: 3.69
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.233; previous revision: 3.232
done
Checking in jsxdrapi.h;
/cvsroot/mozilla/js/src/jsxdrapi.h,v  <--  jsxdrapi.h
new revision: 1.21; previous revision: 1.20
done

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 237524 [details] [diff] [review]
fix

The fix looks bigger than it is -- we already have prefix bytecodes to handle flag-setting needed by for each (v in o) and for ([k,v] in o) loops.  The standard for (k in o) loop needs one too.

/be

Comment 16

[Mike Schroepfer]

Comment on attachment 237524 [details] [diff] [review]
fix

a=schrep

Comment 17

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: fix regression

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 237582 [details] [diff] [review]
fix regression

Many thanks to Gavin for helping find this.  I'm likely to check it in as soon as he vouches for it.

/be

Comment 20

[Brendan Eich [:brendan]]

I checked the regression fix into trunk and 1.8 branch.

/be

Comment 21

[Mike Schroepfer]

Comment on attachment 237582 [details] [diff] [review]
fix regression

marking flags to match cvs state.

Comment 22

[Jesse Ruderman]

This still crashes for me:

./js -v 170

gen = function() { for(let y in [5,6,7,8]) yield ({}); };
for (let it in gen()) ;
gc();

Comment 23

[Mike Moening]

Two things:

1) When I run your script after pulling latest CVS I get the following error at compile time:
-------------------------
SyntaxError: missing ; after for-loop initializer
Line Source:
  function gen() { for(let itty in [5,6,7,8]) yield ({ }); }
-------------------------

2) The corruption appears to be from the error handler. Here's what boundschecker says:

"Dangling Pointer: Pointer 0x046BD3DC, references local variable stmtInfo which has gone out of scope from function Statement."

tc->topStmt appears to be garbage memory.

------------
void
js_PopStatement(JSTreeContext *tc)
{
    JSStmtInfo *stmt=NULL;
    JSObject *blockObj;

    stmt = tc->topStmt;  <---- COMPLAINS HERE
------------

BC says it was deallocated here:
-----------------------
  /* Parse the loop condition or null into pn2. */
  MUST_MATCH_TOKEN(TOK_SEMI, JSMSG_SEMI_AFTER_FOR_INIT);
-----------------------

Are you popping twice? Just a guess.

Comment 24

[Jesse Ruderman]

> 1) When I run your script after pulling latest CVS I get the following 
> error at compile time:
> SyntaxError: missing ; after for-loop initializer

To make the JavaScript engine recognize "let" as a keyword, you need to do one of the following:
* call "version(170)" in the shell
* use "./js -v 170" to run the shell
* use <script type="application/javascript;version=1.7"> in HTML/XUL.

See 351515.

Comment 25

[Mike Moening]

>To make the JavaScript engine recognize "let" as a keyword, you need to do one
>of the following:
>* call "version(170)" in the shell
>* use "./js -v 170" to run the shell
>* use <script type="application/javascript;version=1.7"> in HTML/XUL.
>
>See 351515.

I'm running 1.7 in my HTTP Server embeddding (no jsshell or browser).
JS_VERSION is set to and is reporting 170.
JS_HAS_GENERATORS is set to 1 and js_InitIteratorClasses() is being called during JS_InitStandardClasses.
So why is your script not compiling?

Either way that fix is still corrupting memory.
An error in the error handler I suppose.
Somebody needs to fix that.

Comment 26

[Mike Moening]

Jesse,

There have been a few checkin's on jsparse.c in the last day or so.
Looks like they broke your script (if its supposed to be valid for JS 1.7).
Get the latest JS and try again.
Let me know if I'm all wet...

Comment 27

[Jesse Ruderman]

My script compiles if I run with "-v 170", and fails to compile with "missing ; after for-loop initializer" if I do not run with "-v 170".  Note that JS_HAS_GENERATORS is something that's true or not when you *compile* the JavaScript engine, while "-v 170" and equivalents are run-time switches that affect how individual scripts are compiled.

Comment 28

[Mike Moening]

I added 
   JS_SetVersion(cx, JSVERSION_1_7);
to my compiled embedding.

(Why compile time JS 1.7 is different than setting JS_SetVersion(cx, JSVERSION_1_7)  makes no sense to me. I would bet that most folks would expect them to work the same...)

After JS_SetVersion, it now corrupts heap and dies like you said.  
Either way that other corruption needs fixing too!

I'll let you know what I find in a bit...

Comment 29

[Mike Moening]

First off, the gen->next member is not initialized to NULL in js_NewGenerator().

BC told me the memory for the generator was being stomped on.
But it didn't say where!

Purify found it.  

PUSH(JSVAL_VOID); is overwriting the generator.

Here's the error as purify is reporting it.
----------------------------------------------

    Writing 4 bytes to 0x03ecc3c0 (4 bytes at 0x03ecc3c0 illegal)
    Address 0x03ecc3c0 is 1 byte past the end of a 176 byte block at 0x03ecc310
    Address 0x03ecc3c0 points to a malloc'd block in heap 0x036f0000
    Thread ID: 0x75c
    Error location
        js_Invoke      [js\src\jsinterp.c:1323]
            
                    /* Push void to initialize missing args. */
                    do {
         =>             PUSH(JSVAL_VOID);
                    } while (--nslots != 0);
                }
                JS_ASSERT(nslots == 0);
        js_InvokeConstructor [js\src\jsinterp.c:1933]
        js_Interpret   [js\src\jsinterp.c:3531]
        SendToGenerator [js\src\jsiter.c:765]
        generator_op   [js\src\jsiter.c:890]
        generator_next [js\src\jsiter.c:913]
        js_Invoke      [js\src\jsinterp.c:1372]
        js_InternalInvoke [js\src\jsinterp.c:1470]
        js_CallIteratorNext [js\src\jsiter.c:539]
        js_Interpret   [js\src\jsinterp.c:2751]
        js_Execute     [js\src\jsinterp.c:1621]
        JS_ExecuteScript [js\src\jsapi.c:4262]
-----------------------

Let me know when you have a patch and I'll run it through again.

Comment 30

[Igor Bukanov]

(In reply to comment #29)
> First off, the gen->next member is not initialized to NULL in
> js_NewGenerator().

But that is OK as that member is accessed only through GC functions after the gen->next is initialized in js_RegisterOpenGenerator.

Comment 31

[Igor Bukanov]

I could not reproduce the crash with the latest trunk debug build on Linux. Guys, could you confirm that you still have the crash?

Comment 32

[Robert Sayre]

(In reply to comment #31)
> I could not reproduce the crash with the latest trunk debug build on Linux.
> Guys, could you confirm that you still have the crash?
> 

Still crashes for me on mac:

~/spidermonkey/mozilla/js/src> Darwin_DBG.OBJ/js -v 170
js> version()
170
js> gen = function() { for(let y in [5,6,7,8]) yield ({}); };
function () {
    for (let y in [5, 6, 7, 8]) {
        (yield {});
    }
}
js> for (let it in gen()) ;
js> gc();
Assertion failure: map->nrefs > 0, at jsobj.c:2211
Trace/BPT trap
~/spidermonkey/mozilla/js/src> 

Comment 33

[Brian Crowder]

Here's what valgrind says  (similar to MikeM's purify report in comment #29):

==23264== Invalid write of size 4
==23264==    at 0x8093C4D: js_Invoke (jsinterp.c:1323)
==23264==    by 0x8095390: js_InvokeConstructor (jsinterp.c:1929)
==23264==    by 0x809F5CD: js_Interpret (jsinterp.c:3533)
==23264==    by 0x80B30A4: SendToGenerator (jsiter.c:764)
==23264==    by 0x80B35D9: generator_op (jsiter.c:889)
==23264==    by 0x80B365A: generator_next (jsiter.c:912)
==23264==    by 0x8093E2C: js_Invoke (jsinterp.c:1372)
==23264==    by 0x8094208: js_InternalInvoke (jsinterp.c:1466)
==23264==    by 0x80B2A89: js_CallIteratorNext (jsiter.c:537)
==23264==    by 0x809813E: js_Interpret (jsinterp.c:2751)
==23264==    by 0x80946F3: js_Execute (jsinterp.c:1617)
==23264==    by 0x805E34F: JS_ExecuteScript (jsapi.c:4256)
==23264==  Address 0x403FDE8 is 0 bytes after a block of size 176 alloc'd
==23264==    at 0x40044F5: malloc (vg_replace_malloc.c:149)
==23264==    by 0x8059644: JS_malloc (jsapi.c:1665)
==23264==    by 0x80B2D6C: js_NewGenerator (jsiter.c:643)
==23264==    by 0x80B0DED: js_Interpret (jsinterp.c:6117)
==23264==    by 0x80946F3: js_Execute (jsinterp.c:1617)
==23264==    by 0x805E34F: JS_ExecuteScript (jsapi.c:4256)
==23264==    by 0x8049522: Process (js.c:229)
==23264==    by 0x8049C12: ProcessArgs (js.c:434)
==23264==    by 0x804DBA8: main (js.c:3086)

Comment 34

[Brian Crowder]

Might be worth mentioning, luckily or unluckily, this doesn't crash for me on linux.  But it does crash on OS X

Comment 35

[Robert Sayre]

(In reply to comment #34)
> Might be worth mentioning, luckily or unluckily, this doesn't crash for me on
> linux.  But it does crash on OS X

No crash for me on Windows.

Comment 36

[Mike Moening]

I doesn't matter if it crashes or not.  It needs to be fixed.
If JS is corrupting the heap, we are going to get crashes showing up in different places as the code evolves.

I hacked the js_newGenerator function by changing:

    /* Allocate obj's private data struct. */
    gen = (JSGenerator *)
          JS_malloc(cx, sizeof(JSGenerator) + (nslots - 1) * sizeof(jsval));

to:

    /* Allocate obj's private data struct. */
    gen = (JSGenerator *)
          JS_malloc(cx, sizeof(JSGenerator) + (nslots) * sizeof(jsval));


My thinking is that we are going off the end by 1 slot...so I added another.
This hack works but I doubt its the correct was to fix the problem.

The answer is in comment #29.
Somebody smarter than me (who knows the slots stuff) needs to look at this.

Comment 37

[Brian Crowder]

I'm not sure MikeM's hack is a hack at all.  It definitely looks like we simply need more space for this PUSH operation.  Or is the bug that we're consuming a stack slot somewhere that we shouldn't be?

Comment 38

[Brendan Eich [:brendan]]

Attachment: fix the other bug that was hiding here

The stack pool manipulation was borken.  Fixed with extra asserts(tm)!

/be

Comment 39

[Brendan Eich [:brendan]]

MikeM can find workarounds for specific traces that the bug bites, he's good at that (and I mean that as a compliment -- it helps point toward the real bug).  But there's nothing wrong with the generator allocation size calc.  The problem was a basic violation of the rules of JSArenaPool used as a stack:

pool->current->next == NULL
pool->current can be reached started at pool->first.next and following next

/be

Comment 40

[Mike Moening]

>MikeM can find workarounds for specific traces that the bug bites, he's good at
>that (and I mean that as a compliment -- it helps point toward the real bug).
Hey, I'm better an finding them than fixing them.
I didn't expect you to actually my USE my patch.
Did you really expect me to fix that bug the "right way"?
That fix was SO FAR over my head, its sad.  We need the big guns for patches like this!

I'll test your patch and let you know the results.

Comment 41

[Mike Moening]

Your last patch fixed the heap corruption.

After a purify run and a few quick executions I get the IPW below.
I'm not sure what to make of this.
It might be a false positive.

Let me know what you think.

    [E] IPW: Invalid pointer write in JS_ArenaAllocate {3 occurrences}
        Writing 4 bytes to 0x057a005c (4 bytes at 0x057a005c illegal)
        Address 0x057a005c points into a committed VirtualAlloc'd block 
    Thread ID: 0x7e0
    Error location
        JS_ArenaAllocate [js\src\jsarena.c:208]
                }
            
                p = (void *)a->avail;
         =>     a->avail += nb;
                JS_ASSERT(a->base <= a->avail && a->avail <= a->limit);
                return p;
            }
        js_AllocRawStack [js\src\jsinterp.c:339]
            
                if (markp)
                    *markp = JS_ARENA_MARK(&cx->stackPool);
         =>     JS_ARENA_ALLOCATE_CAST(sp, jsval *, &cx->stackPool, nslots * sizeof(jsval));
                if (!sp) {
                    JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_STACK_OVERFLOW,
                                         (cx->fp && cx->fp->fun)
        js_Invoke      [js\src\jsinterp.c:1299]
        js_InvokeConstructor [js\src\jsinterp.c:1933]
        js_Interpret   [js\src\jsinterp.c:3531]
        SendToGenerator [js\src\jsiter.c:767]
        generator_op   [js\src\jsiter.c:892]
        generator_next [js\src\jsiter.c:915]
        js_Invoke      [js\src\jsinterp.c:1372]
        js_InternalInvoke [js\src\jsinterp.c:1470]
        js_CallIteratorNext [js\src\jsiter.c:539]
        js_Interpret   [js\src\jsinterp.c:2751]
        js_Execute     [js\src\jsinterp.c:1621]
        JS_ExecuteScript [js\src\jsapi.c:4262]

Brendan?

Comment 42

[Brendan Eich [:brendan]]

(In reply to comment #40)
> >MikeM can find workarounds for specific traces that the bug bites, he's good at
> >that (and I mean that as a compliment -- it helps point toward the real bug).
> Hey, I'm better an finding them than fixing them.
> I didn't expect you to actually my USE my patch.

You're too modest -- you zeroed in on bug 351717 against all odds (and by poring through an unbelievable amount of log info, I bet!).

Not sure what I think of that IPW report from purify.  Does the app continue to run?  Does boundschecker concur?

/be

Comment 43

[Brian Crowder]

MikeM:  My line numbers for jsarena.c don't seem to match up well with yours.  Do you have some local patches applied to it, or are you using an older version, perhaps?  (if someone else does match up, and I'm the one smoking crack, please let me know)

Comment 44

[Mike Moening]

>Do you have some local patches applied to it, or are you using an older
>version, perhaps? 

I think I may have some debug code still in there from other bugs.
Sorry. I usually include the actual offending code so line numbers don't get in the way.

Comment 45

[Mike Moening]

Boundscheck is OK about that IPW that purify found.

However it DID find another error that I beleive is a small problem.
I hit it by accident after loading up the wrong script for execution.

Uninitialized Memory Read: Pointer 0x03BDEC74 (4) refers to uninitialized data in block 0x03BDE8A0 (1047) allocated by malloc.

Here's the code where the error occurs:
------------------------------------
static jssrcnote
SrcNoteForPropOp(JSParseNode *pn, JSOp op)
{
    return ((op == JSOP_GETMETHOD &&
             !(pn->pn_attrs & JSPROP_IMPLICIT_FUNCTION_NAMESPACE)) ||
            op == JSOP_SETMETHOD)
           ? SRC_PCDELTA
           : SRC_PCBASE;
}
------------------------------------------
There is not much in that function.  Hopefully you can find it.

Here's the stack on how we got to this error:

---------------------------------------------
>	js32d.dll!SrcNoteForPropOp(JSParseNode * pn=0x03bdec58, JSOp op=JSOP_GETMETHOD)  Line 2283 + 0x3d bytes	C
 	js32d.dll!EmitPropOp(JSContext * cx=0x03bb87b0, JSParseNode * pn=0x03bdec58, JSOp op=JSOP_GETMETHOD, JSCodeGenerator * cg=0x0439eca0)  Line 2349 + 0xf6 bytes	C
 	js32d.dll!js_EmitTree(JSContext * cx=0x03bb87b0, JSCodeGenerator * cg=0x0439eca0, JSParseNode * pn=0x03bdec58)  Line 5659 + 0x9e bytes	C
 	js32d.dll!js_EmitTree(JSContext * cx=0x03bb87b0, JSCodeGenerator * cg=0x0439eca0, JSParseNode * pn=0x03bdec80)  Line 5698 + 0x77 bytes	C
 	js32d.dll!js_EmitTree(JSContext * cx=0x03bb87b0, JSCodeGenerator * cg=0x0439eca0, JSParseNode * pn=0x03bdee18)  Line 5056 + 0x77 bytes	C
 	js32d.dll!Statements(JSContext * cx=0x03bb87b0, JSTokenStream * ts=0x03bde8b0, JSTreeContext * tc=0x0439eca0)  Line 1472 + 0x17e bytes	C
 	js32d.dll!js_CompileTokenStream(JSContext * cx=0x03bb87b0, JSObject * chain=0x03b970e8, JSTokenStream * ts=0x03bde8b0, JSCodeGenerator * cg=0x0439eca0)  Line 501 + 0x77 bytes	C
 	js32d.dll!CompileTokenStream(JSContext * cx=0x03bb87b0, JSObject * obj=0x03b970e8, JSTokenStream * ts=0x03bde8b0, void * tempMark=0x03bb87f8, int * eofp=0x00000000)  Line 3848 + 0x7e bytes	C
 	js32d.dll!JS_CompileUCScriptForPrincipals(JSContext * cx=0x03bb87b0, JSObject * obj=0x03b970e8, JSPrincipals * principals=0x00000000, const unsigned short * chars=0x03ba0d90, unsigned int length=29024, const char * filename=0x03bde578, unsigned int lineno=1)  Line 3943 + 0x9f bytes	C
 	js32d.dll!JS_CompileUCScript(JSContext * cx=0x03bb87b0, JSObject * obj=0x03b970e8, const unsigned short * chars=0x03ba0d90, unsigned int length=29024, const char * filename=0x03bde578, unsigned int lineno=1)  Line 3911 + 0xc5 bytes	C
---------------------------------------------


Here's what the pn variable looks like in memory:
(I expanded the pn_ts so you could see what was being parsed).
---------------------------------------------------------

-		pn	0x03bdec58 {pn_type=22 pn_op='¸' pn_arity='ÿ' ...}	JSParseNode *
		pn_type	22	unsigned short
		pn_op	184 '¸'	unsigned char
		pn_arity	-1 'ÿ'	char
+		pn_pos	{begin={...} end={...} }	JSTokenPos
		pn_offset	0	int
+		pn_u	{func={...} list={...} ternary={...} ...}	<unnamed-tag>
+		pn_next	0x03bdedf0 {pn_type=31 pn_op='=' pn_arity=0 ...}	JSParseNode *
-		pn_ts	0x03bde8b0 {tokens=0x03bde8b0 cursor=3 lookahead=0 ...}	JSTokenStream *
+		tokens	0x03bde8b0 {type=TOK_LP pos={...} ptr=0x03bde9a4 "("<!DOCTYPE HTML PUBLIC>\r\n<html>\r\n<head>\r\n");
" ...}	JSToken [4]
		cursor	3	unsigned int
		lookahead	0	unsigned int
		lineno	2	unsigned int
		ungetpos	0	unsigned int
+		ungetbuf	0x03bde920 "("	unsigned short [6]
		flags	168	unsigned int
		linelen	66	int
		linepos	0	int
+		linebuf	{base=0x03bde988 "response.write("<!DOCTYPE HTML PUBLIC>\r\n<html>\r\n<head>\r\n");
" limit=0x03bdea0c "" ptr=0x03bdea0a "
" }	JSTokenBuf
+		userbuf	{base=0x03ba0d90 "
response.write("<!DOCTYPE HTML PUBLIC>\r\n<html>\r\n<head>\r\n");

response.write("\r\n\r\n");



 if(!page.JSON)
 {
   JSON = 
   {
     _fixStringReplacements :  { '"'  : '\\"', "\\" : "\\\\", "\n" : "\\n", "\r" : "\\r", "\b" : "\\b", "\t" : "\\t", "\f" : "\\f" },  
    
     toJSONString : function(oObj)
     {
       var str = "";
       switch(typeof(oObj))
       {
         case "boolean":
         case "number":
           return oObj;
         case "string":
           if(/[\" limit=0x03baf050 "" ptr=0x03ba0e16 "
response.write("\r\n\r\n");



 if(!page.JSON)
 {
   JSON = 
   {
     _fixStringReplacements :  { '"'  : '\\"', "\\" : "\\\\", "\n" : "\\n", "\r" : "\\r", "\b" : "\\b", "\t" : "\\t", "\f" : "\\f" },  
    
     toJSONString : function(oObj)
     {
       var str = "";
       switch(typeof(oObj))
       {
         case "boolean":
         case "number":
           return oObj;
         case "string":
           if(/[\"\\\x00-\x1f]/.test(oObj	JSTokenBuf
+		tokenbuf	{base=0x03bdebb0 "<!DOCTYPE HTML PUBLIC>
<html>
<head>
&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#65339;" limit=0x03bdec2e "&#64507;&#65339;" ptr=0x03bdec00 "&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#65339;" ...}	JSStringBuffer
+		filename	0x03bde578 "via/shapes.mwsp"	const char *
+		file	0x00000000 {_ptr=??? _cnt=??? _base=??? ...}	_iobuf *
+		principals	0x00000000 {codebase=??? getPrincipalArray=??? globalPrivilegesEnabled=??? ...}	JSPrincipals *
		listener	0x00000000	void (const char *, unsigned int, unsigned short *, unsigned int, void * *, void *)*
		listenerData	0x00000000	void *
		listenerTSData	0x00000000	void *
+		saveEOL	0x00000000 <Bad Ptr>	unsigned short *


-----------------------

This might be better under its own bug. Sorry if I messed up this one.

Comment 46

[Brian Crowder]

MikeM:  If you could provide a smaller testcase in another bug for that, that would be great.

Comment 47

[Mike Moening]

I found the member that wan't initialized. There was a define that confused me the 1st time.  
Take a good look at pn_u->slot and pn_u->attrs below:


-		pn_u	{func={...} list={...} ternary={...} ...}	<unnamed-tag>
+		func	{funAtom=0x03bd9090 body=0x03bdec30 flags=4227595259 ...}	<unnamed-tag>
+		list	{head=0x03bd9090 tail=0x03bdec30 count=4227595259 ...}	<unnamed-tag>
+		ternary	{kid1=0x03bd9090 kid2=0x03bdec30 kid3=0xfbfbfbfb }	<unnamed-tag>
+		binary	{left=0x03bd9090 right=0x03bdec30 val=-67372037 }	<unnamed-tag>
+		unary	{kid=0x03bd9090 num=62778416 }	<unnamed-tag>
-		name	{atom=0x03bd9090 expr=0x03bdec30 slot=-67372037 ...}	<unnamed-tag>
+		atom	0x03bd9090 {entry={...} flags=0 number=376 }	JSAtom *
+		expr	0x03bdec30 {pn_type=29 pn_op=';' pn_arity='ÿ' ...}	JSParseNode *
		slot	-67372037	long
		attrs	4227595259	unsigned int

Comment 48

[Mike Moening]

>MikeM:  If you could provide a smaller testcase in another bug for that, that
>would be great.

I would like to but because I'm in an embedding it takes a long time to extract out the necessary pieces into something somebody else can compile and run. 
From past experience I've noticed that nobody ever bothers to compile my test programs anyway.

I'm betting a smarter man than me can find this one quickly given the info already provided.

I'll open another bug too.

Comment 49

[Brendan Eich [:brendan]]

(In reply to comment #48)
> From past experience I've noticed that nobody ever bothers to compile my test
> programs anyway.

I did compile your JSInterator test program; mrbkap and crowder are helpful types too, so don't give up before you've started.

/be

Comment 50

[Brendan Eich [:brendan]]

Fixed on trunk.  Igor, please feel free to review and ask for any revisions, I was going for double review both in case mrbkap was busy at university, and to get two pairs of eyes on the patch.  I'll nominate for branch approval when you review, or feel free to nominate it yourself if you r+.

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.287; previous revision: 3.286
done
Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.39; previous revision: 3.38
done

/be

Comment 51

[Igor Bukanov]

Comment on attachment 238142 [details] [diff] [review]
fix the other bug that was hiding here

The really weired thing s why on Linux this was not reproducible.

Comment 52

[Igor Bukanov]

The test case is reproducible on Linux, but it is necessary to set  MALLOC_CHECK_=2 when running the shell or MALLOC_CHECK_=1 to get more usable reports without aborting:

~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js -version 170 ~/s/test.js 
~/m/trunk/mozilla/js/src> MALLOC_CHECK_=1 ./Linux_All_DBG.OBJ/js -version 170 ~/s/test.js  
malloc: using debugging hooks
*** glibc detected *** free(): invalid pointer: 0x0816e788 ***
~/m/trunk/mozilla/js/src> MALLOC_CHECK_=2 ./Linux_All_DBG.OBJ/js -version 170 ~/s/test.js  
Aborted

I think running the test suite on Linux should be done with MALLOC_CHECK_=2 set.

Comment 53

[Mike Schroepfer]

Comment on attachment 238142 [details] [diff] [review]
fix the other bug that was hiding here

a=schrep for 181drivers

Comment 54

[Brendan Eich [:brendan]]

There should have been a separate bug for the problem fixed by attachment 238142 [details] [diff] [review] (I'm guilty of overloading and morphing too, so I'm first in promising to reform).  My needs-1.8.1-landing query missed this because it already had fixed1.8.1 for the earlier fix-patch.

Fixed "the other bug that was hiding here" on the 1.8 branch:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.60; previous revision: 3.181.2.59
done
Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.17.2.15; previous revision: 3.17.2.14
done

/be

Comment 55

[Brendan Eich [:brendan]]

Many thanks to Brian Crowder for valgrind and debug-malloc clues and tips that pointed to the generator stack maintenance fix.

/be

Comment 56

[Bob Clary [:bc] (inactive)]

current behavior (modulo whitespace):
expect:
 function ( ) { for ( let y in [ 5 , 6 , 7 , 8 ] ) yield ( { } ) ; } 

actual:
 function ( ) { for ( let y in [ 5 , 6 , 7 , 8 ] ) { yield { } ; } } 

Checking in regress-350793-01.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-350793-01.js,v  <--  regress-350793-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/block/regress-350793-02.js,v
done
Checking in regress-350793-02.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-350793-02.js,v  <--  regress-350793-02.js
initial revision: 1.1
done

(In reply to comment #52)
> The test case is reproducible on Linux, but it is necessary to set 
> MALLOC_CHECK_=2 when running the shell or MALLOC_CHECK_=1 to get more usable
> reports without aborting:
>

I normally run with MALLOC_CHECK_=2 and just reviewed my logs and haven't been seeing any messages with 'glibc detected'...

Comment 57

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux
no glibc messages detected.

Comment 58

[Bob Clary [:bc] (inactive)]

I missed the failure of js1_7/block/regress-350793-02.js when verifying just now. The actual decompiled value does not match the expected value in the test but the actual value looks ok to me: function ( ) { for ( let y in [ 5 , 6 , 7 , 8 ] ) { yield { } ; } } 

Checking in regress-350793-02.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-350793-02.js,v  <--  regress-350793-02.js
new revision: 1.2; previous revision: 1.1
done

Comment 59

[Brendan Eich [:brendan]]

Is the test still producing too much noise?

$ ./Darwin_DBG.OBJ/js -f ../tests/js1_7/shell.js -f ../tests/js1_7/regress/shell.js -f ../tests/js1_7/block/regress-350793-02.js
BUGNUMBER: 350793
STATUS: for-in loops must be yieldable
before 27696, after 27696, break 01008000
PASSED! for-in loops must be yieldable
expect:
 function ( ) { for ( let y in [ 5 , 6 , 7 , 8 ] ) { yield { } ; } } 
actual:
 function ( ) { for ( let y in [ 5 , 6 , 7 , 8 ] ) { yield { } ; } } 
PASSED! for-in loops must be yieldable: compare source

/be

Comment 60

[Bob Clary [:bc] (inactive)]

(In reply to comment #59)

noise as in failing ? no. noise as in more output than necessary? yes.