483783 - avmshell crash when creating large string

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Chris Peyer]

Attachment: Crash dump from windows rev 1535

as:
var myString:String = "";
for(var j:Number = 0; j < 3000000; j++) 
    myString += "a";

Problem only starts when string is near 3000000 characters long.  Introduced in tamarin redux changeset 1533 or 1534.

result build 1534 windows:
This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.

result build 1535 windows:
avmplus crash: exception 0xC0000005 occurred
Writing minidump crash log to avmplusCrash.dmp

result build 1534 mac:
Bus Error

QE Note: Need to add regression test to testsuite.

Comment 1

[Tommy Reilly]

This works for me on mac, its possible Erik fixed this when he fixed that other bug.   Suggest re-test on windows.

Comment 2

[Brent Baker]

Using the sample code provide in comment#0

windows 32 - crash
windows 64 - PASS
windows mobile - crash

mac 10.5 intel 32|64 - PASS
mac 10.5 PPC 32 - bus error
mac 10.5 PPC 64 - PASS

mac 10.4 intel - bus error
mac 10.4 PPC - bus error

linux 32|64 - PASS

Comment 3

[Tommy Reilly]

Cool, thanks Brent, I was able to repro on Win32, working on a fix.

Comment 4

[Tommy Reilly]

fixed in 1622

Comment 5

[Edwin Smith]

- please set the patch flag on patches to make diff readable in bugzilla
- review?  this doesn't look trivial.
- add bug # and reviewer to checkin comment (bug=xxx, r=woever+)

for trivial fixes, sometimes "r=me" is okay, but say so, and this doesn't look trivial.

Comment 6

[Tommy Reilly]

Attachment: patch

changeset:   1622:fef62d74de1a
tag:         tip
user:        Tommy Reilly <[e-mail]>
date:        Tue Mar 24 12:32:15 2009 -0400
summary:     Fix RemoveBlock, two bugs a) not setting blocksLen correctly and b) need_sentinel check was wrong

Comment 7

[Tommy Reilly]

sorry Ed, Erik's not here and its fixing some bugs in some bug fixing he did.  I asked Lars to review and posted the after the fact diff.  The actual fixes are just two lines, the diff is mostly new asserts/sanity checking code.

Comment 8

[Lars T Hansen]

Comment on attachment 369090 [details] [diff] [review]
patch

You should add an assertion to the validation function:

  (block->baseAddr != 0) == (block->size != 0)

This is assumed and in fact checked (in the last case) but not on the path where the fact is used (namely, where next is used to update block).  It would be useful to make it clearer and check it sooner.

Comment 9

[Tommy Reilly]

Attachment: fix remove block issues

Comment 10

[Tommy Reilly]

sandbox build results look almost good, one windows interp crash I need to investigate.

Comment 11

[Lars T Hansen]

Comment on attachment 369421 [details] [diff] [review]
fix remove block issues

I'm sure I don't understand why there has to be one big array covering all the regions, necessitating this shuffling, but we'll cross that bridge later.

In code close to this you need to ensure that 'committed' and 'dirty' are initialized when you create a new sentinel block.  A constructor on HeapBlock might be in order so that these bugs are harder to introduce.

Comment 12

[Tommy Reilly]

fixed in 1630

Comment 13

[Tommy Reilly]

Well you need some way to track what pages are in use/committed/freed that may span regions.  A bitmap for each continguous set of regions would suffice with two bits per (one to know if its in use and one to know if its committed).

Then you need a simple way to lookup free blocks.  A segregated power of two freelist would work nicely (each freelist entry would be a index into the bitmap and a region base addr maybe).   Since pages can be decommitted you can't store the freelist pointer there.  You could have a separate data structure to track uncommitted regions, might make sense given that in the !useVirtualMemory case there are no uncommitted regions.

HeapBlock does all this with one data structure which is nice but RemoveBlock is a bit of a sore spot.

Comment 14

[Lars T Hansen]

The reason I'm complaining is that the single, large array structure is not going to benefit non-VM systems (no region spanning) one hoot, it only induces fragmentation in the underlying memory allocator.  Hanging an array of HeapBlocks off each region data structure would work better in this case.

I don't want to turn this bug into a discussion of whether the interface from GCHeap to the underlying memory is appropriately abstract or not (I don't think it is), but I will start that discussion elsewhere.

Comment 15

[Tommy Reilly]

Blocks span regions on non-VM systems too, anything contiguous is treated as such.

Comment 16

[Lars T Hansen]

(In reply to comment #15)
> Blocks span regions on non-VM systems too, anything contiguous is treated as
> such.

All right.  (A recent change no doubt; until very recently blocksSpanRegions, now gone, was a VM-only setting.  Also until very recently, allocations in a non-VM system were always non-contiguous because blocks were allocated 4095 bytes too large so that we could be assured of alignment, so the point would be moot - it would never happen.)

Comment 17

[Tommy Reilly]

blocksSpanRegions was an recent addition that turned out to not have any effect so I removed it.  And your right but using valloc allocations can now be contiguous but we don't check it and set the contiguous flag so you're still right that blocks don't span regions on !vm but there's no reason they shouldn't that I can think of.

Comment 18

[Chris Peyer]

Attachment: Regression testcase for bug

Comment 19

[Chris Peyer]

Testcase pushed r2809: http://hg.mozilla.org/tamarin-redux/rev/3b68a4c6a462