647750 - ANI: move Proxy into VM

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Steven Johnson]

Proxy uses hooks that nothing else ever needs. It will be simpler in the long run to move it into the VM than to expose and support these hooks.

Comment 1

[Steven Johnson]

This will also require moving IllegalOperationError into the VM, along with a handful of error messages.

Comment 3

[Steven Johnson]

Attachment: Patch

Move Proxy, and necessary error classes and messages into the VM.

Does not add any testcases for it; a followup task will be for QE to identify & port testcases to avmshell.

Comment 4

[Lars T Hansen]

IIUC this is a new public API in the VM.  It therefore requires approval from the AS3 Spec Team.  (That it is an existing API that is being moved from the Flash Player does not nullify that requirement - possibly that strengthens the requirement.)

Comment 5

[Dan Smith]

I wouldn't have expected the AS3 spec team should spend time reviewing an existing API that is being relocated into the VM.  If the API were changing then the spec team should review, but this suggestion appears to add an unnecessary level of review.  Why would moving an API from the Player possibly increase this requirement?

Comment 6

[Lars T Hansen]

> Why would moving an API from the Player possibly increase this requirement?

Because the exiting Player API may not be an appropriate fit for the core language VM.  The whole point of having a spec review in the first place is to ensure that an API, as designed (or in this case ported), is a good fit with the existing core language.  For example:

* Player APIs typically do not provide prototype methods, while Tamarin
  APIs do.
* Player classes place their methods in the public namespace, while Tamarin
  places them in the AS3 namespace.
* Tamarin APIs are generally (not universally) more leniently typed than 
  Player APIs, possibly as a result of the former two points, possibly not.

If we're going to start migrating code with the Player's API style into the VM we should at least have a discussion about impacts.

Comment 7

[Edwin Smith]

Initial notes from a pass through the code.

