535770 - avm support for C++ versioning, testing, etc

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

This is a tracking/parent bug for all bugs having to do with:

- supporting versioned C++ code in the VM in a tidy manner, including APIs
  that allow the player to ask for particular versions

- providing command line arguments to the avmshell to allow coordinated
  testing of versioned ABC / C++ code, for every specific version

- integrating this mechanism with the Flash Player

- etc

Comment 1

[Lars T Hansen]

In an email thread I wrote:

"* We need a reasonable way to version bugs in the C++ and AS3 code; the current system invented by Jim - for C++ only - won't last.  For AS3 we'll naturally build on Jeff's stuff. 

* Once we have a way to version bugs on both C++ and AS3, we need a way whereby we can request the correct version of code at the shell by passing it a command line parameter.  This switch should be sure to coordinate the C++ and AS3 code obviously.  We need to decide the granularity but it'll probably be synced with Flash versions. 

* We need to be sure for Flash to set the versions on the VM correctly; presumably it now reaches in and sets that one bug flag that Jim implemented.  That has to go."

In the same thread Jeff wrote:

"As you probably know, avmshell currently has a command-line flag '-api N' used to test api versioning (that's in part what one of these bugs is about). The value of N ranges from 660 to 670 which align with the FR version ids. This probably deserves some generalization, but it would be nice if api versioning and dynamic versioning shared the same values andtate internal to the AVM. Unfortunately, AIR and FP runtime versioning (as opposed to api versioning) do not share the same numbering system.  AIR versions range from 660 to 670 and FP ranges from 9 to 10 (or at least until they account for Argo). API versioning normalizes the FP values to the AIR values when code is loaded, but non-api version checks are still done with the old FP numbers (9, 10, ...)."

Comment 2

[Lars T Hansen]

(I've recorded some bugs that need versioning as dependencies, but there are many more.)

Comment 3

[Lars T Hansen]

An additional concern here is that we need support in the test suite.

The design will likely be that we want to pass an argument to the vm shell, eg -api=FP_10_0.  But the test cases we should run should only be those tests that were "defined" for that version; ie, if we fix a bug and add versioning support in the AS3 and C++ code, we will not run that code when running the shell on an earlier API, and so any test code that lands for the bugfix must also not be run.

This probably means a couple of things.  runtests.py must be version-aware, and when providing eg --api=FP_10_0 to runtests.py it needs to do the following:

- if a test was added for a later bugfix then it either should not be run, or
  its buggy result should be considered the correct result for that version

- if a test case affects subsequent tests in its file (eg by crashing or
  throwing an exception) it needs to be factored into a separate file so that
  the running of version-oblivious tests is not affected by versioned tests.
  this really ought to be happening already, maybe it is, but we have to be
  more systematic about it.

- versioning info must be noted carefully when landing new test cases.

Comment 4

[Lars T Hansen]

Attachment: Sketches

Comment 5

[Lars T Hansen]

Attachment: Now with an AS3 API

This isn't quite right, Object.AS3::bugzilla() should really be hidden better, eg, should be using the FP_SYS API version, say.  Need to investigate that more.

Comment 6

[Lars T Hansen]

Attachment: The bug fix for 504525, with AS3 versioning

(C++ versioning was demonstrated in the earlier patch and is largely compatible with the older system anyway.)

Comment 7

[Lars T Hansen]

As a general remark, the main design point in the sketch is how to set compatibility flags as a group.  I figure there are two approaches, one where the set is derived from the API string by some fashion (requires extensions to the API system) and one where a group of fixes for a named Player version are requested explicitly.  The latter fits in with the current structure of the Player and is non-mysterious, which fits my temperament, so that's what I've gone with.  A small change to PlayerAvmCore::SetAVMBugFlags will be required, since that function currently reaches in and sets flags directly.

The main reason to object to my approach is that it exposes player version names in Tamarin.  However, the only way I know to avoid that would be through a level of indirection, where the bug flags to be turned on for a particular version are encoded outside the source code, typically in the API config file or a similar file, and where some code generation based on that makes that list manifest in Tamarin.  That seemed unreasonably obscure to me.

Discussion, please :-)

Comment 8

[Lars T Hansen]

To find all bugs that need versioning, go to advanced search, then at the bottom of the page in the section for "Advanced Searching Using Boolean Charts" select

  Flag | is equal to | flashplayer-needsversioning+

Comment 9

[Steven Johnson]

Generally, I like this a lot. However:

-- I'm a little confused about who is expected to be the caller of setBugCompatibilityFlags; AvmCore::handleActionBlock() calls it, which I would expect to be adequate, but so does ShellCore, which seems redundant and confusing to me.

-- setBugCompatibilityFP_9_0 (etc) assume that no other calls have ever been made; we should either add an assert to ensure that at most one call is made per pool, or allow multiple calls (via setting all flags explicitly). The first sounds better to me... 

