590879 - avmshell-exposed bugCompatibility getter is not rational

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

Currently we expose System.bugCompatibility: String as a property that allows test cases to query about the behavior they should expect from versioned built-in methods.

As currently exposed, the mechanism is going to become cumbersome:  The string is of unknown format (the format is not documented in the System class and the command line help for the shell only lists all the legal version, making no effort to state that a particular format will always be used).  Ergo a test case, to be correct, /must/ test (using ==) against every version string for versions where the old behavior of an API should be expected.  This can get cumbersome quickly; already test cases are skipping the test for SWF10 because they "know" SWF9 and SWF10 are the same.  What we want is a "<" test: if the running version is "<" the version in which we know the change to occur, we expect old behavior.

Unfortunately it is not an improvement to document that the string is of the form "SWFn" because "SWF100" is "<" the earlier "SWF20".  At the current pace, with 90 unassigned numbers, four releases a year, and every release providing a new version, we'll reach SWF100 in less than 23 years - and that's assuming that we don't create holes in the range!

The workaround "System.bugCompatibility.substring(3) < 11" is pretty silly.

I propose that we rename System.bugCompatibility as System.swfVersion, that we make it return an integer, and that the -bugcompat switch to the shell is renamed as -swfversion and also takes an integer.

Comment 1

[Lars T Hansen]

(In reply to comment #0)
> Unfortunately it is not an improvement to document that the string is of the
> form "SWFn" because "SWF100" is "<" the earlier "SWF20".  At the current pace,
> with 90 unassigned numbers, four releases a year, and every release providing
> a new version, we'll reach SWF100 in less than 23 years - and that's assuming
> that we don't create holes in the range!

If I had worked harder on my logic and less hard on my rhetoric I would have realized that "SWF11" < "SWF9" and so the system is already broken.

Comment 2

[Steven Johnson]

All sounds fine to me -- I'd be happy to review such a patch :-)

Comment 3

[Lars T Hansen]

Yeah, I guess I asked for that one.

Comment 4

[Lars T Hansen]

This is messy because Domain.loadBytes also takes a string, I need to figure out if that's an old API or a new one we can change.

Comment 5

[Steven Johnson]

(In reply to comment #4)
> This is messy because Domain.loadBytes also takes a string, I need to figure
> out if that's an old API or a new one we can change.

That's a recent change, and this version of "Domain" is custom to avmshell -- change at will.

Comment 6

[Lars T Hansen]

Attachment: Patch: code changes

Comment 7

[Lars T Hansen]

Attachment: Patch: test changes

Comment 8

[Lars T Hansen]

(Note shell_toplevel.{abc,h,cpp} must be regenerated when the patch lands.)

Comment 9

[Steven Johnson]

Comment on attachment 470391 [details] [diff] [review]
Patch: code changes

DomainObject::loadBytes() should use int32_t, not int. (Better yet, uint32_t here and in AS3, since value can't be negative, but that's a quibble.)

Otherwise fine.

Comment 10

[Lars T Hansen]

I found another bug, which is that Domain.load() was still taking a bugCompatibility string, not a swfVersion.  Apparently it's not being tested anywhere.

Comment 11

[Lars T Hansen]

tamarin-redux changeset:   5138:eba4b681487c

Comment 12

[Steven Johnson]

(In reply to comment #10)
> I found another bug, which is that Domain.load() was still taking a
> bugCompatibility string, not a swfVersion.  Apparently it's not being tested
> anywhere.

In that case perhaps we should just remove it entirely, as it was only added for testing purposes.

Comment 13

[Lars T Hansen]

(In reply to comment #12)
> (In reply to comment #10)
> > I found another bug, which is that Domain.load() was still taking a
> > bugCompatibility string, not a swfVersion.  Apparently it's not being tested
> > anywhere.
> 
> In that case perhaps we should just remove it entirely, as it was only added
> for testing purposes.

I will be happy to review such a patch :-)

Comment 14

[Chris Peyer]

Attachment: Update runtests to use swfversion everywhere.

Patch to make use of swfversion nomenclature consistent everywhere.  Followup patch to fix test cases and incorrect .avm_arg files will be addressed in Bug 591885.

Comment 15

[James Sudduth]

Comment on attachment 470893 [details] [diff] [review]
Update runtests to use swfversion everywhere.

Looks good - I don't see any typos or other problems.

Comment 16

[Chris Peyer]

Pushed to redux:

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

Also updated Tamarin MDC docs:
https://developer.mozilla.org/En/Tamarin/Tamarin_Tests/Actionscript_Acceptance_Tests