the patch introduces flash.utils.Proxy to the VM codebase.  I believe
the only other example of a vm-builtin class not in the toplevel
namespace is flash.utils.ByteArray (but I haven't grepped deeply).

If there ever was a spec for Proxy (from the initial FP9 dev cycle) then we should find it and capture source material for the AS3 language spec and/or tamarin doc directory.

Proxy methods are in a dedicated namespace presumably to keep them out of the namespace of objects you'd want to proxy, including objects with methods in the AS3 or public namespace?  (see above about a missing spec)

  http://www.adobe.com/2006/actionscript/flash/proxy

Comments about ActionScript 2.0 and @playerversion and @langversion directives may not belong in the vm source code.  The precedent is established with ByteArray, but should be reviewed for Serrano.  how are the @ directives used?

"submit then add the tests later" seems to be asking for trouble.

Comment 8

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

(In reply to comment #7)
> Initial notes from a pass through the code.
> 
> the patch introduces flash.utils.Proxy to the VM codebase.  I believe
> the only other example of a vm-builtin class not in the toplevel
> namespace is flash.utils.ByteArray (but I haven't grepped deeply).

See also: IDataInput and IDataOutput interfaces which are in flash.utils package but are in VM codebase.

(They have documentation issues similar to the ones that Edwin pointed out.  Really its more of the same stuff that you already see with ByteArray.)

Comment 9

[Jeff Dyer]

Possibly naive question: what is motivating the move from Player to AVM? And, are we to take for granted that the API must be compatible with the player API, or can changes be made? I assume the former, which means API review is irrelevant. But then we might question the wisdom of the move in the first place. Which leads me back to the question of motive.

Comment 10

[Edwin Smith]

The move is because Proxy is tightly knit to the internals of how the VM works, and we'd like to a) not expose those internals in the player/avm API, and b) enable the JIT to optimize code paths going through proxy.  ByteArray was moved into the VM for essentially the same reasons.

Comment 11

[Steven Johnson]

(In reply to comment #7)
> "submit then add the tests later" seems to be asking for trouble.

Eh, agreed. I'll ping QE to see if I can get someone to extract the relevant tests from ATS.

(In reply to comment #10)
> The move is because Proxy is tightly knit to the internals of how the VM works,
> and we'd like to a) not expose those internals in the player/avm API, and b)
> enable the JIT to optimize code paths going through proxy.  ByteArray was moved
> into the VM for essentially the same reasons.

What he said -- as part of the ANI work we're trying to minimize the special-cases exposed outside the VM, and Proxy is just one big huge ugly special-case. Moving it into the VM is (IMHO) simpler and safer than supporting hooks for everything it needs to do. If there is serious pushback on this front, we could of course provide the hooks (some hookage will be necessary for other things that won't ever be moved into the VM) but Proxy strikes me as generally useful enough for any VM client that there's no serious downside.

Comment 12

[Jeff Dyer]

To summarize, an API review should answer these questions: does the current API make sense in the AVM? If so, what compatible extensions, if any, should we make? If not, how could we support the player API with a delegate or base class that implements to core functionality that the VM needs control over while minimizing risks introduced by a implementation architecture?

Comment 13

[Lars T Hansen]

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

GC annotations in ProxyClass worry me: Binding can sometimes hold a MethodEnv*, which would need to be traced, and indeed in this code the bindings are all method bindings.  So annotations seem to be missing.

Comment 14

[Lars T Hansen]

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

I'm not fond of the new Error API - it's incompatible with all the VM error classes in that there's no length property, no prototype.name - but I see we already have other player errors that are similarly incompatible, so what can I do.  It also makes scant sense to label their constructors as "private" when they are clearly public.  Private to whom?  Why?

This brings me around to echoing the other reviewers on the documentation for the Proxy class, which is not good.

* Clearly Proxy has a constructor, because otherwise the main example would
  not make sense, but it is effectively protected.

* Why is there a reference to the "is" operator in the main doc comment?
  There are no overrides here for "is" and "as".

* There are cross references to files that presumably don't exist, eg
  "@see ../../statements.html".

* There are references to the ECMAScript spec, but that scarcely makes sense
  because those algorithms (eg [[Get]]) are only defined for property names
  that are strings.  But the APIs here are defined with name:*.  So what are
  the constraints on the name parameter?  Are there any?

* I don't mind the reference to the playerversion but the "Lite 4" strings
  could at least be formatted along with the others, not set off and indented.

* For getProperty we read that "If the property can't be found, the method 
  returns undefined".  Obviously it should be "must return undefined".  Ditto
  for several of the others.

* The documentation for callProperty is ... confusing.

* Interpretation of the return type of the delete operator is tricky (and
  AS3 does not follow ECMAScript).  What the function should return must be
  discussed.

* Constraints on return values, eg, getDescendants?

* isAttribute documentation is bizarre.

I could go on.  It needs work.

The API itself seems OK.

It's probably OK that "is" and "as" and operations on types / conversions are left out.  Objects of proxy types can model objects of other types only if code manipulating the objects don't look too closely.

(BTW I see some tabs in the patch.)

SR- for the documentation issues.

Comment 15

[Steven Johnson]

(In reply to comment #13)
> GC annotations in ProxyClass worry me: Binding can sometimes hold a MethodEnv*,
> which would need to be traced, and indeed in this code the bindings are all
> method bindings.  So annotations seem to be missing.

Good point... this is how the code was in Flash, so it's no worse, but it's still suboptimal. That said, these Bindings will all go away once Bug 634638 lands, so it may be acceptable to leave this class as non-exactgc till then with a comment about this wart.

(In reply to comment #14)
> I'm not fond of the new Error API - it's incompatible with all the VM error
> classes in that there's no length property, no prototype.name - but I see we
> already have other player errors that are similarly incompatible, so what can I
> do.  It also makes scant sense to label their constructors as "private" when
> they are clearly public.  Private to whom?  Why?

This was pretty much a straight migration of existing Player code, with only trivial cleanup -- I hadn't noticed the discrepancy. Will revisit.

> This brings me around to echoing the other reviewers on the documentation for
> the Proxy class, which is not good.

I didn't review this at all -- but it's (supposedly) the source for our public AS3 documentation.
 
> I could go on.  It needs work.

I won't argue with that, but it's unclear to me who owns this documentation, how up to date (or not) it might be, who Flash stakeholders are.... etc.

Comment 16

[Lars T Hansen]

(In reply to comment #15)
> (In reply to comment #13)
> > GC annotations in ProxyClass worry me: Binding can sometimes hold a
> > MethodEnv*,
> > which would need to be traced, and indeed in this code the bindings are all
> > method bindings.  So annotations seem to be missing.
> 
> Good point... this is how the code was in Flash, so it's no worse, but it's
> still suboptimal.

I've forwarded the issue to Ruchi.

> That said, these Bindings will all go away once Bug 634638
> lands, so it may be acceptable to leave this class as non-exactgc till then
> with a comment about this wart.

OK.

> > I could go on.  It needs work.
> 
> I won't argue with that, but it's unclear to me who owns this documentation,
> how up to date (or not) it might be, who Flash stakeholders are.... etc.

No doubt that's all tricky.  But having documentation that at least says what's going on seems to be the least we should aim for.

Comment 17

[Tommy Reilly]

A couple months ago I proved to myself that Bindings were always indices and removed all write barriers for them (during the MulitnameHashtable write barrier cleanup).   I logged a doc bug to clean up the comments that say Binding's carray MethodEnv's.   I'll dig it up.

Comment 18

[Tommy Reilly]

https://bugzilla.mozilla.org/show_bug.cgi?id=613196

Comment 19

[Ruchi Lohani]

Tom is right I think. Bindings seem to be indices. So ProxyClass annotations are fine. Player crashes with Binding annotations as those are just indices and not valid address.

Comment 20

[Steven Johnson]

(In reply to comment #7)
> the patch introduces flash.utils.Proxy to the VM codebase.  

FWIW, we could probably make this class be "avmplus.Proxy" (or some such), then have flash.utils.Proxy remain in Flash proper as a simple subclass of it. (There's the slight chance that existing content would break, if it somehow assumed the superclass of flash.utils.Proxy, eg by describeType() usage.)
 
> If there ever was a spec for Proxy (from the initial FP9 dev cycle) then we
> should find it and capture source material for the AS3 language spec and/or
> tamarin doc directory.

Before my time -- no idea where it might be. Would you have any clues on this matter?
 
(In reply to comment #14)
> I'm not fond of the new Error API - it's incompatible with all the VM error
> classes in that there's no length property, no prototype.name - but I see we

actually, there is a prototype.name, it's just initialized out of line. I'll fix that. I'll add length too to bring things into symmetry.

> It also makes scant sense to label their constructors as "private" when
> they are clearly public.  Private to whom?  Why?

I *think* that doxygen comment actually means "private" in the sense of "do not document". That said, it's clearly wrong in the context of our code, I'll just remove those comments.

Comment 21

[Steven Johnson]

So I started to take a stab at improving the documentation issues Lars itemized in Comment 14, but we may have a can of worms here... this doxygen commenting is used to generate the public AS3 documentation for various Flash help files & sites; I'm pretty certain that if we change it without input from the relevant Documentation folks we'll get major pushback. (IMHO this documentation doesn't belong in Tamarin in doxygen form at all, but I don't know how else we can maintain this.)

Unfortunately I have no idea who writes / maintains / etc these comments, so I'm not sure how to proceed... does anyone know who the relevant doc gurus are?

Comment 22

[Dan Smith]

RIchard Whitley is the foundation team LR contact.  I'd retargeted this bug for Anza as we can deal with the  doc and any l10n issues for Serrano.

Comment 23

[Steven Johnson]

OK -- this is not an urgent work item, I'm just trying to keep it from going stale

Comment 24

[Steven Johnson]

Attachment: Patch v2

Updated version of existing patch; essentially the same with a few minor tweaks, but mostly just rebased to current tip

Comment 25

[Chris Peyer]

Attachment: Proxy tests ported from the ATS

testProxySmoke and testProxyAdvanced

Comment 26

[Chris Peyer]

Attachment: All ATS Proxy tests ported to tamarin

Comment 27

[Steven Johnson]

(In reply to comment #22)
> RIchard Whitley is the foundation team LR contact.  I'd retargeted this bug for
> Anza as we can deal with the  doc and any l10n issues for Serrano.

LR has indicated that classes in Tamarin are tracked separately, so we can simply remove the documentation from these active files (they have shadow copies elsewhere that update the docs). I'll prepare a revised patch.

Comment 28

[Steven Johnson]

Attachment: Patch v3

Updated patch, with all doxygen comments removed entirely, and testcases rolled into patch.

Comment 29

[Lars T Hansen]

Comment on attachment 528982 [details] [diff] [review]
Patch v3

R+ for the code etc, looks pretty clean.

From a spec point of view it's clearly not good that we don't have a spec for Tamarin's Proxy class.  A proper API review will not be possible without such a spec.  This makes me fairly worried both in terms of assessing current soundness and completeness and future extensions (hello, Vector).  I know the class "is what it is" but that should not stop us from checking that it does in fact make sense.

Comment 30

[Steven Johnson]

TR 6252:70d04f4f43d9

Comment 31

[Tamarin Bot]

changeset: 6300:367ab10a2411
user:      Brent Baker <[e-mail]>
summary:   Bug 647750: count the ProxyGlue object size towards the vm in the size report (r=brbaker)

http://hg.mozilla.org/tamarin-redux/rev/367ab10a2411