563883 - Common code in GCHeap::GCHeap and GCHeap::DestroyInstance should be factored

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

We had an instance (bug #563729) where the code in DestroyInstance did not reset variables properly; this matters when OOM aborts the player but then later actions restart it. The problem would have been avoided had the initialization code been factored as a common method (or methods, depending) to be called from both places.

Comment 1

[Ruchi Lohani]

Attachment: patch

Uploading patch for review. Suggestions welcome. I see only gcheap instance and the profiler that will need to be initialized in GCHeap::GCHeap() and GCHeap::DestroyInstance(). Need to leave leakedBytes, as its returned by GCHeap::Destroy().

Comment 2

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

This can't be right:

+    /* static */
+    void GCHeap::Reset()
+    {
+        instance == NULL;

Comment 3

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

Also, I'm not clear on whether you are deliberately removing the assignments of NULL to "instance" without putting anything in their place to do something similar.  Were you deliberately leaving a dangling reference to the destroyed instance in place, under the assumption that the instance field would never be used again?

(I would have guessed that the "right" thing was to make Reset() set instance to NULL, and to put an invocation of Reset() at every point where there had previously been a "instance = NULL" assignment statement; but I have not looked carefully yet to make sure that is sane.)

Comment 4

[Ruchi Lohani]

Yes, It is not right. Thanks for noticing, will correct it. 
DestroyInstance() is called after/before the instance is set to NULL. DestroyInstance() is calling Reset() too. Now the "instance = NULL" is moved to Reset().

Comment 5

[Lars T Hansen]

I will await a new patch.

Comment 6

[Ruchi Lohani]

Attachment: Updated patch

Updated patch with the review comments.

Comment 7

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

Comment on attachment 448537 [details] [diff] [review]
Updated patch

r+, assuming you fix the big nit (first comment below).

Nit: you have re-introduced tab characters into GCHeap.h.  (You have also introduced dangling spaces at the end of lines in both files, which Ed's fixtabs script also removes and therefore I assume we are trying to eschew.)

I see now that the instance->DestroyInstance() call invokes Reset() towards the very end of its execution, (which sets GCHeap::instance to NULL).  As far as I can tell, this is not unsound (just unsettling), and I am guessing that trying to clean up the handling of the instance global should be part of a more general clean up of this code.

Question: Should we consider renaming Reset() as ResetStatics, to make it clear at the call sites that this has nothing to do with the non-static members of GCHeap?

Comment 8

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

(In reply to comment #7)
> (From update of attachment 448537 [details] [diff] [review])
> r+, assuming you fix the big nit (first comment below).

Also, regarding this comment:

@@ -203,6 +213,9 @@
         VMPI_lockInit(&m_spinlock);
         VMPI_lockInit(&gclog_spinlock);
+        // Reset should be called at the starting before using/initializing any statics

1. A small grammatical correction: I think the correct phrase below is "at the start", not "at the starting."

2. One might think from reading this comment that the invocations of Reset() in DestroyInstance() are erroneous.  Perhaps say instead that "Reset should be called at the start and end of the GCHeap's lifetime" ?  (Or, if you prefer, "dynamic extent" rather than "lifetime"?)

Comment 9

[Ruchi Lohani]

 (In reply to comment #7)
> (From update of attachment 448537 [details] [diff] [review])
> r+, assuming you fix the big nit (first comment below).
> 
> Nit: you have re-introduced tab characters into GCHeap.h.  (You have also
> introduced dangling spaces at the end of lines in both files, which Ed's
> fixtabs script also removes and therefore I assume we are trying to eschew.)

Is there any internal coding standard which we are following in the tamarin code? I would like to make sure I am aware of that. 
> 
> I see now that the instance->DestroyInstance() call invokes Reset() towards the
> very end of its execution, (which sets GCHeap::instance to NULL).  As far as I
> can tell, this is not unsound (just unsettling), and I am guessing that trying
> to clean up the handling of the instance global should be part of a more
> general clean up of this code.

I did not get what you are trying to say here.
> 
> Question: Should we consider renaming Reset() as ResetStatics, to make it clear
> at the call sites that this has nothing to do with the non-static members of
> GCHeap?

I am in favor of this.

Comment 10

[Ruchi Lohani]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 448537 [details] [diff] [review] [details])
> > r+, assuming you fix the big nit (first comment below).
> 
> Also, regarding this comment:
> 
> @@ -203,6 +213,9 @@
>          VMPI_lockInit(&m_spinlock);
>          VMPI_lockInit(&gclog_spinlock);
> +        // Reset should be called at the starting before using/initializing
> any statics
> 
> 1. A small grammatical correction: I think the correct phrase below is "at the
> start", not "at the starting."
> 
Will correct it. 
> 2. One might think from reading this comment that the invocations of Reset() in
> DestroyInstance() are erroneous.  Perhaps say instead that "Reset should be
> called at the start and end of the GCHeap's lifetime" ?  (Or, if you prefer,
> "dynamic extent" rather than "lifetime"?)

This particular comment is local and specific to the call to Reset() in GCHeap::GCHeap() where it should be called before initializing. I think the above comment which you suggested will be more appropriate where Reset() is declared, so that someone trying to use it will know how & where its suppose to be used.

Comment 11

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

(In reply to comment #9)
>  (In reply to comment #7)
> > (From update of attachment 448537 [details] [diff] [review] [details])
> > r+, assuming you fix the big nit (first comment below).
> > 
> > Nit: you have re-introduced tab characters into GCHeap.h.  (You have also
> > introduced dangling spaces at the end of lines in both files, which Ed's
> > fixtabs script also removes and therefore I assume we are trying to eschew.)
> 
> Is there any internal coding standard which we are following in the tamarin
> code? I would like to make sure I am aware of that. 

This is the main item I can think of:

  https://zerowing.corp.adobe.com/display/FlashPlayer/Tamarin+Coding+Standards

The relevant (but non-normative) rule on that page is "Spaces only please, no
tabs"; as I mentioned, I was only inferring the effort to eschew dangling
spaces from the behavior of Ed's fixtabs script.

> > I see now that the instance->DestroyInstance() call invokes Reset() towards the
> > very end of its execution, (which sets GCHeap::instance to NULL).  As far as I
> > can tell, this is not unsound (just unsettling), and I am guessing that trying
> > to clean up the handling of the instance global should be part of a more
> > general clean up of this code.
> 
> I did not get what you are trying to say here.

I just wanted to note that I find it unsettling when a function call like:

  A::foo->baz()

ends up setting A::foo to NULL.

Such code does not make your patch incorrect.  It just caught my attention, to see an "instance" reset method actually resetting static state (moreover, static state that happens be the receiver of the method call, not that this is a virtual method, but its still the source of the "this" ptr).

Reviewing the code, I decided there was no danger in introducing this Reset() call within DestroyInstance().  So I concluded that this kind of thing can be left as-is until there is an actual need to change the behavior of DestroyInstance, e.g. if we ever decided for some reason to have >1 instance of GCHeap floating around.  So I kept the r+, but wanted to note the issue.

Comment 12

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

(In reply to comment #10)
> This particular comment is local and specific to the call to Reset() in
> GCHeap::GCHeap() where it should be called before initializing. I think the
> above comment which you suggested will be more appropriate where Reset() is
> declared, so that someone trying to use it will know how & where its suppose to
> be used.

Good point; I withdraw my suggestion.

Comment 13

[Ruchi Lohani]

Cool thanks. It was actually the same before also. i.e.
instance->DestroyInstance() was called right after "instance = NULL" which did
sound unsettling to me too.

Comment 14

[Ruchi Lohani]

Attachment: Patch after review comments

Updated with review comments till now.

Comment 15

[Lars T Hansen]

Comment on attachment 448537 [details] [diff] [review]
Updated patch

Although I'm r+'ing this patch, it's really the follow-on patch I've reviewed, and it looks good.

Ruchi, just in case you don't know - the normal way of replacing patches is to check an old patch as 'obsolete' when uploading a new patch (a checkbox is provided for that purpose).  The downside of that is that the metainfo (Felix's r+) is lost, so we have to note that in comments.

Comment 16

[Lars T Hansen]

(In reply to comment #11)
> 
> This is the main item I can think of:
> 
>   https://zerowing.corp.adobe.com/display/FlashPlayer/Tamarin+Coding+Standards
> 
> The relevant (but non-normative) rule on that page is "Spaces only please, no
> tabs"; as I mentioned, I was only inferring the effort to eschew dangling
> spaces from the behavior of Ed's fixtabs script.

At this point I consider "spaces only, no tabs" normative in MMgc code, since we went through and cleaned up all the files a few weeks ago.

I know there are settings in visual studio and xcode to make them insert spaces, and we have modelines for vi and Emacs in all files, so for the most part things are not terribly hard to manage.

Comment 17

[Ruchi Lohani]

Attachment: Fix profiler delete issue

I just figured out that something is wrong with my previous patch. When ResetStatics is called from GCHeap::GCHeap(), it will unnecessarily try to delete profiler which it should not. Adding a check to see if the profiler was initialized in the first place.

Comment 18

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

Comment on attachment 451695 [details] [diff] [review]
Fix profiler delete issue

Ah, delete -1, fantastic.  Looks good to me.

Comment 19

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

Ruchi: can you add a short comment, probably to the declaration of profiler,
indicating the invariant that the code is maintaining with respect to where (in
the important control flow points) profiler must be either NULL or a valid
MemoryProfiler*, versus where profiler must be either -1 or NULL?

My understanding is that it can be -1 or NULL when there is no GCHeap (and when
the GCHeap is in the midst of being constructed), but after the GCHeap has been
constructed, then it must be either NULL or a valid MemoryProfiler*.  And I
think (but am not 100% sure) that there is no contexts where it might be any of
the three -- but if this is true, it would be good to document it (and maybe
add the appropriate assertion; not as certain about utility of such assertions,
let Lars make call there).

Comment 20

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

(In reply to comment #19)
> And I think (but am not 100% sure) that there is no contexts where it might be any of
> the three

Of course, the whole point of your latest change is that ResetStatics() /is/ such a spot in the code.  I guess my point is that it sounds like ResetStatics() is the only function where the profiler can take on any of the three categories of value, and it would be nice to have a clear statement saying so.

Comment 21

[Ruchi Lohani]

Yes, ResetStatics is the only place where the profiler can be (-1: when called from GCHeap::GCHeap()) or (either NULL or valid MemoryProfiler* : when called from DestroyInstance(), the value profiler holds will depend on whether MMGC_MEMORY_INFO is defined or not). The check is mainly because the ResetStatics is called by both GCHeap::GCHeap() and GCHeap::DestroyInstance(). 

Possible assert could be in DestroyInstance() before calling ResetStatics() : 
GCAssert(IsProfilerInitialized());   //profiler is either NULL or a valid MemoryProfiler reference. I am not sure whether it will be useful in detecting any random behavior.

Comment 22

[Ruchi Lohani]

This has r+. 
Lars can you push it for me?

Comment 23

[Lars T Hansen]

Assuming ownership, will push.

Comment 24

[Lars T Hansen]

tamarin-redux changeset:   4876:e56e68d43913