573737 - Throw from ctor arg may corrupt GCFinalizedObject

Status: RESOLVED FIXED

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

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Problem summary (extracted from email dialogue below):

(1) XMLObject::operator new is run, allocating a new (zeroed) memory chunk.
(2) args to XMLObject::XMLObject are pushed on the stack; in evaluating these, we throw an exception
(3) later, we try to clean up the XMLObject that was new'ed (but not ctor'ed), but since it's all zeroed -- including the C++ vtable -- we crash trying to vall the virtual dtor.

Pasted below is (most of) an e-mail dialogue between mmgc folks about the problem (ordered with most recent email first).  Will be uploading a proposed selftest shortly.

On Jun 22, 2010, at 3:52 AM, Lars Hansen wrote:

The underlying problem is that the 'finalize' bit, which requires the vtable to be present, is set by the allocator, not when the object has been constructed.  We could try to split that up, but the performance implications are uncertain and I'm not sure how we would do it, really (maybe GCFinalizedObject::GCFinalizedObject could set it).  The better long-term choice is probably still to remove the requirement for GC'd storage to subclass GC classes, and instead store all this meta-information in a manner we control.

--lars

On Jun 21, 2010, at 11:03 PM, Steven Johnson wrote:

Ignore previous, instead change

	GCAssert(*(intptr_t*)obj != 0);
	bits[i] &= ~(kFinalize<<(j*4));		// Clear bits first so we won't get second finalization if finalizer longjmps out
	obj->~GCFinalizedObject();


to

	bits[i] &= ~(kFinalize<<(j*4));		// Clear bits first so we won't get second finalization if finalizer longjmps out
	if (*(intptr_t*)obj != 0)
		obj->~GCFinalizedObject();

?

On Jun 21, 2010, at 2:00 PM, Steven Johnson wrote:

So, in theory, we could fix it with chaning

	obj->~GCFinalizedObject();

to something like

	if (((void**)obj)[-1] != NULL)
		obj->~GCFinalizedObject();


?

On Jun 21, 2010, at 1:58 PM, Ruchi Lohani wrote:

I think I confused everything here. My earlier point that the RCObject is first getting reaped in ReapObject and then in GCAlloc::Finalize() is not valid.
Actually that wasn’t happening!

The problem is exactly what steven said below.


From: Steven Johnson 
Sent: Monday, June 21, 2010 1:56 PM
To: Felix Klock II
Cc: Ruchi Lohani; Lars Hansen
Subject: Re: RC Finalized object dtor

I think she's saying that this happens:

(1) XMLObject::operator new is run, allocating a new (zeroed) memory chunk.
(2) args to XMLObject::XMLObject are pushed on the stack; in evaluating these, we throw an exception
(3) later, we try to clean up the XMLObject that was new'ed (but not ctor'ed), but since it's all zeroed -- including the C++ vtable -- we crash trying to vall the virtual dtor.

yes?


On Jun 21, 2010, at 1:53 PM, Felix Klock II wrote:


Ruchi-

Just so I understand:

You're saying that the stack is overflowing while trying to initialize a string object, and that by chance the memory that it is trying to initialize was previously occupied by an RCObject [which is what led to the hypothesis that an argument to ZCT::ReapObject was later being passed to GCAlloc::Finalize()], and your current hypothesis is that our finalization system does not have safe-guards in place (or at least not sufficiently many) to ensure that partially initialized objects are not eligible for finalization?

Is all of the above a correct interpretation?

-Felix

On Jun 21, 2010, at 4:45 PM, Ruchi Lohani wrote:


Including Steven.

I think I figured out the problem:
From the allocation in XMLClass.cpp :

XMLObject *x = new (core->GetGC()) XMLObject(toplevel->xmlClass(),core->string(arg), defaultNamespace);

Somehow the call to core->string(arg) is going into handling stack overflow; stackoverflow exception is thrown and the player is aborted but the memory allocation has already happened and the memory is initially memset to 0 but the ctor did not get called. GC crashes when trying to finalize this object which was partially initialized.

I have worked around this crash by taking out ‘core->string(arg)’ from the call above. Is there any other better way we probably would want to fix it?

