398920 - Need to disallow native methods as object constructors

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Tommy Reilly]

Somehow this became common place, even though we didn't intend to allow it.  If an object has slots we generated initialization abc instructions and chain with the AS3 constructor, this crashes if there is no AS3 constructor.   We could fix it but the preferred mechanism is to write the constructor in AS3 and call a private native function to do any native work.

Comment 1

[Steven Johnson]

Ouchie. No argument here; there will be some code churn as a fair about of native glue code uses this, but better safe than sorry. Anyone think we shouldn't disallow this?

Changing nativegen.py to forbid it is trivial, changing the existing glue code is simple but a pain.

Comment 2

[Edwin Smith]

Why forbid a native constructor when all we need is to provide a way for the native constructor to call super()?  Such a hook should be easy to provide (and wrap), for reference see the code in Interpreter.cpp for INSTR(constructsuper).  

of course, if as3->native->as3 call paths are to be avoided, then using a helper function is better.  but i'm proposing a less boil-the-ocean fix.

Comment 3

[Steven Johnson]

I don't think there are a lot of existing native constructors, but changing them isn't too hard. Agreed that adding a way to call super() from native would work, but it would still be error prone (what if you forget?)... also, avoiding unnecessary as3->native->as3 call paths is IMHO a good thing to reduce stack usage.

Comment 4

[Stan Switzer]

Agreed with Steven.  You could fail to call it, or you could call it wrong. Letting the compiler generate the super() call is correct by construction.

BTW, it just occurs to me that classes with native constructors will probably not even invoke their own member initializers, so it's not just a superclass constructor issue.

Comment 5

[Steven Johnson]

Attachment: Patch

Enclosed is a patch that disallows native ctors. There are only two such in the avmshell (Dictionary and Domain). Flash only has 16 by my count so adjusting it should be easy enough.

Comment 6

[Lars T Hansen]

I might object to outlawing native constructors if outlawing them effectively makes a change to a published language specification.  (On the other hand, the only software to be affected would be embedders of Tamarin or FP source code, and presumably there aren't many of those besides ourselves.  So it wouldn't be a strong objection.)

Comment 7

[Steven Johnson]

various changes in redux have already affected embedders in similar ways, so that isn't much of an issue. Also keep in mind that the "published specification" for embedders of Tamarin has been effectively nonexistent up to now ... we very much need to create and document a maintainable embedding API (which would include a native-class mechanism that doesn't require exposing all the internal details of the VM, as it does now.)

In the meantime, I have a review+ ... any strong objections to pushing this?

Comment 8

[Steven Johnson]

pushed to redux as changeset:   1135:d102d833fa14