-- in fact, since we're requiring subclasses to override setBugCompatibilityFlags, why not simply have it return the bugset expected (as an enum)? That would allow us to keep the APIs in PoolObject completely private to the VM.

Comment 10

[Lars T Hansen]

(In reply to comment #9)
> Generally, I like this a lot. However:
> 
> -- I'm a little confused about who is expected to be the caller of
> setBugCompatibilityFlags; AvmCore::handleActionBlock() calls it, which I would
> expect to be adequate, but so does ShellCore, which seems redundant and
> confusing to me.

It's not redundant.  The builtin and shell (or playerglue) classes are not processed through handleActionBlock.  This split, where the builtins must be handled separately, is common throughout the avm interface code in my experience and it seemed OK to just propagate that meme.

> -- setBugCompatibilityFP_9_0 (etc) assume that no other calls have ever been
> made; we should either add an assert to ensure that at most one call is made
> per pool, or allow multiple calls (via setting all flags explicitly). The
> first
> sounds better to me... 

Agree.

> -- in fact, since we're requiring subclasses to override
> setBugCompatibilityFlags, why not simply have it return the bugset expected
> (as an enum)? That would allow us to keep the APIs in PoolObject completely
> private to the VM.

Do you think that even in light of the explanation of the non-redundancy of the calls as outlined above?

Comment 11

[Steven Johnson]

(In reply to comment #10)
> It's not redundant. 

Ah. In that case, I think maybe we should enforce that it's called exactly once per pool (ie, an embedder can't overlook it somehow), rather than leaving things to chance.

> Do you think that even in light of the explanation of the non-redundancy of the
> calls as outlined above?

Nope.

Comment 12

[Lars T Hansen]

So the way the bugzilla function is going to appear in Object is like this:

  [API(CONFIG::AIR_SYS)]
  public static native function bugzilla(n:int):Boolean;

That works fine but it means we have to clean up various things around versioning that we had put off so far.  Specifically:

- API versioning is always enabled in the shell, the stuff in avmfeatures.as
  goes away.

- Object.as must include api-versions.as, but that file currently lives in
  the flash player code

- The api-versions.as file should be generated by the apivergen.as script

- We should pow-wow with the Flash people to agree on where the .xml input
  file and the .h and .as output files live, ie, whether the Flash player
  copies the stuff from Tamarin or vice versa.  Some precedent for the former
  exists.

Comment 13

[Lars T Hansen]

Attachment: More developed still

Essentially the same as the two previous patches but with sketches for using API versioning to hide Object.bugzilla from general view.  (Probably a big patch as it includes changes to builtin.abc, etc.)

Comment 14

[Edwin Smith]

(In reply to comment #7)
> As a general remark, the main design point in the sketch is how to set
> compatibility flags as a group.  I figure there are two approaches, one where
> the set is derived from the API string by some fashion (requires extensions to
> the API system) and one where a group of fixes for a named Player version are
> requested explicitly.  The latter fits in with the current structure of the
> Player and is non-mysterious, which fits my temperament, so that's what I've
> gone with.  A small change to PlayerAvmCore::SetAVMBugFlags will be required,
> since that function currently reaches in and sets flags directly.
> 
> The main reason to object to my approach is that it exposes player version
> names in Tamarin.  However, the only way I know to avoid that would be through
> a level of indirection, where the bug flags to be turned on for a particular
> version are encoded outside the source code, typically in the API config file
> or a similar file, and where some code generation based on that makes that list
> manifest in Tamarin.  That seemed unreasonably obscure to me.
> 
> Discussion, please :-)

Its a reasonable tradeoff.  A hypothetical longer term requirement is for N (>> 1) embedders, to have many differet bug sets, and this could introduce a scaling problem (should all embedders get add functions in tamarin for this?).  Not all embedders are necessarily even Adobe products.  But, I'm willing to worry about that later if it gets bad.

In a future code base, with 100's of fixes, will we wish we had reversed the sense of the flags?

    void PoolObject::setBugCompatibilityReduxLatest()
    {
        setBugCompatibilityFP_10_0();

        bugzilla442974 = 1;
        bugzilla478804 = 1;
        bugzilla504525 = 1;
        bugzilla538107 = 1;
        /* and hundreds more */
    }

I don't think so, I think its right as-is.  each new group can use the previous group, then add more.  so we'll only have hundreds in one group, if that many were fixed in one tamarin release.  this situation is better than having to go back to old version sets, to add more flags, if the default were "fixed" and old version sets had to opt-in to get the broken behavior.

Comment 15

[Edwin Smith]

Should the AS3 api be a global function instead of a static method on Object?  call sites will be slightly smaller (abc size), with no increase in api exposure afaict.

Comment 16

[Edwin Smith]

Comment on attachment 439665 [details] [diff] [review]
More developed still

A nice-to-have followon bug could enable swf version detection and enable the versioning mechanics for  abc's with native methods in -Dverifyonly mode.   It would also be nice to support versioning in the SWF file support in the shell, generally.

Another followup bug should add JIT smarts to assume the bug flags are readonly, and to optimize ABC code using them.  Some fixes will likely be in hot builtin abc code.

Don't forget to "hg rm" noapi-versions.h

As a style guideline I prefer this:

    if (bugXXX) { /* new code */ } else { /* old code */ }

Over this (which is what we have in XMLObject.cpp):

    if (!bugXXX) { /* old code */ } else { /* new code */ }

(not worth changing whats there just for that reason though).

Comment 17

[Lars T Hansen]

(In reply to comment #15)
> Should the AS3 api be a global function instead of a static method on Object? 
> call sites will be slightly smaller (abc size), with no increase in api
> exposure afaict.

I actually made a small adjustment over the weekend but not in this direction: it turns out that the AS3 api probably needs to be an instance method on Object, because the bug flags are relative to the pool in which the actual type of the object lives.  So it can't be static or global, or at least that's my understanding so far.

Comment 18

[Edwin Smith]

Hmm, good point.  By checking the pool of the concrete "this" instance, are you approximating the pool of the user-code?  that implies we support different pools in the same vm instance to have different bug compatibility sets enabled.  Is that the same granularity that api versioning uses?  I think it is.  is that good/bad?

example callstack:
Object  // builtin.abc <-- bugzilla() lives in this abc, whether static/global/instance
DisplayObject // avmglue.abc   <--- this code calls bugzilla()
MyApp // app.swf    <--- this is what determines API version, IIRC
Widget // sdk.swf <-- slave to MyApp.swf's api version, even if different, IIRC

Which abc (pool) do we want bugzilla() to check?
a) always builtin.abc
b) always avmglue.abc (or shell.abc)
c) always the pool calling bugzilla()
d) always the first non-builtin pool calling bugzilla() (dynamically scoped via MethodFrame)
e) all pools get the same bits so it doesn't matter ... move bits to AvmCore?