Thanks
Ruchi




From: Ruchi Lohani 
Sent: Monday, June 21, 2010 10:38 AM
To: Ruchi Lohani; Lars Hansen
Cc: Felix Klock II
Subject: RE: RC Finalized object dtor

RCobject finalization:

            NPSWF32.dll!avmplus::ScriptObject::~ScriptObject()  Line 85      C++
            NPSWF32.dll!avmplus::XMLObject::~XMLObject()  + 0x21 bytes                C++
            NPSWF32.dll!avmplus::XMLObject::`scalar deleting destructor'()  + 0x16 bytes   C++
            NPSWF32.dll!MMgc::ZCT::ReapObject(MMgc::RCObject * obj=0x072ce688)  Line 543 + 0x10 bytes   C++
            NPSWF32.dll!MMgc::ZCT::Reap(bool scanStack=true)  Line 390  C++
            NPSWF32.dll!MMgc::ZCT::AddSlow(MMgc::RCObject * obj=0x07349798)  Line 255           C++
            NPSWF32.dll!MMgc::ZCT::Add(MMgc::RCObject * obj=0x07349798)  Line 68       C++
            NPSWF32.dll!MMgc::GC::AddToZCT(MMgc::RCObject * obj=0x07349798)  Line 596          C++
            NPSWF32.dll!MMgc::RCObject::RCObject()  Line 213       C++
            NPSWF32.dll!avmplus::AvmPlusScriptableObject::AvmPlusScriptableObject(avmplus::SamplerObjectType_ * sot=0x059d83c8)  Line 49 + 0x26 bytes    C++
            NPSWF32.dll!avmplus::ScriptObject::ScriptObject(avmplus::VTable * _vtable=0x059d83c8, avmplus::ScriptObject * _delegate=0x059fdc08)  Line 53 + 0x24 bytes      C++
            NPSWF32.dll!avmplus::PlayerScriptObject::PlayerScriptObject(avmplus::VTable * vtable=0x059d83c8, avmplus::ScriptObject * delegate=0x059fdc08)  Line 19 + 0x1e bytes       C++
            NPSWF32.dll!avmplus::EventDispatcherObject::EventDispatcherObject(avmplus::VTable * ivtable=0x059d83c8, avmplus::ScriptObject * delegate=0x059fdc08)  Line 434 + 0x1e bytes     C++
            NPSWF32.dll!avmplus::DisplayObject::DisplayObject(avmplus::VTable * ivtable=0x059d83c8, avmplus::ScriptObject * delegate=0x059fdc08)  Line 67 + 0x2e bytes     C++
            NPSWF32.dll!avmplus::InteractiveObject::InteractiveObject(avmplus::VTable * ivtable=0x059d83c8, avmplus::ScriptObject * delegate=0x059fdc08)  Line 61 + 0x1e bytes       C++
            NPSWF32.dll!avmplus::ContainerObject::ContainerObject(avmplus::VTable * ivtable=0x059d83c8, avmplus::ScriptObject * delegate=0x059fdc08)  Line 54 + 0x1e bytes       C++
            NPSWF32.dll!avmplus::SpriteObject::SpriteObject(avmplus::VTable * ivtable=0x059d83c8, avmplus::ScriptObject * delegate=0x059fdc08)  Line 54 + 0x27 bytes         C++
        NPSWF32.dll!avmplus::MovieClipObject::MovieClipObject(avmplus::VTable * ivtable=0x059d83c8, avmplus::ScriptObject * delegate=0x059fdc08)  Line 43 + 0x1e bytes       C++
            NPSWF32.dll!avmplus::MovieClipClass::createInstance(avmplus::VTable * ivtable=0x059d83c8, avmplus::ScriptObject * prototype=0x059fdc08)  Line 32 + 0x35 bytes     C++
            NPSWF32.dll!avmplus::ClassClosure::newInstance()  Line 101 + 0x1f bytes           C++
            NPSWF32.dll!avmplus::ClassClosure::construct(int argc=0x00000000, int * argv=0x0009e174) Line 115 + 0x8 bytes                C++
            NPSWF32.dll!avmplus::InteractiveObjectClassBase::construct(int argc=0x00000000, int * argv=0x0009e174)  Line 312         C++
            NPSWF32.dll!avmplus::op_construct<avmplus::MethodEnv *>(avmplus::MethodEnv * env=0x0604d130, int ctor=0x05818829, int argc=0x00000000, int * atomv=0x0009e174)  Line 149 + 0x26 bytes C++
            0608d6c1()         
            NPSWF32.dll!avmplus::_AvmAssertMsg(int assertion=0x00000000, const char * message=0x00000000)  Line 70 + 0x7 bytes             C++
            80000101()         

From: Ruchi Lohani 
Sent: Monday, June 21, 2010 10:27 AM
To: Lars Hansen
Cc: Felix Klock II
Subject: RE: RC Finalized object dtor

Alloc Stack trace in Finalize() (from printAllocStackTrace()) :
MMgc::GCHeap::AllocHook(gcheap.cpp:2004) - 0x59ca8d1
                 MMgc::GCAlloc::AllocFromQuickList(gcalloc.cpp:469) - 0x59e58c8
                 MMgc::GCAlloc::Alloc(gcalloc.cpp:418) - 0x59e3ea6
                 MMgc::GC::Alloc(gc.cpp:1377) - 0x59d60e4
                 MMgc::GC::AllocRCObject(gc-inlines.h:219) - 0x54317ac
                 MMgc::RCObject::operator new(gcobject.h:205) - 0x5431777
                 avmplus::XMLClass::ToXML(xmlclass.cpp:187) - 0x5ab8785
                 avmplus::XMLClass::construct(xmlclass.cpp:125) - 0x5ab85a0
                 avmplus::op_construct<avmplus::MethodEnv *>(instr-inlines.h:149) - 0x5a9a8a5


        NPSWF32.dll!MMgc::GCAlloc::Finalize()  Line 843 + 0xc bytes      C++
            NPSWF32.dll!MMgc::GC::Finalize()  Line 1531     C++
            NPSWF32.dll!MMgc::GC::Sweep()  Line 1666      C++
            NPSWF32.dll!MMgc::GC::ForceSweepAtShutdown()  Line 1598 C++
            NPSWF32.dll!MMgc::GC::~GC()  Line 1049            C++
            NPSWF32.dll!MMgc::GC::`scalar deleting destructor'()  + 0x16 bytes        C++
            NPSWF32.dll!MMgcDestructTaggedScalarChecked<MMgc::GC>(MMgc::GC * mem=0x06343030)  Line 291 + 0x10 bytes     C++
            NPSWF32.dll!CorePlayer::DestroyPlayer()  Line 1469 + 0xc bytes               C++
            NPSWF32.dll!CommonPlayer::DestroyPlayer()  Line 375                C++
            NPSWF32.dll!PlatformPlayer::DestroyPlayer()  Line 737 C++
            NPSWF32.dll!PlatformPlayer::~PlatformPlayer()  Line 639             C++
            NPSWF32.dll!PlatformPlayer::`scalar deleting destructor'()  + 0x16 bytes               C++
            NPSWF32.dll!NPP_Destroy(_NPP * instance=0x01b01c4c, _NPSavedData * * __formal=0x0012fbc0) Line 873 + 0x34 bytes          C++



From: Lars Hansen 
Sent: Monday, June 21, 2010 10:02 AM
To: Ruchi Lohani
Cc: Felix Klock II
Subject: Re: RC Finalized object dtor

That's worrisome.  Do you have some call stacks?  Once an object has been deleted by the ZCT reaper it should not be subject to garbage collection, the only thing I can think of is that the former runs while the latter is active, and that shouldn't happen either.

--lars

On Jun 21, 2010, at 6:59 PM, Ruchi Lohani wrote:


Hi,

While helping someone here debug a fuzzed swf crash issue which was in MMgc (GCAssert(*(intptr_t*)obj != 0); being hit in GCAlloc.cpp:838 and then crash while trying to call the dtor)
When are the RCFinalizedObject dtor suppose to be called? What I am seeing is RCObject getting finalized in ZCT::ReapObject() and then later on again inGCAlloc::Finalize() where the crash happens as the vtable is 0. I tried getting the allocation trace where the assert was hit and the object reported was the same which got reaped before in ReapObject().  The player is getting aborted (corrupt swf) and the crash is while force sweep at shutdown.

Thanks
Ruchi

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: attempt abort of GCFinalizedObject before fully initialized

Here's a self-test for the above scenario; note in particular that we cannot in general directly expose the evil order of evaluation that is biting us here, so I had to use some placement new trickery to emulate it.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Note: If some of d1's in the selftest are inadvertently kept alive via stale stack references, the verify checks will fail (for the wrong reason).  :(

(Worse yet, if the d2's are kept alive in the same manner, the selftest will succeed (for the wrong reason).

So future work (assuming the strategy in this selftest is deemed acceptable) would be to adopt an approach that does not rely on all garbage being reclaimed precisely, but rather use a probabilistic approach.  (Better still, design a common framework for many mmgc selftests to share.)

Comment 3

[Steven Johnson]

Comment on attachment 453066 [details]
attempt abort of GCFinalizedObject before fully initialized

All testcases should include a function named "deathString"

Comment 4

[Ruchi Lohani]

(In reply to comment #1)
> Created an attachment (id=453066) [details]

DWB(Stringp) m_s; 
should be 
DRCWB(Stringp) m_s;

Comment 5

[Ruchi Lohani]

Attachment: Call finalizer only if the object is in sane state

This is going to prevent all such insane cases where the object remains in an uninitialized state after allocation. On Mac the behavior is actually different: the arguments to the ctor are evaluated first and the memory gets allocated only after that. Any feedback on adding more comments to the code? I think the bug contains much detailed information on the issue.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #1)
> Created an attachment (id=453066) [details]
> attempt abort of GCFinalizedObject before fully initialized
> 
> Here's a self-test for the above scenario; note in particular that we cannot in
> general directly expose the evil order of evaluation that is biting us here, so
> I had to use some placement new trickery to emulate it.

Hmm.  On further inspection, I'm not quite sure how even the first verify condition was passing.

I'll be uploading a new version of the selftest shortly, as well as an experimental patch to set the kFinalize bit from within the GCFinalizedObject constructor ...

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: selftest abort of GCFinalizedObject before fully initialized

Cleanup of selftest.
1. Incorporated feedback (good catch by Ruchi), 
2. switched to statistical verification rather than incorrectly assuming gc will be 100% precise, 
3. fixed logical verification errors (D::~D logic was decrementing rather than incrementing the counter..) and 
4. got rid of the manual inlining of GCFinalizedObject::operator new (which meant that my later sketchy changes to GCFinalizedObject::operator new were masked in the test, bah)

(Left Ruchi and Steven off feedback list, since they chimed in earlier, but really, feel free to look at it if you like...)

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: set kFinalize bit in ctor rather than via operator new

Here's a patch that takes the other approach for fixing the bug.  It passes my selftest, but I have not tested it otherwise.

Most importantly, I have not tested it for speed regression, which I think is what Lars is most concerned about w.r.t. this approach.  (There might be a way to make it faster, or to speed up other paths assuming we can get rid of AllocPtrZeroFinalized entirely now...)

(So we may want to adopt Ruchi's patch, at least until we discover a case where its detection fails...)

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 453242 [details] [diff] [review]
Call finalizer only if the object is in sane state

1. Nit: you should take out the assert.  (I had the same instinct that you did, but Steven pointed out to me yesterday that our policy is: "assertions should only trigger for shouldn't-happen conditions -- we now know this can happen")

2. If everyone else is willing to go along with the vptr-inspection trick, then I am too.  (I might rope Tommy into weighing in on the matter too, though.)

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #8)
> Created an attachment (id=453286) [details]
> set kFinalize bit in ctor rather than via operator new

Oh, by the way, there is precedent for this, namely in RegExpObject.  (Of course I doubt that is a hot path...)

Comment 11

[Lars T Hansen]

Comment on attachment 453242 [details] [diff] [review]
Call finalizer only if the object is in sane state

(Holding my nose.)

As Felix said, the GCAssert needs to be removed.

This will be an adequate stopgap solution I expect, we might continue to search for something better.

Comment 12

[Lars T Hansen]

Comment on attachment 453286 [details] [diff] [review]
set kFinalize bit in ctor rather than via operator new

This is probably correct (and better than what we currently have) but it is likely to make most allocation slower, unfortunately: SetFinalize dispatches on size to the appropriate underlying allocator mechanism.

Comment 13

[Lars T Hansen]

Comment on attachment 453285 [details] [diff] [review]
selftest abort of GCFinalizedObject before fully initialized

You write:

// (really, not dying is passing.)
%%verify (D::finalized_count() > 90)

If that's the case then a simple

%%verify true

would do.

Why are you concerned about the finalization fraction anyway?

f+ tentatively because I think the test tests what it should test, but there's stuff here I don't understand why is there.

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #13)
> (From update of attachment 453285 [details] [diff] [review])
> You write:
> 
> // (really, not dying is passing.)
> %%verify (D::finalized_count() > 90)
> 
> If that's the case then a simple
> 
> %%verify true
> 
> would do.
> 
> Why are you concerned about the finalization fraction anyway?

Well, really the point was to make sure that the finalizer is actually getting run.

  %%verify (D::finalized_count() > 0) 

would also do that job, I guess.  But "%%verify true" would not catch the (important) bug of when finalizer invocation is turned into a no-op.

Having said that, I think we should settle on our expectation for statistical tests of reclamation, and then use that percentage consistently.  For that, I would not be satisfied with ">0%" as our standard across the board.  I picked 90% out of a hat, but there was no basis for it.

Comment 15

[Lars T Hansen]

Why / under what circumstances would the finalizer invocation be turned into a no-op?

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #15)
> Why / under what circumstances would the finalizer invocation be turned into a
> no-op?

I said it was a bug.  It was probably a mistake to call it an "important bug."

One of my attempts to fix the bug removed the kFinalize setting from operator new, but screwed up setting kFinalize in the constructor.  (I had not yet seen RegExpObject code at the time and was flailing; it was late.)

Now, while I could (and do) chalk that up to my own stupidity, it seems to me that when one is fixing a bug like this, a mistake like that is actually likely to pop up, and if the self-test can catch it, why not include the check?

Comment 17

[Lars T Hansen]

I absolutely agree that it would be good to check.  But is this the test case that should check it?

Comment 18

[Ruchi Lohani]

Attachment: uninitialized finalized object crash fix

Adding Tommy on review for the updated patch. 
Removed the assert and updated comments in code.

Comment 19

[Tommy Reilly]

Comment on attachment 453389 [details] [diff] [review]
uninitialized finalized object crash fix

The only negative thing I can think of is that we might mask a different problem with this if that we would really rather find out about.  We have had problems before crashing here with a zero vtable (trying to finalize something twice for instance).  

Did we dismiss having the ctor sequence flip the finalize bit for some reason?

Comment 20

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #19)
> Did we dismiss having the ctor sequence flip the finalize bit for some reason?

I think Lars addressed in in comment 12.

I believe I know what he means about the newly introduced conditional branch.  I do not yet know of a way to avoid it; e.g. to my knowledge there is not a way to overload constructors based on the size of the whole object being constructed.

Comment 21

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #17)
> I absolutely agree that it would be good to check.  But is this the test case
> that should check it?

(I chatted with Lars and now understand what he is getting at; I will refactor the test case code.)

Comment 22

[Ruchi Lohani]

Adding the null vptr check in finalization might reintroduce other problems. 
Btw, how is finalizing twice possible? We reset the finalize bit before finalizing so is there a chance we will again do finalization?   
Setting finalize bit in ctor also will cause allocation slower.
How about modifying the way allocation is done (no args should throw) that is getting into this trouble? That will of course fix only this instance of problem and there might be several other places ready to behave in the same way.

Comment 23

[Tommy Reilly]

(In reply to comment #22)
> Btw, how is finalizing twice possible?

Old bug, just an example

Comment 24

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: selftest abort of GCFinalizedObject before fully initialized

Refactored self-test as Lars suggested, got rid of cruft from when I was flailing on it, and added general improvements.  (Time for me to stop playing with this though.)

(Also, there's one selftest with bug id embedded in its filename.  do we want to continue that pattern...?  I'm inclined not to if a reasonable filename is available.)

Comment 25

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #22)
> Adding the null vptr check in finalization might reintroduce other problems. 
> Btw, how is finalizing twice possible? We reset the finalize bit before
> finalizing so is there a chance we will again do finalization?   
> Setting finalize bit in ctor also will cause allocation slower.

Ruchi: in the above, it sounds like you are presenting independent arguments: one against null_vptr patch, and another against  set-kFinalizer-during-ctor patch.  (Am I misinterpreting?)

(I could try to set aside time to gather concrete data on how much slower allocation gets when setting kFinalizer in ctor. The problem is that I'm likely only to test on my mac, but I suspect we should be worrying about all platforms, especially mobile, right?)

> How about modifying the way allocation is done (no args should throw) that is
> getting into this trouble? That will of course fix only this instance of
> problem and there might be several other places ready to behave in the same
> way.

The problem is that the requirement "no args should throw" is too hard to verify unless we strengthen the constraint significantly; e.g. I jokingly suggested requiring SSA form when using mmgc's new to Lars yesterday.  So it would be yet another mmgc requirement that went unchecked until worst possible moment.

Comment 26

[Lars T Hansen]

Comment on attachment 453389 [details] [diff] [review]
uninitialized finalized object crash fix

We've lived with the assert for a while without it bugging anyone; as a stopgap measure this is likely to be fine.

Comment 27

[Lars T Hansen]

(In reply to comment #24)
 
> (Also, there's one selftest with bug id embedded in its filename.  do we want
> to continue that pattern...?  I'm inclined not to if a reasonable filename is
> available.)

For regressions I'd be much more comfortable if they were in individual files named by the bug number of the regression, like that one file.  Currently we're not doing that uniformly, there are regression tests eg in mmgc_basics whose test names are the bug numbers.  That more or less works the same way but tends to clutter up the tests.  Over time we'll presumably have many, many regression tests, coming up with meaningful names will become increasingly difficult, and finding the test case given the bug will also become harder.  Better to just bite the bullet and clean it up imo.

Comment 28

[Trevor Baker]

does this repro in fp9?

Comment 29

[Lars T Hansen]

(In reply to comment #28)
> does this repro in fp9?

It ought to repro - allocation happens the same way in FP9, with the 'new' operator setting the finalization flag and the constructor setting up the vtable, obviously.

Comment 30

[Trevor Baker]

Has this landed in p4?  Is there a watson bug number?

Comment 31

[Tommy Reilly]

Comment on attachment 453389 [details] [diff] [review]
uninitialized finalized object crash fix

+1 for crush, we need to fix the crash this is a good safe fix.    But I'm still worried going forward that we could miss some new bug that also manifests as a zero'd vtable

Comment 32

[Ruchi Lohani]

(In reply to comment #30)
> Has this landed in p4?  Is there a watson bug number?

Watson bug number: 2638752
The fix has landed in crush with other changes related to this Watson bug. Change list: 686766

(In reply to comment #31)
> (From update of attachment 453389 [details] [diff] [review])
> +1 for crush, we need to fix the crash this is a good safe fix.    But I'm
> still worried going forward that we could miss some new bug that also manifests
> as a zero'd vtable

Probably we could have left the assert in place? At least could help in those zero'd vtable rare cases. But it will affect debugging on the client side. QE has added a test case to the automated test suite and the assert will interfere in that in debug builds.

Comment 33

[Lars T Hansen]

There is a problem here that's more complicated than we may have realized, and it might warrant a separate bug because the underlying problem is in some sense a different kind of problem.

Right now we're crashing because the 'finalize' flag is set before the vtable is installed; if the constructor is never run then we have an object on the heap that will have a NULL vtable pointer and the destructor call will cause a crash.

The proposed fix is to install the 'finalize' flag in the constructor, presumably in the GCFinalizedObject constructor.

Doing that would be more correct but it would not actually be fully correct.  Consider an object inheritance chain:

   GCFinalizedObject
   RCObject
   AvmPlusScriptableObject
   ScriptObject
   MyGlueObject

At any point in the construction chain, the vtable that's installed in the object is a vtable that is appropriate for the object, eg, ScriptObject's vtable does not have any pointers to MyGlueObject's virtual functions, only to ScriptObject's functions.  Thus if any constructor in this chain aborts the construction process by throwing an exception, we're left with an object on the heap where the vtable points to a destructor that starts with some intermediate object, not the most derived object.  (At least this is my understanding - and I believe I've observed that happening.)

Thus setting the kFinalizedObject flag in the GCFinalizedObject constructor does not guard against invoking the wrong destructor.  On the other hand, setting it in the most derived class's constructor doesn't solve the problem either because it means some objects that need a destructor may not be destroyed (if the construction process throws an exception).

The underlying problem is that constructors can throw exceptions at all, ie, that construction is not atomic and guaranteed to succeed, or if you like, that it is possible to observe intermediate, possibly inconsistent, states of an object outside that object's constructor(s).

That may be acceptable if you're programming in C++, but what we've done here by choosing to use the C++ vtable mechanism to support garbage collection is to expose this problem to the ActionScript programmer, and to Flash Player developers for that matter.

Maybe I'm being overly dramatic, since any careless (partly-completed) update to an object may leave it in an inconsistent state, but it does seem wrong that it's possible to construct objects that are nonsensical from the GC's point of view.

Comment 34

[Ruchi Lohani]

Yes, the object partial initialization can happen if a constructor in the inheritance chain throws. Its also correct that the object's vptr will point to the vtable of the class whose ctor throws. 

So if the kFinalizedObject flag is set in GCFinalizedObject ctor and ScriptObject ctor in the above mentioned chain throws, the object's vptr will be pointing to the ScriptObject's vtable. While calling dtor, all the dtors up in the chain will be called including ~ScriptObject(). I am not sure whether this is the wrong invocation of dtors you mentioned in the above comment. We dont want the dtor of MyGlueObject to be called as it was not initialized (ctor did not get called). What do we expect the behavior to be in case some intermediate ctor throws?

Comment 35

[Trevor Baker]

The remaining issue raised in comment #33 isn't making crush.  Can we mark this crush targeted issue as resolved/fixed and log a new one for the remaining work?

Comment 36

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #35)
> The remaining issue raised in comment #33 isn't making crush.  Can we mark this
> crush targeted issue as resolved/fixed and log a new one for the remaining
> work?

Opened separate ticket for issues from comment #33 in bug 581016

Comment 37

[Steven Johnson]

Comment on attachment 453389 [details] [diff] [review]
uninitialized finalized object crash fix

pushed: http://hg.mozilla.org/tamarin-redux/rev/a7d91aa8cb5b

Comment 38

[Ruchi Lohani]

The code patch has been pushed. The selftest is pending review from Tommy. Need to push the selftest before this bug should be marked fixed/resolved.

Comment 39

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 453732 [details] [diff] [review]
selftest abort of GCFinalizedObject before fully initialized

Shall we push this then, Tommy?  (and by "we" I really mean "you" :)

Comment 40

[Tommy Reilly]

http://hg.mozilla.org/tamarin-redux/rev/097ed82dbd05

Comment 41

[Ruchi Lohani]

Both patch and selftest have been pushed. Marking it resolved.

Comment 42

[Trevor Baker]

Regarding comment #29, yes, it does crash in fp9 crush.  It was not originally targeted to fp9 in watson, where the bug originated.

Comment 43

[Ruchi Lohani]

Comment on attachment 453732 [details] [diff] [review]
selftest abort of GCFinalizedObject before fully initialized

Marking f+. Test has been reviewed and pushed.

Comment 44

[Ruchi Lohani]

Comment on attachment 453286 [details] [diff] [review]
set kFinalize bit in ctor rather than via operator new

Marking f+. We had discussed about it here and a separate bug was opened: 581016.