615830 - StringOutputStream is a train wreck of broken rules and wrong assumptions

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

This object is derived from OutputStream, which is a GCObject.  Yet StringOutputStream and OutputStream both have destructors.  Those destructors will not be called when the object is GC'd.  Yet, as the object is a GCObject, it may not be stack allocated - but code in StringOutputStream::writeN assumes that it may be, because there's a test there to do or not do a write barrier depending on whether it's heap-allocated or not.

In Tamarin, StringOutputStream is only used to define a member of StringBuffer (a subtype of PrintWriter, which is once again a GCObject with an inert destructor).  In turn, StringBuffer is always stack-allocated except in one instance - for some reason, ConsoleOutputStream decides to heap-allocate it instead of having it as a member.

The Flash Player uses StringBuffer, again always stack allocated except for its ConsoleOutputStream implementation.

See also bug #479431 which advocates removing the whole thing (though for different reasons).

Comment 1

[Steven Johnson]

Sweet! Can we add some crazy operator overloading to increase the fun? Maybe have "operator&" return a char* ?

Comment 2

[Edwin Smith]

See the string of denied patches and comments on bug 562480, for more support for removal or heavy-handed cleanup.

Comment 3

[Rick Reitmaier]

+1 for aggressive cleanup, but be warned you'll want to do so hand-in-hand with player code.  While we're at it, let's clean up our verbose output mechanism; and I'm not being sarcastic.

Comment 4

[Tommy Reilly]

I want a pony.

Comment 5

[Lars T Hansen]

+ Felix since he just ran into a related bug elsewhere.

Comment 6

[Lars T Hansen]

I'm starting to understand what needs to happen here. 

One aspect is that PrintWriter/StringBuffer are never themselves GC-objects, but that they can reference GC-managed resources that msut be kept alive if they do.  This means PrintWriter/StringBuffer must always be allocated on the stack, in a root, or inside a managed object, and never in FixedMalloc memory or global variables, say.

Another aspect is that there are two kinds of output streams, those that are GC objects and those that are not.  They do *not* share a common base class - it  is the central error of the current design that they do.  Having two kinds of streams adds a little complexity, but it's easy to control the complexity with a simple adapter class inside PrintWriter.

Comment 7

[Lars T Hansen]

Attachment: Patch

Needs careful review.

I need to vet this in the Flash Player but based on preliminary code inspection there should be no problems with the very slight API changes that are introduced here.

Comment 8

[Steven Johnson]

Comment on attachment 515337 [details] [diff] [review]
Patch

I'm a little unclear on why separate base classes is a good thing, here, but I'm willing to take your word for it, as you've thought about this nastiness a lot more than me.

It would probably be a good idea to test-build this in FRR before landing.

Comment 9

[Lars T Hansen]

(In reply to comment #8)
> Comment on attachment 515337 [details] [diff] [review]
> Patch
> 
> I'm a little unclear on why separate base classes is a good thing, here, but
> I'm willing to take your word for it, as you've thought about this nastiness a
> lot more than me.

It's not just a good thing, but a necessary thing.  Whether something is a managed object or not is part of the object's type.  I know it is that way but I'm struggling to explain exactly how it is that way...

If you look to C++ it's generally speaking wrong to apply the 'delete' operator to stack-allocated storage; similarly, it's incorrect for the program to expect a heap allocated object to be destroyed when a stack-allocated pointer to the object goes out of scope.  Storage management is in the type of the thing but it's hard to declare it as part of the type (except in Modula-3 where a subtype of OBJECT is on the heap and a RECORD is on the stack :-)  C++ makes us forget that because many types can be stack-allocated or heap-allocated and the destructor is the same in either case.  But with GC'd storage, where the GC expects that metainformation is stored with the object and that it must therefore have been allocated some special way, it's not true in any case.

> It would probably be a good idea to test-build this in FRR before landing.

I've fixed a couple of locations in AIR and the Player that needed fixing; will land those in FRR when this lands in TR.

Comment 10

[Tamarin Bot]

changeset: 6116:8f7f25fec789
user:      Lars T Hansen <[e-mail]>
summary:   Fix 615830 - StringOutputStream is a train wreck of broken rules and wrong assumptions (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/8f7f25fec789