Minor point: the jit optimization i suggeted is only easy with a, b, or c.  As a precedent, security-context queries are typically (always?) (D).

Comment 19

[Steven Johnson]

(In reply to comment #18)

> d) always the first non-builtin pool calling bugzilla() (dynamically scoped via
> Minor point: the jit optimization i suggeted is only easy with a, b, or c.  As
> a precedent, security-context queries are typically (always?) (D).

Correct, always (d)

Comment 20

[Lars T Hansen]

The AS3 API just mirrors the C++ API, which goes directly to the pool.  So let's leave the AS3 API out of it for now.

As for the C++ API, I simply mirrored what was there before (the fix from you and Jim): the flags were on the pools.  This is probably correct: if you load a swf of an older version as a sub-swf of a newer version, the old code should run with old (presumably buggy) semantics.  That means it's important to query the right pool object for every method.

There's an interesting wrinkle however, in that if user code of version m < n calls a builtin method on an instance of a class that was not subclassed by the user code, then the user code of version m will get a method of version n.  This is sort of hokey: the builtins have semantics that mirror the SWF version, and dynamically loaded code of different versions may get different behavior depending on whether they create instances of system classes or their own classes.  It's sort of consistent but it's not great.

Comment 21

[Lars T Hansen]

(Note that bugfix version checking and API versioning are orthogonal; the former is about behavior for a given method or algorithm, the latter is about visibility of names.  When I write 'versioning' in the context of this bug I always mean the former.)

In a nutshell, we need to do more thinking here, and in particular we need to frame the problem carefully with use cases.

I've talked to Lee, Jim, and Deneb.  While Lee thought it would be OK if there was some breakage if SWF 1 of version x loaded SWF 2 of version y and the latter inherited some of the former's behavior, Jim and Deneb did not agree. 

The baseline that I believe everyone agrees on is (a) only builtin methods - and sometimes algorithms - are subject to bugfix version checks and (b) if a player of any version x loads one or more swfs all of version y s.t. y <= x then the behavior observed by all code in that swf set is equivalent to the behavior exibited by a player of version y loading the swf set.  The problem /only/ becomes difficult when a swf set containing swfs of multiple versions is  loaded into a VM core.

Some questions are:

 - If m is a versioned method on builtin class C s.t. m's behavior differs
   between x and y, and SWF 1 and SWF 2 both instantiate C directly, which
   method behaviors do SWF 1 and SWF 2 observe on their respective instances?

 - If SWF 1 instantiates C and SWF 2 calls m on it, what behavior does SWF 2
   observe?  Ditto in the other direction.

 - If SWF 1 subclasses C with D and then instantiates D and calls m on
   the latter instance, what behavior does it observe?  What if it passes
   the object to SWF 2 and SWF 2 calls m?  Ditto in the other direction.

I think it will be very useful to leave discussions of mechanism aside while we focus on the desirable behavior.

Comment 22

[Lars T Hansen]

BTW Jim and Deneb appeared a bit upset with the current implementation of ScriptPlayer::SlowCalcScriptPlayerVersion, so there may be issues on the Player side to deal with as well before testing an eventual mechanism.

Comment 23

[Deneb Meketa]

I'll try and weigh in on several questions:

- Caller vs. instantiator: usually in Player, we have made versioning decisions based on who is calling, not who instantiated.

- Initial versioning mechanism: CalcScriptPlayerVersion should be renamed CallHereForAWrongAnswer.  I would much prefer determining the current SecurityContext and finding version from that.

- Live versioning mechanism: why not track this as part of CodeContext?  A CodeContext could include a pointer to a set of bug flags.

- Importance: If you told a bunch of our customers that, when their content was loaded alone, versus when it was loaded by other content of a different version, its behavior would change... you might have a revolt.  I think it's imperative to ensure that a SWF's own version is respected in making compatibility decisions, regardless of what else is going on.  Mixing SWF versions is commonplace.

Comment 24

[Lars T Hansen]

Attachment: More developed still

For safekeeping only, collects all work so far.

Note that a number of the changes here are also offered on bug #559994 and bug #51178 and will likely land independently (as they solve other unrelated problems).

Comment 25

[Steven Johnson]

(In reply to comment #20)
> There's an interesting wrinkle however, in that if user code of version m < n
> calls a builtin method on an instance of a class that was not subclassed by the
> user code, then the user code of version m will get a method of version n. 

I think the upshot is that the versioning needs to follow the same rules as CodeContext: behavior is defined by the most lowest non-builtin code on the callstack. This means (sadly) that bugs in builtin code must always examine the current CodeContext for their behavior (thus precluding JIT optimizations, alas). This may or may not be a performance issue, but I'm inclined to avoid premature optimization panic in this case. 

If this is the case, then the obvious approach would be to actually use CodeContext for bugflag lookup. This will require some augmentation of CodeContext but nothing too dramatic I think. (On the plus side, this would mean that CodeContext actually gets exercise in a meaningful way in avmshell, which isn't currently the case...)

(In reply to comment #21)
> The baseline that I believe everyone agrees on is (a) only builtin methods -
> and sometimes algorithms - are subject to bugfix version checks  

Agreed.

> and (b) if a
> player of any version x loads one or more swfs all of version y s.t. y <= x
> then the behavior observed by all code in that swf set is equivalent to the
> behavior exibited by a player of version y loading the swf set. 

Not sure I agree; the ideal here is that the swfs of version y see behavior compatible with Player Y, and the swfs of version x see behavior compatible with Player X. What you have described above appears to suggest that swf x will see behavior of Player Y (ie, forcing the lowest-version behavior), which sounds unacceptably wrong to me.

>  - If m is a versioned method on builtin class C s.t. m's behavior differs
>    between x and y, and SWF 1 and SWF 2 both instantiate C directly, which
>    method behaviors do SWF 1 and SWF 2 observe on their respective instances?

IMHO: if the most recent caller of m is from SWF 1, then m should behave according to SWF 1's version rules. (ditto for SWF 2.) 
 
>  - If SWF 1 instantiates C and SWF 2 calls m on it, what behavior does SWF 2
>    observe?  Ditto in the other direction.

IMHO: SWF 2 should always see the behavior from SWF 2's version, regardless of who instantiated the instance.
 
>  - If SWF 1 subclasses C with D and then instantiates D and calls m on
>    the latter instance, what behavior does it observe?  What if it passes
>    the object to SWF 2 and SWF 2 calls m?  Ditto in the other direction.

Assuming that m has not been overridden my D, I think the same rules as the previous example apply: instantiator does not matter, only the most-recent-caller matters.
 
> I think it will be very useful to leave discussions of mechanism aside while we
> focus on the desirable behavior.

I agree. (FWIW, though, I think the CodeContext approach mentioned above will provide what I think is the desirable behavior.)

(In reply to comment #23)
>  I think it's
> imperative to ensure that a SWF's own version is respected in making
> compatibility decisions, regardless of what else is going on.  Mixing SWF
> versions is commonplace.

Agree 100%.

Comment 26

[Edwin Smith]

Have we ruled out a scheme whereby versioning only depends on load order and the domain tree, and not the call stack?  With the call stack being inspected, there is much more chance for dynamicaly changing behavior, and of course its harder to make fast (agree about premature optimization, but still).

IIRC the traditional approach has been that the first movie loaded dictates what children see, and its independent of call stack.

Comment 27

[Edwin Smith]

nevermind, i'm re-reading old comments and my question was addressed previously.

Comment 28

[Rick Reitmaier]

Good point though, there is a lot of information buried in the comments.  

Once we're ready, an attachment containing a summary of the proposal (incl. justification / comment references) might prove useful.

Comment 29

[Steven Johnson]

(In reply to comment #28)
> Once we're ready, an attachment containing a summary of the proposal (incl.
> justification / comment references) might prove useful.

At this point, I really need some feedback from Flash/AIR principals... I'll nudge them.

Comment 30

[Stan Switzer]

Actually, Bugzilla should be a class with properties for each bug flag:

 [API(CONFIG::AIR_SYS)]
public class Bugzilla 
{
    static private function getBugFlag(i:int):Boolean;
    static const bugFoo:Boolean = getBugFlag(0);
    static const bugBar:Boolean = getBugFlag(1);
    etc
}

This leads to clearer and faster code since you can use property accessors instead of native function calls.

I'm pretty sure that compatibility being determined by the instantiating toplevel is every bit as satisfactory as climbing back the invoker chain.  Perhaps more so, frankly.

Comment 31

[Stan Switzer]

> I'm pretty sure that compatibility being determined by the instantiating
> toplevel is every bit as satisfactory as climbing back the invoker chain. 
> Perhaps more so, frankly.

This contradicts Deneb, but so be it.  I think it's better to be able to reason about behavior for each instantiation of code than to reason about behavior for every invoker of the same code. It's just plain weird if in a single instance of a class the behavior of a function depends on who invokes it.  Especially if it modifies the (shared) state of the class.

You'd go insane trying to worry about the combinatorics.  Or you would if you worried about it.  More likely you'd just assume it was OK and be wrong sometimes.

Comment 32

[Steven Johnson]

(In reply to comment #31)
> This contradicts Deneb, but so be it.  I think it's better to be able to reason
> about behavior for each instantiation of code than to reason about behavior for
> every invoker of the same code. It's just plain weird if in a single instance
> of a class the behavior of a function depends on who invokes it.  Especially if
> it modifies the (shared) state of the class.

It's easier to reason about, but gives a totally unsatisfactory result when combining SWF of different versions; it means that (say) SWF10.2 could behave differently when loaded by SWF10.0 (vs. being loaded toplevel). I'm totally with Deneb here.

Comment 33

[Steven Johnson]

Attachment: More developed still (rebased)

(Same patch, just rebased to current tip)

Comment 34

[Steven Johnson]

Attachment: Alternate, CodeContext-based approach

This is an alternate approach that I think will be more robust that Lars patch; it's based on his in many ways, but the bug-compatibility flags live in a structure that is derived from the CodeContext, giving us a way to maintain proper behavior for multiple SWF's loaded with different bug-compatibility settings.

This is not ready for a full review; I need to do more scrubbing, and test integration into Flash, to see how it will play out in practice, but feedback on the general approach and long-term suitability is urgently desired, so we can decide to move forward with this design (or not).

Comment 35

[Steven Johnson]

Attachment: Testcase

Here's a testcase (for Bug 504525, Vector.concat) demonstrating the workings. It's a bit awkward and doesn't scale well -- it embeds compiled ABC into the testcase so that it can load it dynamically -- and also uses a somewhat hacky approach to versioning (the newly-added Domain.bugCompatibility flag) -- but does demonstrate that the concept works.

(Longer term, I may remove Domain.bugCompatibility in favor of looking at metadata in the abc file to decide "bug compatibility" -- opinions?)

Comment 36

[Dan Schaffer]

I ran with the patch and was able to use the -bugcompat modes FP_9_0 FP_10_0 and ReduxLatest and see the different order of Vector.concat.

ideas:  Can we add a getter for Domain. bugCompatibility to return the current compatibilty mode as set by -bugcompat?   QE was thinking the regression testcase could query the Domain.bugCompatibility flag in the testcase and assert based on the bugCompatiblity getter.  We could run the version testcases through all of the -bugcompat flags.  Also in deep test phase we could run the entire testsuite with each of the -bugcompat modes.

also for convenience please add -bugcompat to usage shell usage string and show the possible modes.

Comment 37

[Steven Johnson]

(In reply to comment #36)
> ideas:  Can we add a getter for Domain. bugCompatibility to return the current
> compatibilty mode as set by -bugcompat?   QE was thinking the regression
> testcase could query the Domain.bugCompatibility flag in the testcase and
> assert based on the bugCompatiblity getter. 

I actually deliberately omitted such a getter, mainly because I was (and am) worried that code might use it to test for bugfixes, rather than more specific bug flags, eg

    if (domain.version >= kFP9_0_0) ... // no! bad!
    if (bugzilla(bugnumber)) ... // yes! correct!

that said, sounds like QE has a good usecase for this approach, so perhaps I should put something like it in place. (If you can think of an alternate approach for your testing needs, though, that would be preferable IMHO...)

> also for convenience please add -bugcompat to usage shell usage string and show
> the possible modes.

Will do.

Comment 38

[Steven Johnson]

(In reply to comment #35)
> (Longer term, I may remove Domain.bugCompatibility in favor of looking at
> metadata in the abc file to decide "bug compatibility" -- opinions?)

Answering my own question, I think metadata is definitely a better approach; I'll offer a revised patch that takes that approach.

On a different topic, one concern I have is about subclassing CodeContext; Flash/AIR have (always) used a subclass of CodeContext ("PlayerCodeContext"), and while we have so far pretty much gotten lucky with the fact that there is explicit up-casting going on, I think this is a recipe for possible pain down the road. It's probably not practical to remove the subclassing (Flash needs to embed some extra information that isn't going into Tamarin anytime soon), so I may add a factory method to AvmCore to ensure that all CodeContext's can be a predictable class...

Comment 39

[Steven Johnson]

(In reply to comment #37)
> (In reply to comment #36)
> > ideas:  Can we add a getter for Domain. bugCompatibility to return the current
> > compatibilty mode as set by -bugcompat?

How about if we make it System.bugcompat? That decouples it more clearly from Domain.

Comment 40

[Dan Schaffer]

(In reply to comment #39)
> How about if we make it System.bugcompat? That decouples it more clearly from
> Domain.

Sure, System.bugcompat would be great.

Comment 41

[Steven Johnson]

(In reply to comment #38)
> (In reply to comment #35)
> > (Longer term, I may remove Domain.bugCompatibility in favor of looking at
> > metadata in the abc file to decide "bug compatibility" -- opinions?)
> 
> Answering my own question, I think metadata is definitely a better approach;
> I'll offer a revised patch that takes that approach.

Answering my own question *again*, I think an even better (and simpler) approach would be to avoid messing with ABC metadata at all, and instead, use a SWF version (!) to do this, since avmshell can handle trivial ABC-only SWFs.

Comment 42

[Steven Johnson]

Attachment: Reviewable Patch

After more tweaking, and doing proof-of-concept integration with Flash, I think this patch is ready for a real review.

Notable differences from the previous patch:

-- changed the versions from "FP_#_#" to "SWF#", as this more clearly expresses the meaning and reality. Also explicitly added "SWF11" as the currently highest-known version ("Latest" merely aliases this).

-- bottlenecking, on the assumption that the embedder will want to subclass BugCompatibility

-- special-case handling for bugzilla 444630; SWF9 and SWF10 require it to be configurable on a per-AvmCore level, for backwards compatibility. (Ugh.)

-- punted on the idea of "bugcompat" metadata, as it proved to be too messy and invasive, and not at all useful in terms of how Flash/AIR will end up working; instead, we should jsut use the "-bugcompat" flag to set the expected compatibility. (For dynamically loaded abc, I added a new optional argument to Domain.load and Domain.loadBytes.)

-- added System.bugcompat, which returns the value that was given to -bugcompat on the commandline.

Comment 43

[Steven Johnson]

Attachment: Tests

Adds testcases:

-- a set for the fix for Vector.concat, to ensure that it's fixed (or not) for different -bugcompat

-- a set for the general BugCompatibility approach, verifying that a Loadee's bugcompatibility isn't affected by the Loader's. (This happens to use the Vector.concat fix for verification.)

Comment 44

[Steven Johnson]

Attachment: Reviewable Patch

After more tweaking, and doing proof-of-concept integration with Flash, I think this patch is ready for a real review.

Notable differences from the previous patch:

-- changed the versions from "FP_#_#" to "SWF#", as this more clearly expresses the meaning and reality. Also explicitly added "SWF11" as the currently highest-known version ("Latest" merely aliases this).

-- bottlenecking, on the assumption that the embedder will want to subclass BugCompatibility

-- special-case handling for bugzilla 444630; SWF9 and SWF10 require it to be configurable on a per-AvmCore level, for backwards compatibility. (Ugh.)

-- punted on the idea of "bugcompat" metadata, as it proved to be too messy and invasive, and not at all useful in terms of how Flash/AIR will end up working; instead, we should jsut use the "-bugcompat" flag to set the expected compatibility. (For dynamically loaded abc, I added a new optional argument to Domain.load and Domain.loadBytes.)

-- added System.bugcompat, which returns the value that was given to -bugcompat on the commandline.

Comment 45

[Lars T Hansen]

Comment on attachment 463017 [details] [diff] [review]
Reviewable Patch

This looks good to me, so r+ with the following nits to pick:

I know it's a detail but I object somewhat strongly to the use of 'bugcompat' as an abbreviation, especially on the AS3 System object, but generally (apart from the use as a flag name to avmshell).  False economy.

Not clear to me why you renamed shell_codeContext in ShellCore.h, so might warrant a comment.  Also, (nit) elsewhere you use the pattern user_whatEver, but in this case userCodeContext. 

The FIXME in ShellCore.cpp needs to be resolved somehow.

The doc block on the struct internal to BugCompatibility is almost certainly wrong, it references FP_SYS and talks about Object::bugzilla.

Indeed that doc block probably needs to discuss how CodeContext is used to check compatibility?

Comment 46

[Steven Johnson]

(In reply to comment #45)
> I know it's a detail but I object somewhat strongly to the use of 'bugcompat'
> as an abbreviation, especially on the AS3 System object, but generally (apart
> from the use as a flag name to avmshell).  False economy.

Fair enough. What would you prefer?
 
Other nits will be resolved appropriately prior to pushing (pending superreview, of course)

Comment 47

[Lars T Hansen]

(In reply to comment #46)
> (In reply to comment #45)
> > I know it's a detail but I object somewhat strongly to the use of 'bugcompat'
> > as an abbreviation, especially on the AS3 System object, but generally (apart
> > from the use as a flag name to avmshell).  False economy.
> 
> Fair enough. What would you prefer?

Elsewhere you've used 'bugCompatibility' which seems to be obvious enough.

Comment 48

[Stan Switzer]

> It's easier to reason about, but gives a totally unsatisfactory result when
> combining SWF of different versions; it means that (say) SWF10.2 could behave
> differently when loaded by SWF10.0 (vs. being loaded toplevel). I'm totally
> with Deneb here.

I've tried looking at the cases where this actually arises in the Player.  There aren't a lot of ways to get objects from other toplevels anyway; it is deliberately hard. The sandbox bridge effectively switches toplevels so the point is moot. There are shared events, a messy business for sure and not something we'd want to mess with. With ApplicationDomain.getDefinition(), neither approach seems more compelling to me but why mess with it?  But probably most typically are the DisplayList objects.  And those pretty much have to have behavior determined by the caller, not the object's instantiating toplevel.

My concern was/is that _builtin_ ABC code might get different results from the native functions it calls depending on who _it_ was called from... even in a single object instance, leading to possible comedy. But I think we can live with that if we are aware of it. The only really compelling case I can think of favors the CodeContext approach.

So I'm sold... not that I have to be. :) But I don't want to leave a sour note of dissent hanging out here.

The more we use builtin ActionScript to implement internal features ("eat your own dogfood") the more the question of how builtin ABC and CodeContexts work together will come up. We've muddled through with the "ignore builtins" model OK. Alternatives (least privilege, etc.) aren't clearly compelling at this point, but we might eventually need a more sophisticated model.

Comment 49

[Steven Johnson]

(In reply to comment #45)
> The FIXME in ShellCore.cpp needs to be resolved somehow.

That will probably need to be done as a followup patch, as it will require patching various existing acceptance tests. Not sure whether to open a new bug for it or continue with this one, but either way, I'd like to land this change, then do a followup patch that corrects the existing tests and changes the default.

Comment 50

[Steven Johnson]

Comment on attachment 463016 [details] [diff] [review]
Tests

pushed as http://hg.mozilla.org/tamarin-redux/rev/3ad427924afe

Comment 51

[Steven Johnson]

Comment on attachment 463017 [details] [diff] [review]
Reviewable Patch

pushed as http://hg.mozilla.org/tamarin-redux/rev/3ad427924afe

Comment 53

[Steven Johnson]

(In reply to comment #49)
> (In reply to comment #45)
> > The FIXME in ShellCore.cpp needs to be resolved somehow.
> 
> That will probably need to be done as a followup patch, as it will require
> patching various existing acceptance tests. 

Correcting myself: actually, existing acceptance tests are fine with a default bugcompat=kLatest; backtracking on my code a bit, I was confused by the weird status of the e4x fix (bugzilla444630), but all existing tests pass properly with existing patch and kLatest. Pushed as http://hg.mozilla.org/tamarin-redux/rev/6830b2b8fdc1 

With that, we're (in theory) ready to close this bug and move on the the dependent bugs, though it's probably worth looking at the dependent bugs dealing with testing infrastructure to decide if we need something rigorous, or if an ad-hoc, per-bug approach will do. I'll leave this

Comment 54

[Steven Johnson]

(oops)

Comment 55

[Steven Johnson]

With that, we're (in theory) ready to close this bug and move on the the
dependent bugs, though it's probably worth looking at the dependent bugs
dealing with testing infrastructure to decide if we need something rigorous, or
if an ad-hoc, per-bug approach will do. I'll leave this open for now and ask someone in QE (cpeyer?) to chime in with their thoughts.

Comment 56

[Dan Schaffer]

Attachment: testcase for versioning and vector.concat versioned bug

added a testcase to test -bugcompat flag and System.bugCompatibility property.
also added regression testcase for the Vector.concat bug to use for model of other version bugs.  works like:
- in test: if System.bugcompatiblity=='SWF9' or 'SWF10': assert old behavior 
           else assert new behavior
- create test.as.avm_args: containing '',SWF9,SWF10,SWF11 so every testcase in directory is run 4 times (once against each swf version and default)

Comment 57

[Dan Schaffer]

Attachment: testcase for versioning and vector.concat versioned bug

Comment 58

[Steven Johnson]

FYI: as it turns out, at present, SWF9 and SWF10 are equivalent as far as the VM is concerned (see the comments about the weird e4x behavior). I'm going to leave them as distinct for now, though, in case the Player needs them for a yet-to-be-determined reason; that said, for avmshell testing, we probably only need to test one of them.

Comment 59

[Brent Baker]

Comment on attachment 464111 [details] [diff] [review]
testcase for versioning and vector.concat versioned bug

- should put a comment in test/acceptance/as3/ShellClasses/SystemBugCompatDefault.as that this will fail and need to be updated when the default compat is rev'd

- TITLE comments in SystemBugCompat9|10 are incorrect, says SWF11 not SWF9|10

- I _was_ a little concerned that there is nothing really in place for bug_504525.as to ensure that the -bugcompat was actually set properly. We could be running with -bugcompat SWF9, but the shell could break and still use the default and the testcase would still pass since it would validate based on System.bugCompatibility. But looking at the other tests (SystemBugCompat*.as), I guess those are the tests that ensure that the bugcompat is working properly.

Comment 60

[Dan Schaffer]

(In reply to comment #59)
> Comment on attachment 464111 [details] [diff] [review]
> testcase for versioning and vector.concat versioned bug
> 
> - should put a comment in
> test/acceptance/as3/ShellClasses/SystemBugCompatDefault.as that this will fail
> and need to be updated when the default compat is rev'd
> 
> - TITLE comments in SystemBugCompat9|10 are incorrect, says SWF11 not SWF9|10
> 
> - I _was_ a little concerned that there is nothing really in place for
> bug_504525.as to ensure that the -bugcompat was actually set properly. We could
> be running with -bugcompat SWF9, but the shell could break and still use the
> default and the testcase would still pass since it would validate based on
> System.bugCompatibility. But looking at the other tests (SystemBugCompat*.as),
> I guess those are the tests that ensure that the bugcompat is working properly.

thanks for feedback.  updated patch to add comment in SystemBugCompatDefault.as and updated TITLE in SystemBugCompat9|10.

on last point yeah I think having testcases for -bugcompat separately should ensure the mechanism is working so each regress/versioning can assume the versions are correct.

also I made a few minor updates:
- for differential testing mark skips tied to bug 487841
- remove as3/Vector/bug504525 files, moving the testcases into regress/versioning and using the .as_avmargs files containing all -bugcompat= versions

Comment 61

[Steven Johnson]

Attachment: Move currentBugCompatibility() from Toplevel to AvmCore

A minor tweak based on further usage. currentBugCompatibility() is better suited to live on AvmCore rather than Toplevel: it normally depends on codeContext() (which only requires AvmCore); if there is no CodeContext, it returns the BugCompatibility of the builtins. But, it's already defined elsewhere that the builtins will always use kLatest, so there's no need to access Toplevel's AbcEnv and friends to extract this; we can simply cache it in AvmCore. This makes fixes like Bug 585791 simpler.

Comment 62

[Chris Peyer]

Comment on attachment 464111 [details] [diff] [review]
testcase for versioning and vector.concat versioned bug

Looks like the patch was not updated with changes from Comment 60.

I also think that instead of having individual .avm_args files in regress/versioning/ we could just have a dir.avm_args file so that all tests in that folder automatically are tested against all versions.  Will also make adding future versions to test easier.

How about tests for Domain.load and Domain.loadBytes bugcompat?

Should there be a negative test for -bugcompat: e.g. passing in -bugcompat SWF200? (I would assume that in that case bugcompat reverts to the default value)

Comment 63

[Steven Johnson]

(In reply to comment #62)
> Should there be a negative test for -bugcompat: e.g. passing in -bugcompat
> SWF200? (I would assume that in that case bugcompat reverts to the default
> value)

It fails with "invalid argument" and shows usage, doesn't fall back to a default

Comment 64

[Steven Johnson]

Comment on attachment 464523 [details] [diff] [review]
Move currentBugCompatibility() from Toplevel to AvmCore

pushed to TR as 5014:f3d3bded5462

Comment 65

[Steven Johnson]

This has also landed in Flash/AIR now -- I think we can close this bug as fixed and move on to dependencies.