560427 - Abort() may be called from AllocNoOOM()

Status: VERIFIED FIXED

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

Description

[Brent Getlin]

#4  0x184c02ce in MMgc::GCHeap::Abort (this=0x18a80310) at /Users/bgetlin/depot/users/bgetlin/volatile/Argo_Athena_GMC/code/products/player/osx/../../../third_party/avmplus/MMgc/GCHeap.cpp:2297
#5  0x184c0519 in MMgc::GCHeap::CheckForHardLimitExceeded (this=0x18a80310) at /Users/bgetlin/depot/users/bgetlin/volatile/Argo_Athena_GMC/code/products/player/osx/../../../third_party/avmplus/MMgc/GCHeap.cpp:700
#6  0x184c19d4 in MMgc::GCHeap::CheckForMemoryLimitsExceeded (this=0x18a80310) at /Users/bgetlin/depot/users/bgetlin/volatile/Argo_Athena_GMC/code/products/player/osx/../../../third_party/avmplus/MMgc/GCHeap.cpp:449
#7  0x184c1db0 in MMgc::GCHeap::Alloc (this=0x18a80310, size=1, flags=31, alignment=1) at /Users/bgetlin/depot/users/bgetlin/volatile/Argo_Athena_GMC/code/products/player/osx/../../../third_party/avmplus/MMgc/GCHeap.cpp:363
#8  0x184b65fa in MMgc::GCHeap::AllocNoOOM (this=0x18a80310, size=1, flags=15) at GCHeap.h:401

Comment 1

[Brent Getlin]

Attachment: GCHeap::Alloc() patch

Comment 2

[Tommy Reilly]

Comment on attachment 440110 [details]
GCHeap::Alloc() patch

I don't see how the second part of this patch is related to the bug, if its related please explain, otherwise log a new bug.

Comment 3

[Brent Getlin]

It's important because removing the "CheckForMemoryLimitsExceeded()" in the first part, we're never checking to see if we've breached softLimit by making this "canFail" allocation.  This is necessary because our policy is to release this memory back to GCHeap when this occurs.  Since HardLimit can be breached this way too, but we're not allowed to trigger Abort(), we want to follow the same logic here.

Comment 4

[Dan Smith]

Brent, please submit to Argo, TR-Argo, and TR today.  Thanks.

Comment 5

[Tommy Reilly]

tamarin-redux:

changeset:   4554:ebc7d2ca6bc0
user:        Tommy Reilly <[e-mail]>
date:        Wed Apr 21 13:42:35 2010 -0400
summary:     Fix 560427, pushing for bgetlin (r=treilly,sr=lhansen)

tamain-redux-argo:


changeset:   4015:fc8a98548a06
user:        Tommy Reilly <[e-mail]>
date:        Wed Apr 21 13:38:43 2010 -0400
summary:     Fix 560427, pushing for bgetlin (r=treilly,sr=lhansen)

Comment 6

[Dan Schaffer]

is there a testcase we can use for regression testing and marking this bug verified?

Comment 7

[Tommy Reilly]

A selftest can be created that calls AllocNoOOM and verifies that NULL is returned instead of triggering abort.

Comment 8

[Dan Schaffer]

Added selftest code the ST_mmgc_gcheap.st (below).  The tests shows with Alloc(...,kCanFail) in a loop after several successful allocations Alloc() returns NULL and does not trigger OOM.  marking bug as verified.  A problem is the test code does not free memory so the next test fails.  any ideas how I can free the allocated memory after the test finshes?

here is the test code:
added to extensions/ST_mmgc_gcheap.st:
%%test AllocNoOOM
       GCHeap *heap;
       heap = GCHeap::GetGCHeap();
       int ctr=0;
       char didFail=0;
       while (true) {
           void *item=heap->AllocNoOOM(10,GCHeap::kCanFail);
           //printf("allocs %d %d\n",ctr++,item);
           if (item==NULL) {
               didFail=1;
               break;
           }
       }
       %%verify didFail == 1

Comment 9

[Lars T Hansen]

(In reply to comment #8)
> any ideas how I can free the allocated memory after the test finshes?

Put them in a list and then traverse the list and free the memory after the loop?  (Ping me offline if you need more help than that.)