shell needs a flag to set version to regression test bug fixes that have been versioned

NEW
Assigned to

Status

Tamarin
Virtual Machine
P1
normal
9 years ago
7 years ago

People

(Reporter: Dan Schaffer, Assigned: Dan Schaffer)

Tracking

unspecified
Q1 12 - Brannan
Dependency tree / graph
Bug Flags:
in-testsuite ?
flashplayer-injection -
flashplayer-qrb +
flashplayer-bug -
flashplayer-needsversioning +

Details

(Whiteboard: loose-end)

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
currently we have 1 versioned bug where the fix is for flash 10, so flash 9 still exhibits the older behavior.  A shell flag like -version=9 or -version=10 would set the bit mask in PoolObject.cpp to change the behavior.  With the fix we could add regression tests for the versioned bug for both the old behavior and the new behavior.

the versioned bug fix is: https://bugzilla.mozilla.org/show_bug.cgi?id=444630
Flags: flashplayer-qrb?

Updated

9 years ago
Assignee: nobody → jodyer
Status: NEW → ASSIGNED
Flags: flashplayer-qrb? → flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.1

Comment 1

9 years ago
if VMCFG_TEST_API_VERSIONING is defined, then 'avmshell -api N' treats user code as if it was version N, where N is one of the player version ids (660-670). Will that do the trick?
(Assignee)

Comment 2

9 years ago
Created attachment 407809 [details] [diff] [review]
enable -api parameter in shell 

patch to add -api parameter to shell.  when building shell do:
$ ../configure.py --enable-shell --enable-api-versioning
...
$ shell/avmshell --usage
...
[-api N] execute ABCs as version N (see api-versions.h)
Attachment #407809 - Flags: review?(cpeyer)

Updated

9 years ago
Attachment #407809 - Flags: review?(cpeyer) → review+

Comment 3

9 years ago
Also need a way to easily enable/disable the bugzilla bitfield bugs from the shell (preferable over having to compile the value in).

At the moment, then bit-flag for bug 444630 is set to 0 in the shell, with no easy way to turn it on / off.

From Jim Corbetts checkin notes:

Summary:Implemented the avmplus bug fix versioning. (Different than the API versioning)
This is so we can fix bugs in the avmplus without any references to the Flash Player (since it's open source and all) Basically, we set a bitField on the pool object (which Erik T. added) with contants that represent the bugzilla bugs that we want to fix. This is done when we're initializing the built-ins and any laoded files pools.
The code then checks the bitfield to decide if it wants to execute the old way or the fixed way.
** If you have an avmplus bug that needs version checking, add an entry to the enum in PoolObject.h (look for kbug) then add it to the bugFlags initialization in PlayerAvmCore::PlayerAvmCore and PlayerAvmCore::QueueAbcBuffer

Updated

9 years ago
Blocks: 506032

Comment 4

9 years ago
(In reply to comment #3)
> Also need a way to easily enable/disable the bugzilla bitfield bugs from the
> shell (preferable over having to compile the value in).
> 
> At the moment, then bit-flag for bug 444630 is set to 0 in the shell, with no
> easy way to turn it on / off.

Actually the current solution with bit flags in an int will collapse almost immediately once we start landing fixes.  It needs to be replaced immediately with standard C++ bit fields or a similar scalable mechanism.  (There is no reason that I can see that we should not be using standard C++ bit fields here.)

If there's any nervousness about placing these variables in the pool's top-level namespace - eg debuggers that insist on displaying them all, thus hiding too many other members - then put them in a struct.

Names of the bitfield could usefully name the bug tracking system, ie "bug444630" is better rendered as "bugzilla444630", if we anticipate landing fixes for other bug trackers.

Comment 5

9 years ago
Pushed --enable-api-versioning patch to redux - changeset: 2878:3d195cbcc865

Comment 6

9 years ago
More thoughts about versioning: it seems to me that -api N should set the flags for specific bugs in a way consistent with how version N sets flags in the player.  In particular, the player needs an API to set those flags in the VM as well and it will probably (should probably) use the same mechanism it uses to communicate the version of the swf.  There is no utility, I think, in setting flags in configurations other than those we would use in the player, because it may not make sense for individual bugs to be tested - the fix for one bug may rely on the fix for another, and that would be correct if the former bug is only "fixed" in the same or later versions as the latter.

Comment 7

9 years ago
(and I meant to add:)

That being so, is there any reason at all - other than custom, and /maybe/ documentation - why we should have flags for individual bugs and not for API versions in the C++ code?  The utility of hundreds of flags is sort of lost on me.

Comment 8

9 years ago
Comment on attachment 407809 [details] [diff] [review]
enable -api parameter in shell 

Shouldn't the avmfeatures.py only set AVMFEATURE_xxx? This patch adds a VMCFG_xxx option.

Comment 9

9 years ago
(In reply to comment #8)
> (From update of attachment 407809 [details] [diff] [review])
> Shouldn't the avmfeatures.py only set AVMFEATURE_xxx? This patch adds a
> VMCFG_xxx option.

Yeah, kind of strange.  Could still be valid if we don't think we should expose that feature, but that's a slippery slope we probably should not be going down without careful thinking.

Comment 10

9 years ago
(In reply to comment #7)
> (and I meant to add:)
> 
> That being so, is there any reason at all - other than custom, and /maybe/
> documentation - why we should have flags for individual bugs and not for API
> versions in the C++ code?  The utility of hundreds of flags is sort of lost on
> me.

+1, I think the original design of the "bugflags" feature didn't consider the possible size of changes. Switching to a "bug-compatible-version" set of flags (with a few possible exceptions for special cases) makes more sense IMHO.

Updated

9 years ago
Flags: flashplayer-needsversioning+

Updated

9 years ago
Target Milestone: flash10.1 → Future

Updated

9 years ago
Priority: P2 → --

Comment 11

9 years ago
Bug cleanup note:

This bug is only about providing a switch to the avmshell that makes the avmshell request a particular, named, coherent version of C++ and ABC code for testing.  For other aspects of the problem, go up to bug #535770 and look at sibling bugs of this bug.
Assignee: jodyer → nobody
Blocks: 535770
Summary: shell needs a flag to set version to regression test bug fixes have been versioned → shell needs a flag to set version to regression test bug fixes that have been versioned

Updated

9 years ago
Status: ASSIGNED → NEW

Updated

9 years ago
Blocks: 444630

Updated

8 years ago
Assignee: nobody → lhansen

Updated

8 years ago
Priority: -- → P3
Target Milestone: Future → flash10.2

Comment 12

8 years ago
A more-or-less complete solution is attached to bug #535770 but it's premature to land anything until the problem has been researched in more depth and the core solutions have been implemented.

Updated

8 years ago
Assignee: lhansen → nobody

Updated

8 years ago
Priority: P3 → P1

Comment 13

7 years ago
Is this really not yet resolved?
Flags: flashplayer-bug-
Whiteboard: must–fix-candidate

Updated

7 years ago
Whiteboard: must–fix-candidate → must-fix-candidate

Updated

7 years ago
Target Milestone: Q3 11 - Serrano → Q4 11 - Anza

Updated

7 years ago
Flags: in-testsuite?
Flags: flashplayer-injection-

Updated

7 years ago
Whiteboard: must-fix-candidate → has-patch

Comment 14

7 years ago
Dan, determine whether this bug can be closed or the action required to close it.
Assignee: nobody → dschaffe
Whiteboard: has-patch → loose-end

Updated

7 years ago
Target Milestone: Q4 11 - Anza → Q1 12 - Brannan
You need to log in before you can comment on or make changes to this bug.