Last Comment Bug 513095 - (TR_MAX) Vector.filter throws a type error
(TR_MAX)
: Vector.filter throws a type error
Status: VERIFIED FIXED
:
Product: Tamarin
Classification: Components
Component: Virtual Machine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: flash10.1
Assigned To: Tom Harwood
:
Mentors:
: 507501 509502 (view as bug list)
Depends on:
Blocks: 507501
  Show dependency treegraph
 
Reported: 2009-08-27 14:49 PDT by Christopher A. Brichford
Modified: 2010-06-25 05:31 PDT (History)
8 users (show)
cpeyer: in‑testsuite+
dansmith: flashplayer‑qrb+
trbaker: flashplayer‑needsversioning+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Native impl of Vector._map and Vector._filter returns Atom (4.44 KB, patch)
2009-09-21 08:29 PDT, Tom Harwood
lhansen: review+
edwsmith: superreview+
Details | Diff | Review
Re-doing original patch (1.67 KB, patch)
2010-01-13 14:47 PST, Tom Harwood
stejohns: review+
edwsmith: superreview+
Details | Diff | Review
Re-enable test coverage for Vector.filter,map (1.32 KB, patch)
2010-01-13 14:48 PST, Tom Harwood
cpeyer: review+
Details | Diff | Review

Description Christopher A. Brichford 2009-08-27 14:49:59 PDT
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 2009-08-27 14:51:53 PDT
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 2009-08-28 04:39:12 PDT
Looks similar to #507501, which has Vector.map() not returning the proper type
Comment 3 Tom Harwood 2009-09-18 07:44:13 PDT
(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 2009-09-21 08:29:29 PDT
Created attachment 401858 [details] [diff] [review]
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 2009-09-28 08:58:19 PDT
Comment on attachment 401858 [details] [diff] [review]
Native impl of Vector._map and Vector._filter returns Atom

pushed changeset 2612
Comment 6 Tom Harwood 2009-09-28 09:00:01 PDT
*** Bug 507501 has been marked as a duplicate of this bug. ***
Comment 7 Tom Harwood 2009-09-28 09:05:40 PDT
*** Bug 509502 has been marked as a duplicate of this bug. ***
Comment 8 Chris Peyer 2009-10-01 10:50:15 PDT
Need to add testcases to acceptance suite.
Comment 9 Chris Peyer 2009-10-06 18:11:36 PDT
Media added: http://hg.mozilla.org/tamarin-redux/rev/2705
Comment 10 Tom Harwood 2009-12-11 12:59:12 PST
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 2009-12-16 10:29:21 PST
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 2009-12-16 11:03:12 PST
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 2009-12-16 11:08:01 PST
(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 2009-12-16 11:40:17 PST
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 2009-12-16 12:23:46 PST
(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 2010-01-13 12:59:45 PST
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 2010-01-13 14:47:27 PST
Created attachment 421520 [details] [diff] [review]
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 2010-01-13 14:48:23 PST
Created attachment 421521 [details] [diff] [review]
Re-enable test coverage for Vector.filter,map
Comment 19 Steven Johnson 2010-01-13 15:27:27 PST
Comment on attachment 421520 [details] [diff] [review]
Re-doing original patch

be sure to rebuild the builtins when you push.
Comment 20 Tom Harwood 2010-01-13 16:40:51 PST
(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 2010-01-14 07:05:49 PST
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 2010-01-14 07:50:48 PST
(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 2010-01-14 08:01:36 PST
Fixed at last.
Comment 24 Chris Peyer 2010-02-22 14:43:43 PST
Verified fixed redux r3871
Comment 25 cmstreg 2010-06-25 03:53:34 PDT
In which flash player version / flex SDK is this issue solved?
Comment 26 Lars T Hansen 2010-06-25 05:31:08 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.