489862 - HeapBlock list gets corrupted causing gc to hang

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P1)

Description

[Rick Reitmaier]

The first indication that I've found that this condition is about to occur is an assert in GCHeap.AllocBlock will fire.  Specifically,
 
   GCAssert(amountRecommitted > 0);

If you examine the blocks at that point you'll see something like:

(gdb) p block->size
$202 = 3
(gdb) p (block+3)->sizePrevious
$203 = 1

which indicates that the block list is corrupt.  Continuing from this point, you'll find the gc will continuously run (at least in my case).

Comment 1

[Chris Peterson [:cpeterson]]

I can reproduce the GCAssert(amountRecommitted > 0) assertion failure with the Debug ActiveX Flash Player. Load the following test page (unfortunately, only available within Adobe's corporate network) and click the RUN buttons for each of the four Flash movies:

http://ats.macromedia.com/Players/ATS/ATS.html

Assert stack trace:

>	Flash.ocx!VMPI_debugBreak()  Line 61 + 0x8 bytes	C++
 	Flash.ocx!MMgc::GCDebugMsg(const char * p=0x5ce09020, bool debugBreak=true)  Line 73	C++
 	Flash.ocx!MMgc::_GCAssertMsg(int assertion=0, const char * msg=0x5ce09020)  Line 54 + 0xb bytes	C++
 	Flash.ocx!MMgc::GCHeap::AllocBlock(int size=3, bool & zero=true)  Line 766 + 0x14 bytes	C++
 	Flash.ocx!MMgc::GCHeap::_Alloc(int size=3, bool expand=false, bool zero=true, bool track=true)  Line 197 + 0x10 bytes	C++
 	Flash.ocx!MMgc::GC::heapAlloc(unsigned int siz=3, bool expand=false, bool zero=true, bool internal=true)  Line 3155 + 0x21 bytes	C++
 	Flash.ocx!MMgc::GC::heapAllocNoProfile(unsigned int size=3, bool expand=false, bool zero=true)  Line 1196	C++
 	Flash.ocx!MMgc::GC::AllocBlockIncremental(int size=3, bool zero=true)  Line 1083 + 0x13 bytes	C++
 	Flash.ocx!MMgc::GC::AllocBlock(int size=3, int pageType=3, bool zero=true)  Line 1055 + 0x11 bytes	C++
 	Flash.ocx!MMgc::GCLargeAlloc::Alloc(unsigned int size=11760, int flags=3)  Line 53 + 0x1e bytes	C++
 	Flash.ocx!MMgc::GC::AllocAlreadyLocked(unsigned int size=11760, int flags=3)  Line 801 + 0x16 bytes	C++
 	Flash.ocx!MMgc::GC::Alloc(unsigned int size=11740, int flags=3)  Line 739	C++
 	Flash.ocx!MMgc::GCObject::operator new(unsigned int size=32, MMgc::GC * gc=0x06a94028, unsigned int extra=11708)  Line 86	C++
 	Flash.ocx!avmplus::TraitsBindings::alloc(MMgc::GC * gc=0x06a94028, avmplus::Traits * _owner=0x076f49f0, const avmplus::TraitsBindings * _base=0x0e806018, avmplus::MultinameHashtable * _bindings=0x15edc828, unsigned int slotCount=13, unsigned int methodCount=223, unsigned int interfaceCapacity=2)  Line 280 + 0xf bytes	C++
 	Flash.ocx!avmplus::Traits::_buildTraitsBindings(const avmplus::Toplevel * toplevel=0x00000000, avmplus::AbcGen * abcGen=0x00000000, avmplus::ImtBuilder * imtBuilder=0x00000000)  Line 1470 + 0x2c bytes	C++
 	Flash.ocx!avmplus::Traits::_getTraitsBindings()  Line 2114 + 0xe bytes	C++
 	Flash.ocx!avmplus::Traits::getTraitsBindings()  Line 447 + 0x8 bytes	C++
 	Flash.ocx!avmplus::Traits::_buildTraitsBindings(const avmplus::Toplevel * toplevel=0x00000000, avmplus::AbcGen * abcGen=0x00000000, avmplus::ImtBuilder * imtBuilder=0x00000000)  Line 1452 + 0x14 bytes	C++
 	Flash.ocx!avmplus::Traits::_getTraitsBindings()  Line 2114 + 0xe bytes	C++
 	Flash.ocx!avmplus::Traits::getTraitsBindings()  Line 447 + 0x8 bytes	C++
 	Flash.ocx!avmplus::Traits::containsInterface(avmplus::Traits * t=0x06d094c8)  Line 459 + 0x2b bytes	C++
 	Flash.ocx!avmplus::AvmCore::istype(int atom=206340361, avmplus::Traits * itraits=0x06d094c8)  Line 1997 + 0x1e bytes	C++
 	Flash.ocx!avmplus::PlayerAvmCore::isPlayerType(int atom=206340361, int class_id=291)  Line 793 + 0x24 bytes	C++
 	Flash.ocx!avmplus::EventDispatcherObject::NativeDispatchEvent(avmplus::EventObject * event=0x11dda9e8, avmplus::List<MMgc::GCWeakRef *,1> * stack=0x02dff4a4)  Line 1364 + 0x16 bytes	C++
 	Flash.ocx!avmplus::EventDispatcherObject::NativeDispatchEvent(avmplus::EventObject * event=0x11dda9e8, avmplus::List<MMgc::GCWeakRef *,1> * stack=0x02dff4a4, bool & preventDefault=false)  Line 1256 + 0x1a bytes	C++
 	Flash.ocx!avmplus::EventDispatcherObject::NativeConstructAndDispatchEvent(avmplus::List<MMgc::GCWeakRef *,1> * stack=0x02dff4a4, bool & preventDefault=false, int clsId=27, const char * fmt=0x5cd90070, ...)  Line 1214	C++
 	Flash.ocx!avmplus::EventDispatcherObject::DispatchBaseEvent(avmplus::String * type=0x06bdc9a0, bool bubbles=true, bool cancelable=false)  Line 1723 + 0x26 bytes	C++
 	Flash.ocx!DisplayList::SetParent(SObject * obj=0x11fab388, SObject * newParent=0x0c4f5b08, int newIndex=0, bool sendAddRemoveEvents=true)  Line 3244	C++
 	Flash.ocx!avmplus::ContainerObject::addChild(avmplus::DisplayObject * child=0x0c4d89c8)  Line 158 + 0x1b bytes	C++
 	Flash.ocx!avmplus::LoaderObject::AddChildObject(avmplus::DisplayObject * displayObject=0x0c4d89c8)  Line 359	C++
 	Flash.ocx!avmplus::PlayerAvmCore::ExecuteQueuedConstructor(SObject * sObject=0x11fab388, avmplus::List<SObject *,1> & initList={...})  Line 2656	C++
 	Flash.ocx!avmplus::PlayerAvmCore::ExecuteQueueTopDown(avmplus::List<SObject *,1> & queue={...}, int operation=0, avmplus::List<SObject *,1> & initList={...})  Line 2738	C++
 	Flash.ocx!avmplus::PlayerAvmCore::ExecuteQueuedScripts()  Line 2529	C++
 	Flash.ocx!CorePlayer::DoPlay_DoFrame()  Line 12162	C++
 	Flash.ocx!CorePlayer::DoPlay(bool wait=true)  Line 12480 + 0x8 bytes	C++
 	Flash.ocx!CFlashTimerSink::OnTimer(tagVARIANT __formal=UI4 =  176896031)  Line 1172	C++
 	mshtml.dll!5daf7efd()

Comment 2

[Dan Smith]

Still reproducible?

Comment 3

[Trevor Baker]

Dan, can you try this to see if it still repros?

Comment 4

[Mathieu Baron]

While reading GCHeap::FreeBlock(), I found something that can explain why this
occurs:

Code base: tamarin-central-dab354bc047c
Date:    2009-08-17
URL:     http://hg.mozilla.org/tamarin-central/archive/dab354bc047c.zip
File:     ./MMgc/GCHeap.cpp

Method:           
  (line 972)  void GCHeap::FreeBlock(HeapBlock *block) 

Description:
    The problem can occur with the following situation:

    Initial Memory state:
                   | . . . . . . . . . . . .|
                   | Uncomitted memory (B)  |
                   |                        |
              +->  +========================+  8192
              |    | Block in use (B)       |
              |    |                        |  
              |    |                        |
  Region -----|    +------------------------+  4096
  (0 to 8KB)  |    | Block to free(A)       |
              |    |                        |<-- Trying to free this block
              |    |                        |
              +->  +========================+  0


Let’s say we want to free Block #A

The first thing that is done in the method is to check if the block is a full
region to release it to the OS. 
Code: 
  (line 976)     // big block's that map to a region are free'd right away
  (line 977)     Region *r = AddrToRegion(block->baseAddr);
  (line 978)     if(block->baseAddr == r->baseAddr && block->endAddr() ==
r->reserveTop)
  (line 979)     {
  (line 980)         RemoveBlock(block);
  (line 981)         return;
  (line 982)     }

In this case, the block #A is not a full reserved region and we do not enter
the "if" statement. We must continue to trace the method. 


Merge the Block #A with it’s predecessor. 
Code: 
  (line 989)    // Try to coalesce this block with its predecessor
  (line 990)    HeapBlock *prevBlock = block - block->sizePrevious;
  (line 991)    if (!prevBlock->inUse() && prevBlock->committed) {
  (line 992)        // Remove predecessor block from free list
  (line 993)        RemoveFromList(prevBlock);
  (line 994)                       
  (line 995)        // Increase size of predecessor block
  (line 996)        prevBlock->size += block->size;
  (line 997)                       
  (line 998)        block->size = 0;
  (line 999)        block->sizePrevious = 0;
  (line 1000)       block->baseAddr = 0;                
  (line 1001)                   
  (line 1002)       block = prevBlock;
  (line 1003)     }

In the current case, there is no predecessor but there is no condition to
validate this. Here is what can happen:
  o  When there is no previous block "block->sizePrevious == 0".

  o After execution of this line, prevBlock will be a pointer on the current
block (prevBlock == block):
       (line 990)    HeapBlock *prevBlock = block - block->sizePrevious;

  o We will enter the "if" branch:
       (line 991)    if (!prevBlock->inUse() && prevBlock->committed) {

  o We will corrupt block information because (prevBlock == block). We will
double the "size" of the current block:
       (line 995)        // Increase size of predecessor block
       (line 996)        prevBlock->size += block->size;

       - With the current example, this is the same as doing:
           block->size += block->size;


**See also Bug 511088 - GCHeap::FreeBlock can corrupt block list or cause acces violation

Comment 5

[Tommy Reilly]

Attachment: fixes sizePrevious == 0 condition

Comment 6

[Steven Johnson]

Nice find!

Comment 7

[Tommy Reilly]

fixed: changeset:   2375:44a17dc86db4