Bug 513095 (TR_MAX)

Vector.filter throws a type error

VERIFIED FIXED in flash10.1

Status

Tamarin
Virtual Machine
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: Christopher A. Brichford, Assigned: Tom Harwood)

Tracking

unspecified
flash10.1
x86
Mac OS X
Bug Flags:
in-testsuite +
flashplayer-qrb +
flashplayer-needsversioning +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
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

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

Updated

8 years ago
Alias: TR_MAX
Assignee: nobody → tharwood
Flags: flashplayer-qrb+
(Assignee)

Comment 3

8 years ago
(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.
(Assignee)

Updated

8 years ago
Blocks: 507501
(Assignee)

Comment 4

8 years ago
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.
Attachment #401858 - Flags: superreview?(edwsmith)
Attachment #401858 - Flags: review?(lhansen)

Updated

8 years ago
Attachment #401858 - Flags: review?(lhansen) → review+

Updated

8 years ago
Attachment #401858 - Flags: superreview?(edwsmith) → superreview+
(Assignee)

Comment 5

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

pushed changeset 2612
Attachment #401858 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Duplicate of this bug: 507501
(Assignee)

Updated

8 years ago
Duplicate of this bug: 509502

Comment 8

8 years ago
Need to add testcases to acceptance suite.
Assignee: tharwood → cpeyer
Flags: in-testsuite?

Comment 9

8 years ago
Media added: http://hg.mozilla.org/tamarin-redux/rev/2705
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+

Updated

8 years ago
Flags: flashplayer-needsversioning+
(Assignee)

Comment 10

8 years ago
Reopened because the AS3 side of this change was backed out.  Need to determine whether or not it needs to be versioned.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

8 years ago
Assignee: cpeyer → tharwood
(Assignee)

Updated

8 years ago
Attachment #401858 - Attachment is obsolete: false

Updated

8 years ago
Priority: P5 → --
Target Milestone: flash10.2 → Future
(Reporter)

Comment 11

8 years ago
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

8 years ago
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.
(Assignee)

Comment 13

8 years ago
(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

8 years ago
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?
(Assignee)

Comment 15

8 years ago
(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>.
(Assignee)

Comment 16

7 years ago
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.
(Assignee)

Comment 17

7 years ago
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.
Attachment #401858 - Attachment is obsolete: true
Attachment #421520 - Flags: review?(stejohns)
(Assignee)

Comment 18

7 years ago
Created attachment 421521 [details] [diff] [review]
Re-enable test coverage for Vector.filter,map
Attachment #421521 - Flags: review?(cpeyer)

Comment 19

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

be sure to rebuild the builtins when you push.
Attachment #421520 - Flags: review?(stejohns) → review+

Updated

7 years ago
Attachment #421521 - Flags: review?(cpeyer) → review+
(Assignee)

Comment 20

7 years ago
(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.
(Assignee)

Updated

7 years ago
Attachment #421520 - Flags: superreview?(edwsmith)

Comment 21

7 years ago
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.
Attachment #421520 - Flags: superreview?(edwsmith) → superreview+
(Assignee)

Comment 22

7 years ago
(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
(Assignee)

Comment 23

7 years ago
Fixed at last.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Target Milestone: Future → flash10.1

Comment 24

7 years ago
Verified fixed redux r3871
Status: RESOLVED → VERIFIED

Comment 25

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

Comment 26

7 years ago
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.
You need to log in before you can comment on or make changes to this bug.