513095 - (TR_MAX) Vector.filter throws a type error

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Christopher A. Brichford]

Vector.filter throws a type error when the following program is compiled with -DAS3:
var items:Vector.<String> = new Vector.<String>;
items.push("one");
items.push("two");
items.push("three");

var filtered:Vector.<String> = items.filter(function(item:String, index:int, source:Vector.<String>):Boolean
{
return item == "two";
});

Watson Bug #2400228

Comment 1

[Christopher A. Brichford]

This bug is probably due to the fact the _filter in VectorImpl.as is declared as return an ArrayObject.  The AS3 filter methods in Vector.as call _filter and attempt to coerce the result of _filter to Vector$object, Vector$uint, etc.  The C++ code in VectorClass.cpp seems to return a new Vector not an Array.  It is interesting that the acceptance tests did not catch this bug....

Comment 2

[Brent Baker]

Looks similar to #507501, which has Vector.map() not returning the proper type

Comment 3

[Tom Harwood]

(In reply to comment #1)
> _filter in VectorImpl.as ... return[s] ArrayObject.  
> The C++ code in VectorClass.cpp seems to return a new Vector not an Array.

The actual implementation of _filter (and _map) is hidden in VectorClass.h, and it does return an ArrayObject.  A straightforward delgation to VectorBaseObject::filter/map causes a few ripple effects, but something along these lines is clearly called for.

Comment 4

[Tom Harwood]

Attachment: Native impl of Vector._map and Vector._filter returns Atom

Looks like the original implementation piggybacked off Array helpers, which kinda worked, but there are better VectorBaseClass helpers now.  Moved AS3 Vector.map out of VectorImpl to match the treatment of AS3 Vector.filter.

Comment 5

[Tom Harwood]

Comment on attachment 401858 [details] [diff] [review]
Native impl of Vector._map and Vector._filter returns Atom

pushed changeset 2612

Comment 8

[Chris Peyer]

Need to add testcases to acceptance suite.

Comment 9

[Chris Peyer]

Media added: http://hg.mozilla.org/tamarin-redux/rev/2705

Comment 10

[Tom Harwood]

Reopened because the AS3 side of this change was backed out.  Need to determine whether or not it needs to be versioned.

Comment 11

[Christopher A. Brichford]

Looks like this bug is being deferred?  I think we have existing AIR apps that will break if this bug is not fixed.

Comment 12

[Lars T Hansen]

Bug requires a version check in order to be changed (according to the setting of the flag flashplayer‑needsversioning above).  But that won't be possible in Player 10.1 - there is no new version number to check against.

Comment 13

[Tom Harwood]

(In reply to comment #11)

As Lars noted, the change was backed out to preserve existing behavior until the API can be versioned.

Comment 14

[Trevor Baker]

Tom, when we last corresponded regarding this bug you thought you might have been mistaken that it required versioning...??  Have you concluded that it does indeed require versioning, and thus can not be fixed in 10.1?

Comment 15

[Tom Harwood]

(In reply to comment #14)
> Have you concluded that it does
> indeed require versioning, and thus can not be fixed in 10.1?

Yes, unfortunately.

The change does have an externally visible effect: the return type of Vector.<foo>.map() changes from Array to Vector.<foo>.  This will fail to compile if an existing AS3 program explicitly states that the return type is Array, e.g.,

var x:Array = fooVector.map(rumplestiltskin);

It can also change runtime behavior without a compile error, in so far as the behavior of Array differs from that of Vector.<foo>.

Comment 16

[Tom Harwood]

This was introduced by changelist 1041, "Patch to allow autogeneration of native-method maps from nativegen.py" on 10/31/2008 and so it's an injection and can be fixed after all.  Checking with Steven J. on why these routines were defined to return ArrayObject* in that changelist.

Comment 17

[Tom Harwood]

Attachment: Re-doing original patch

Re-did the original patch, except for some gratuitous re-arrangment of Vector::map that didn't seem worthwhile on a second look.

Comment 18

[Tom Harwood]

Attachment: Re-enable test coverage for Vector.filter,map

Comment 19

[Steven Johnson]

Comment on attachment 421520 [details] [diff] [review]
Re-doing original patch

be sure to rebuild the builtins when you push.

Comment 20

[Tom Harwood]

(In reply to comment #19)
> be sure to rebuild the builtins when you push.

Indeed.  I reverted 'em for grins and avmshell dutifully segfaulted.  I'll grab Edwin tomorrow AM and look into building direct thunks (which should be a compile-time error).  Last time I tried that I ran into dueling AVM_FEATURE settings.

Comment 21

[Edwin Smith]

Comment on attachment 421520 [details] [diff] [review]
Re-doing original patch

confirmed that the AS3/C++ signatures match, R+ conditionally on rebuilding builtins with and without direct thunks, and checking in the new builtins without direct thunks.

Comment 22

[Tom Harwood]

(In reply to comment #21)
> R+ conditionally on rebuilding
> builtins with and without direct thunks, and checking in the new builtins
> without direct thunks.

Built both ways, verified builtin.cpp is configured for indirect thunks on last check build:

#ifndef AVMPLUS_INDIRECT_NATIVE_THUNKS
  #error nativegen.py: --directthunks requires AVMFEATURE_INDIRECT_NATIVE_THUNKS=1
#endif

Comment 23

[Tom Harwood]

Fixed at last.

Comment 24

[Chris Peyer]

Verified fixed redux r3871

Comment 25

[cmstreg]

In which flash player version / flex SDK is this issue solved?

Comment 26

[Lars T Hansen]

It is not fixed in any released player as of now.  Player 10.1 did not introduce a new content version number with which to tag swfs that want to make use of incompatible bug fixes.  The fix will presumably be released in the next player version that does introduce such a version number.