629241 - Clean up Object creation infrastructure

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

The ways that an Object and Class are supposed to override construct() and createInstance are poorly defined and error-prone; examination of existing Flash/AIR glue code shows much copy-n-paste with questionable issues. This bug hopes to create a series of patches that will:
-- automate as much of this as possible (ie, have nativegen emit boilerplate construction code where possible) so that as little customization in embedder code is necessary
-- restructure the conventions to make incorrect usage impossible (and at least really hard)
-- as an added bonus, possibly improve object creation speed

Comment 1

[Steven Johnson]

Attachment: Eradicate most of construct() overrides outside the VM

As it turns out, almost 100% of the overrides to construct() outside of the VM are for a single purpose: namely, to throw kCantInstantiateError in order to simulate an Abstract Base Class.

This patches nativegen.py to emit a construct() method declaration (in DECLARE_SLOTS_) iff the class has customconstruct set in the metadata. (In debug builds, it also emits a stub construct() method that calls the superclass construct(), to catch cases where a construct has surreptitiously been thrown in without the customconstruct metadata being set. Note that these stubs mean that the GCC-specific checkConstructOverride code in MethodEnv.cpp had to go away, but that's OK, as this solution is arguably more robust.)

We also extend customconstruct to allow "none" as a value, which simply creates a stub that throws kCantInstantiateError (which has been moved into the VM).

(There's also a special case of customconstruct="instance" which allows both the class and instance to override construct(), used only for special cases of ClassClosure, FunctionObject, MethodClosure)

This ends up eliminating all but 3 construct() overrides in Flash/AIR: two for WeakMethodClosure/WeakFunctionClosure (which can and should be moved inside the VM anyway, see bug 500548), and JavaScriptBridgeObject (in AIR, which does real work and will require more study to eliminate).

Obviously this involves a great deal of churn in the Flash/AIR glue code (not in this bug for obvious reasons) but it's all mechanical and results in less explicit code, more auto-generated code, so that's a big win.

Assuming this passes muster, next step is to eradicate those last dangling construct() overrides if possible, then make it impossible to add new construct overrides; followed by a similar exercise on createInstance() [ie, eradicating unnecessary ones]

Comment 2

[Lars T Hansen]

Cool.

When you go over the code, can you check that kCantInstantiateError is used uniformly, that there are no exceptions?  Some preliminary scanning by Edwin at an earlier date suggested this wasn't necessarily so, but there's the confusion between "abstract classes" on the one hand and "private constructors" on the other hand, so it's possible that there was noise in the data.

I'm going to assume that kCantInstantiateError is "the" error to throw for the purposes of the abstract classes spec.

Comment 3

[Lars T Hansen]

BTW, kCantInstantiateError isn't an error to throw, it is an error code to attach to an error object to throw.  The error type thrown with kCantInstantiateError is invariably ArgumentError.

My hunch is that we would benefit from distinguishing carefully between "abstract class" and "private/protected constructor" during the rewrite, because these will probably both be language features in AS3 sometime in 2011 / by the time Nigel ships, at which point it might be useful to use those concepts directly in the glue.  (I know, compatibility.  Still.)

Comment 4

[Steven Johnson]

(In reply to comment #2)
> When you go over the code, can you check that kCantInstantiateError is used
> uniformly, that there are no exceptions?  

Where possible, yes, but throwing the "same" errors may be more appropriate. (It looks to me like kCantInstantiate is used almost exclusively.)

(In reply to comment #3)
> BTW, kCantInstantiateError isn't an error to throw, it is an error code to
> attach to an error object to throw.  The error type thrown with
> kCantInstantiateError is invariably ArgumentError.

Err, yes... do you think that "throwCantInstantiateError()" is misleading? I can rename it but it seems clear enough to me.

Comment 5

[Lars T Hansen]

(In reply to comment #4)
> (In reply to comment #2)
> > When you go over the code, can you check that kCantInstantiateError is used
> > uniformly, that there are no exceptions?  
> 
> Where possible, yes, but throwing the "same" errors may be more appropriate.
> (It looks to me like kCantInstantiate is used almost exclusively.)

The reason I'm asking is that I'm trying to define two AS3 facilities, one for private constructors and one for abstract classes, and one of the outstanding issues is to determine what errors to throw when they are abused, esp for abstract classes.  So I'm just looking for data.  If the data from the existing player code are ambiguous then the abstract class mechanism can be designed without any consideration that it will be used in the player code, ie, i can choose whatever error behavior I want.  But if the Player has uniform behavior that I can adopt then the player code can be simplified by using the new abstract classes (resp. private constructors).

> (In reply to comment #3)
> > BTW, kCantInstantiateError isn't an error to throw, it is an error code to
> > attach to an error object to throw.  The error type thrown with
> > kCantInstantiateError is invariably ArgumentError.
> 
> Err, yes... do you think that "throwCantInstantiateError()" is misleading? I
> can rename it but it seems clear enough to me.

No, that's fine, though my half-informed gut feeling is that you probably shouldn't implement that abstraction unless there's complete uniformity across the player code base (or you introduce versioned checks to create such uniformity going forward).

Comment 6

[Edwin Smith]

(In reply to comment #1)
> Created attachment 507333 [details] [diff] [review]
> Eradicate most of construct() overrides outside the VM

> This patches nativegen.py to emit a construct() method declaration (in
> DECLARE_SLOTS_) iff the class has customconstruct set in the metadata. (In
> debug builds, it also emits a stub construct() method that calls the superclass
> construct(), to catch cases where a construct has surreptitiously been thrown
> in without the customconstruct metadata being set.

Doing that should trigger an assert or C++ compile error, making it silently work is going in the wrong direction; it's hiding a mistake.  I still beleive this is the case even after we can generate the construct() method.

> Note that these stubs mean
> that the GCC-specific checkConstructOverride code in MethodEnv.cpp had to go
> away, but that's OK, as this solution is arguably more robust.)

Bug 575848 has rationale and background for why any mismatch between overriding of constrct() and the customconstruct metadata should be treated as an error.

Comment 7

[Edwin Smith]

(In reply to comment #6)
> (In reply to comment #1)
> > Created attachment 507333 [details] [diff] [review]
> > Eradicate most of construct() overrides outside the VM
> 
> > This patches nativegen.py to emit a construct() method declaration (in
> > DECLARE_SLOTS_) iff the class has customconstruct set in the metadata. (In
> > debug builds, it also emits a stub construct() method that 
> > calls the superclass
> > construct(), to catch cases where a construct has surreptitiously been thrown
> > in without the customconstruct metadata being set.

Hmm, maybe "to catch cases ..." means to make the C++ compiler (or linker) fail?  This is good, if its what you mean:  compiler error vs runtime assert.  But it leaves the user with no breadcrumbs for what to do to fix it (the assert did).

comments on the generated construct() implementation could be enough.

Comment 8

[Edwin Smith]

As part of, or a blocker, can we gather up whatever writings we have on native glue, and include them in a spec in the doc directory?  Then, mods like this one can include reviewable changes to the spec.

Its a new year, our culture needs to go this way anyway, this would be a solid step in the right direction.

Comment 9

[Edwin Smith]

(In reply to comment #2)
> Some preliminary scanning by Edwin at an earlier date suggested this
> wasn't necessarily so

I remember seing some inconsistency but I can't remember whether it was varying error code or varying error type, both, or how bad it was.  Any details I might have had would be on Bug 575848.  The dehydra scanner I used is attached there.

Comment 10

[Edwin Smith]

Comment on attachment 507333 [details] [diff] [review]
Eradicate most of construct() overrides outside the VM


customconstruct='instance' should only be allowed on a predefined list of types, and they all appear to be in the VM.  Can we make the tool barf on abuse?

Should we leave placeholders in the player's error constants list, so we dont accidentally define 2 enums with the same value?  (dehydra could check this).

Is there any safeguard against kNumErrorConstants having a too-small value?

Removing the assert will require updating this wiki page:
https://zerowing.corp.adobe.com/display/flashruntime/Coding+ActionScript+APIs
(ideally, that wiki could point to the spec file in third_party/avmplus/docs)

> #define AvmThunk_DEBUG_ONLY(...) __VA_ARGS__

Invariably, someone always trips up on use of varargs this way.  We've done it a lot in nanojit but I consider it an antipattern now.  I'm guessing you use the macro in generated code, so if its not too horrible, I'd suggest the generated code just use #ifdef _DEBUG instead.  The best justification would be if the macro were used to wrap an expression (and itself had to be in expression position in generated code), but that doesn't appear to be the case, looking at nativegen.py.

Comment 11

[Steven Johnson]

(In reply to comment #10)
> customconstruct='instance' should only be allowed on a predefined list of
> types, and they all appear to be in the VM.  Can we make the tool barf on
> abuse?

Good call, I'll do that as a followup.
 
> Is there any safeguard against kNumErrorConstants having a too-small value?
> 
> Removing the assert will require updating this wiki page:
> https://zerowing.corp.adobe.com/display/flashruntime/Coding+ActionScript+APIs
> (ideally, that wiki could point to the spec file in third_party/avmplus/docs)

I'll update the wiki.
 
> > #define AvmThunk_DEBUG_ONLY(...) __VA_ARGS__
> 
> Invariably, someone always trips up on use of varargs this way.  We've done it
> a lot in nanojit but I consider it an antipattern now.  I'm guessing you use
> the macro in generated code, so if its not too horrible, I'd suggest the
> generated code just use #ifdef _DEBUG instead.

Nah, the problem is that the generated code is inside a macro... putting #ifdef inside a #define doesn't work so well :-)

That said, I could always generate two versions of the macro (one ifdef DEBUG and one not), but I'd prefer to save that for if/when this actually breaks. (Plus, the debug-only emitting of this stuff may be transitory; once all classes are locked down we can probably remove these.)

Comment 12

[Steven Johnson]

> Is there any safeguard against kNumErrorConstants having a too-small value?

It's calculated by ErrorGen.as.

Comment 13

[Steven Johnson]

Comment on attachment 507333 [details] [diff] [review]
Eradicate most of construct() overrides outside the VM

TR 5842:ab93ecc8b313

Comment 14

[Steven Johnson]

(In reply to comment #6)
> Doing that should trigger an assert or C++ compile error, making it silently
> work is going in the wrong direction; it's hiding a mistake. 

It does generate a C++ compile error; you can't redeclare or redefine the construct method since we quietly provide a stub. So it's better than the status quo, actually (compile/link error vs runtime error).

(In reply to comment #8)
> As part of, or a blocker, can we gather up whatever writings we have on native
> glue, and include them in a spec in the doc directory? 

Good call. I'll start one.

Comment 15

[Steven Johnson]

Attachment: Remove naked calls to createInstance(), replace with newInstance()

createInstance() shouldn't ever be called directly, it should only be called via newInstance(), which feeds it the proper arguments. This cleans up existing uses to follow this rule. (Uses in Flash/AIR are trivial and will be easy to clean.)

Also corrects a few createIntance implementations that ignore the "delegate" argument and use prototypePtr() instead; note that an exhaustive survey of all call sites in Flash/AIR/avmshell showed that prototypePtr() is always the input arguments, so usage is equivalent.

Comment 16

[Steven Johnson]

Comment on attachment 509240 [details] [diff] [review]
Remove naked calls to createInstance(), replace with newInstance()

marking obsolete as I am going to upload a patch queue instead.

Comment 17

[Steven Johnson]

Attachment: createInstance removal part 1: Remove naked calls to createInstance(), replace with newInstance()

createInstance() shouldn't ever be called directly, it should only be called
via newInstance(), which feeds it the proper arguments. This cleans up existing
uses to follow this rule. (Uses in Flash/AIR are trivial and will be easy to
clean.)

Also corrects a few createIntance implementations that ignore the "delegate"
argument and use prototypePtr() instead; note that an exhaustive survey of all
call sites in Flash/AIR/avmshell showed that prototypePtr() is always the input
arguments, so usage is equivalent.

Comment 18

[Steven Johnson]

Attachment: createInstance removal part 2: move createInstance() definition to ClassClosure

Only ClassClosure (and descendants) ever use createInstance, so let's move it to be rooted there, which will make subsequent patches easier to follow.

Comment 19

[Steven Johnson]

Attachment: createInstance remove part 3: remove the VTable arg from the CreateInstanceProc

It's redundant, as it's *always* equal to cls->vtable->ivtable. Also convert the procedure to FASTCALL.

Comment 20

[Steven Johnson]

Attachment: createInstance remove part 4: have nativegen emit createInstanceProc stubs

Note that the stubs aren't actually being *used* for anything yet; that comes in the next patch. The idea, however, is that FooClass::createInstanceProc() is the unambiguous creation proc for FooClass. 

Note that a handful of vm-internal classes are special-cased, either because they are constructed in a special way, or because construction requires extra arguments; a survey of Flash/AIR shows that these will be unnecessary outside the VM proper.

Comment 21

[Steven Johnson]

Attachment: createInstance remove part 5: remove createInstance

removes all the virtual createInstance methods and uses the createInstanceProc stubs introduced last patch.

Also beefs up the nativegen emission to allow customcreateinstance="none", in which we autogenerate a stub which asserts/throws.

Of note: we now only need to replicate the old calculate-based-on-scope-chain in one case (creating a new class that is a nonnative class, ie a purely ABC-defined one).

Also note that we take an ugly approach to initializing the createProc for native classes: we jam it into the ivtable and have ClassClosure ctor extract it. This is ugly but is explicitly designed to avoid having to change the signature of all existing ClassClosure subclass ctors, which will cause huge code churn. (Admittedly, removing the createInstance methods will also do so, but removing code is much simpler than modifying code.) It is my hope that subsequent ANI work will make this hackery redundant, but in any event, it's hidden from user code, so the damage is modest.

Comment 22

[Lars T Hansen]

Comment on attachment 509246 [details] [diff] [review]
createInstance removal part 2: move createInstance() definition to ClassClosure

I'm a little unclear on the functional changes to ClassClosure::createInstance relative to the original version but I assume you have your reasons.

Comment 23

[Lars T Hansen]

Comment on attachment 509250 [details] [diff] [review]
createInstance remove part 4: have nativegen emit createInstanceProc stubs

In impossibleCreateInstanceProc, instead of AvmAssert(0) I find that it's more informative to do eg AvmAssert(!"Should not be invoked").  The latter allow eg QE to recognize the problem for what it is; less time is wasted.

All that said, is it correct to assert here?  Is this not simply a class whose constructor is private, and the only correct thing to do is throw an exception, and not to assert?  Similar in other cases, classes are either abstract or with private constructors, nothing stops user code (including test cases) from trying to construct them, but asserting would not be right.

Comment 24

[Lars T Hansen]

Comment on attachment 509255 [details] [diff] [review]
createInstance remove part 5: remove createInstance

"at a minimize" should be "at a minimum" (twice).

In ClassClosure: as a matter of style (and possibly IMHO) "friend" declarations should be at the class head, preceding all other code in the class body, including (in this case) "private" annotations and such.  Copious comments will make it plain why the friend declaration is there.

FunctionObject::FunctionObject over-indented?

Comment 25

[Steven Johnson]

(In reply to comment #23)
> In impossibleCreateInstanceProc, instead of AvmAssert(0) I find that it's more
> informative to do eg AvmAssert(!"Should not be invoked").  The latter allow eg
> QE to recognize the problem for what it is; less time is wasted.

Good point, will do.
 
> All that said, is it correct to assert here?  Is this not simply a class whose
> constructor is private, and the only correct thing to do is throw an exception,
> and not to assert?

Hmm... you have a good point; the main purpose of this stub is as a placeholder for nonconstructable items so that the code generator can always emit *something*; I originally put in the assert to verify to myself that we weren't ever getting here. Let me re-examine this assumption.

>  Similar in other cases, classes are either abstract or with
> private constructors, nothing stops user code (including test cases) from
> trying to construct them, but asserting would not be right.

True, but those don't (currently) go thru this path. That said, I've found that I need to implement abstract-base-class variants to complete this work (to regularize existing ad hoc usage in Flash) -- I'll post that as a followup patch and include possible cleanup to this site there (rather than revise this patch). Ok?

(In reply to comment #24)
> "at a minimize" should be "at a minimum" (twice).

Willfix.
 
> In ClassClosure: as a matter of style (and possibly IMHO) "friend" declarations
> should be at the class head, preceding all other code in the class body,
> including (in this case) "private" annotations and such.  Copious comments will
> make it plain why the friend declaration is there.

Willfix.
 
> FunctionObject::FunctionObject over-indented?

Willfix.

Comment 26

[Steven Johnson]

Attachment: createInstance removal part 6: eradicate customcreateinstance=true

Yes, we just added it, but inspection shows that everything that uses it could either use "none", or the new option, "cls", to pass the Class into the Instance int he ctor (needed by ByteArray and Vector). Add one special-case for Object itself, and now 100% of the createInstanceProc's are autogenerated. (This may seem a bit gratuitous but paves the way for the next patch, coming soon)

Comment 27

[Steven Johnson]

Attachment: createInstance removal part 7: collapse customconstruct and customcreateinstance into a single thing

Now that createInstance is 100% autogenerated, let's simplify the metadata by collapsing both of these into a single "construct" directive. In subsequent patches will add a few more construct directives that the Player needs.

Comment 28

[Steven Johnson]

Attachment: createInstance remove part 8: add construct="abstract"

Support for abstract base classes via metadata. Also restructured nativegen to avoid emitting redundant stubs for "impossible", etc., and to implement construct=none via a createInstance stub rather than a construct override.

Comment 29

[Steven Johnson]

Attachment: createInstance removal part 9: add "abstract-internal"

(hopefully, this is the last substantive patch... whew)

Flash has some objects with restricted inheritance (eg DisplayObject), which (in addition to being abstract base class) also prohibit subclassing by user code. I'm calling this "abstract-internal" for lack of a better term. 

They accomplish this by walking the hierarchy every time an object is created and checking for mismatched pools; we can do better by identifying this situation at class construction time.

Comment 30

[Steven Johnson]

Attachment: testcase for construct="none","abstract","abstract-internal"

Testcases are good.

Comment 31

[Steven Johnson]

It looks like I'm going to need one more: "internal", which is like abstract-internal without the abstract part (ie, class can be instantiated but subclasses are restricted to same file). I'll get to that monday.

Comment 32

[Steven Johnson]

Attachment: createInstance part 9 (v2): "restricted" and "restricted-internal"

Obsoleted previous patch:
(1) changed nomenclature from "internal" to "restricted" (too confusing vs as3 "internal")
(1) allowed "restricted" to be specified separately from "abstract"
(2) tweaked "restricted" to only restrict *immediate* children; grandchild and further down the line do not have restricions. (This is the semantic needed by existing Flash, as it turns out.)

Comment 33

[Steven Johnson]

Attachment: testcases for part 9

Comment 34

[Steven Johnson]

Attachment: createInstance part 10: construct="cls" is a bad idea

The previous patches had a subtle bug: class types all inherit directly from ClassClosure, not from the instance inheritance chain. e.g., say we have "class Child extends Parent", then the C++ ChildObject will extend ParentObject, but C++ ChildClass does NOT extend ParentObject -- they both extend ClassClosure.

This became a problem if ParentClass is native, and ChildClass is pure AS3: they both use ParentClass::createInstanceProc to create their instances, but it was typed as receiving a ParentClass argument (which is wrong for ChildClass). 

This becomes especially insidious if ParentClass has a bunch of static member vars, and ChildClass has none, and the (now-gone) construct="cls" object tries to peek at those member vars, which aren't there for ChildClass... kaboom.

Anyway... construct="cls" was really only useful for Vector, and I restructured that code to solve the problem in a different way.

Comment 35

[Steven Johnson]

Attachment: Rebased, combined patch

This is just a combination of all of the above patches, rebased to current tip; I originally had thought it would be easier to review the sequence of patches, but you might it easier to review in this form.

Comment 36

[Steven Johnson]

Attachment: createInstance part 11: add construct="check"

ChrisB points out that some classes need to be able to prevent construction in arbitrary runtime scenarios, and this scheme requires them to longjmp from the ctor, which is a bad idea due to partial-object-contruction (which would cause dtors pain during gc). So, this patch adds an optional pre-creation hook that a class can use to throw an arbitrary error *prior* to the ctor being called.

Comment 37

[Christopher A. Brichford]

Comment on attachment 511501 [details] [diff] [review]
createInstance part 11: add construct="check"

Looks ok to me.

Comment 38

[Christopher A. Brichford]

It just occurred to me that in a subsequent patch we should add a debug assert that asserts that the C++ constructor for an AS3 object never throws an AS3 exception.

Comment 39

[Lars T Hansen]

Looks plausible (based on combined patch).  One new FIXME in VectorClass.cpp - intentional?

Comment 40

[Steven Johnson]

(In reply to comment #39)
> Looks plausible (based on combined patch).  One new FIXME in VectorClass.cpp -
> intentional?

Yeah, it's intentional -- the problem is that newVector() clams to return ObjectVectorObject, but could return something else if you passed a different type. In practice this never seems to happen in extant usage, but it could. I didn't change it (yet) because there are a few dozen use cases in Flash glue that need changing. https://bugzilla.mozilla.org/show_bug.cgi?id=634007

Comment 41

[Steven Johnson]

Pushed all these as 5923:76950edc0724

Comment 42

[Tamarin Bot]

changeset: 5928:e1a6946d5a90
user:      Steven Johnson <[e-mail]>
summary:   Bug 629241 followup: MSVC linker will merge identical functions in release builds, so we can't use the function pointer to distinguish between not-creatable and abstract-base; add a flag to Traits for this purpose instead. (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/e1a6946d5a90