During the course of fixing bug 599357, the following patch landed in TR: https://bugzilla.mozilla.org/attachment.cgi?id=479524 It was subsequently discovered that this patch broke compatibility with prior versions, specifically with respect to the exceptions raised for invalid vector indices. A new patch, operating in a quite different fashion, was then landed in TR: https://bugzilla.mozilla.org/attachment.cgi?id=489206 Unfortunately, the final VM import into the Spicy branch was done between these two patches, thus injecting an incompatibility. Subsequent work on bug 456852 in the Spicy branch corrected and rationalized the the handling of invalid vector indices for SWF11, preserving the prior behavior for SWF10 and earlier. Unfortunately, the prior behavior included the bug injected into Spicy by the first patch above, not the expected SWF10 behavior as defined by Argo.

This patch is based on the final patch for TR, https://bugzilla.mozilla.org attachment.cgi?id=489206, updated with the SWF11 changes (fix for bug 456852), which have not yet landed in TR. I stripped out the fused add/index optimization, retaining just the specialization for double-valued indices. Unfortunately, the optimized path depends on nanojit support that is missing on some platforms, including ARM. I introduced specializations of getprop_index and friends for double-valued indices to remove an extra double->atom conversion from the inlined code, as well as to allow an out-of-line fastpath.

The patch above is not as tight as the original Spicy code when the inlined fastpath is not applicable. Although {getprop,setprop,initprop}_index_d is specialized for the argument type, there is no specialization for Vectors with specialized element types. For any but Vector.<*>, the result of a getproperty must be converted to an atom. For Vector.<Number>, this requires an allocation. The existing Spicy code, in contrast, specializes on the declared element type of the Vector, calling a _getNativeXXXProperty() function that returns a value of precisely that type. The patch above achieves compatibility by peeling off the non-erroneous case, and delegates to the existing code patch (which ultimately winds through getAtomProperty()) to handle the exceptional cases. The present patch attempts to emulate the behavior that would result from such delegation without actually doing it. It almost succeeds. The catch is that getAtomProperty() erroneously searches the prototype chain for negative indices. If we have specialized to _getNativeDoubleProperty(), we expect the result to be of the declared element type of the Vector, which cannot be guaranteed. Instead of forgoing the specialization, we throw RangeError in this case, as we do in SWF11. Note that _getNativeIntProperty has always had this behavior -- it is clearly a bug that when _getNativeDoubleProperty was introduced, it didn't follow suit. This patch fails to be compatible only in a case where we may violate the basic principle that a property of a Vector.<T> instance named by a number should always be of type T. Specifically, this situation arises when a negative number is used as an index, and an object on the prototype chain has a like-named property. I'm inclined to suggest we should just implement this fix, enforcing the principle above. Maintaining compatibility with issues that manifest differently depending on which specializations are performed is likely to be fragile, as the pervasive presumption is that (correct) specializations are semantics-preserving and may be enabled/disabled without consequence to correctness.

(In reply to comment #3) > Created attachment 508961 [details] [diff] [review] > Compatible (mostly) fix for the original scheme used in Spicy > Note that _getNativeIntProperty has always had this behavior -- it is > clearly a bug that when _getNativeDoubleProperty was introduced, it didn't > follow suit. Correction: _getNativeDoubleProperty() did indeed adopt the same behavior. The issue is that _getIntProperty() and _getNativeIntProperty() were allowed to behave differently than the unspecialized getAtomProperty() method, and that _getDoubleProperty() and _getNativeDoubleProperty() were added without noting their divergence from the previous unspecialized path.

The backport from TR fails on this test case: Vector.<int>.prototype[-3]="quux"; var v_i:Vector.<int> = new Vector.<int>(); var n: Number = -3; print(v_i[n]); when invoked as follows: wmaddox-macpro:~ wmaddox$ tr-spicy/deb32/shell/avmshell -swfversion 10 -Ojit badVectorProp.abc Assertion failed: "((((avmplus::Atom)((uintptr_t(atom) & 7)))==kDoubleType))" ("/Users/wmaddox/tr-spicy/core/AvmCore-inlines.h":502) Trace/BPT trap This is not an issue with the backport or the modifications, as it manifests in TR as well. Argo: prints "quux" Spicy: throws ReferenceError Spicier: throws ReferenceError (-swfversion 10) TR: asserts It is being assumed that the result of getprop_index agrees with the declared index type of the Vector, though represented as an Atom. Searching the prototype chain, as done in Argo (and, to date, in TR) breaks this assumption. The Spicy/Spicier results reflect the incompatibility with Argo that is the subject of this bug.

The verifier contains logic that infers an int, uint, or Number for the type of an element of an appropriately-typed Vector where the index is known to be numeric. Given the type system implemented in the verifier, the prototype chain search on negative indices (admittedly a bug in the first place) is simply unsound.

Curious that Argo did not assert, I noticed that the verifier in Argo does *not* infer that the type of a numerically-named property is that of the Vector element type when the index type is Number, only when it is int or uint. This changed in Spicy and TR. Possible resolutions: 1) Keep the verifier change, and do not search the prototype chain for numeric properties. 2) Keep the verifier change, and search the prototype chain only in those cases where the verifier has inferred '*' as the type of the property. 3) Back out the verifier change, and remove the specialization on the vector element type from the code paths and helpers specialized on the Number index type. 4) Version both the verifier change and the specializations, so that the treatment of SWF10 remains identical to Argo. Note that Spicy/Spicier has already made hash of this, and has moved us somewhat unintentionally in the direction of 2). Keep in mind that there is never any question of searching the prototype chain except in the case of negative numeric indices. For all other numeric indices, we do not search, and for non-numeric cases we *do* search, and this has been consistent behavior across versions. The dubious handling of negative indices at issue here is clearly a bug, and obsolete as of SWF11.

If we want to maintain backward compatibility for this edge case, we'll need to drop the " || indexType == NUMBER_TYPE" addition in Verifier::emitGetProperty. Then we will always return an atom from this getProp call and if assigning to an integer, we should inline an integer fastpath check? (Same for setprop, etc.)

Here, it is no longer dependent upon the specializations performed whether the prototype chain is searched for negative indices.

The horse has left the barn with the Spicy release, thus the patch proposed in attachment 508961 [details] [diff] [review] is strictly more compatible with Argo than what we will soon be shipping. Leaving the additional inference in place in the verifier positions us much better to deliver the performance wins that Vector (vs. Array) was intended to provide. On the face of it, it seems quite unlikely that real-world code would depend on the behavior in question. We should be prepared to back this out, however, if experience with Spicy proves otherwise.

Comment on attachment 508961 [details] [diff] [review] Compatible (mostly) fix for the original scheme used in Spicy Sorry but I'm not clear on this. Is this code targeted for tamarin-redux? If not and/or its already landed are you looking for a post-landing review?

(In reply to comment #11) > Comment on attachment 508961 [details] [diff] [review] > Compatible (mostly) fix for the original scheme used in Spicy > > Sorry but I'm not clear on this. Is this code targeted for tamarin-redux? If > not and/or its already landed are you looking for a post-landing review? This is specifically a Spicy fix. TR needs some work as well, but much of the Vector code was rewritten following the Spicy branch, so this patch is not applicable to TR.

(In reply to comment #12) > (In reply to comment #11) > > Comment on attachment 508961 [details] [diff] [review] > > Compatible (mostly) fix for the original scheme used in Spicy > This is specifically a Spicy fix. ^^^ e.g, now Wasabi

Comment on attachment 508961 [details] [diff] [review] Compatible (mostly) fix for the original scheme used in Spicy nothing is obviously wrong, but I figured I'd get your attention with a feedback- just to see if we could summarise the situation lucidly, or have a discussion, or both. What i've surmised from skimming the comments is that argo had non-spec-compliant behavior, and an optimization in spicy broke the behavior, but not the way we want it; This patch clearly versions something, i just can't tell what, exactly. my $.02, in this case, is that if we had a 1-release bug (an injection that shipped, but we're quickly patching it), we should only support the pre-injection and post-all-fixed-up behavior; i.e. just fix the bug. Also, have we missed a chance to optimize out a version check? if we're doing a version check in a helper called from JIT code, the only time the check really needs to happen at runtime is when the JIT code is builtin code -- user code getting jit compiled has a version known at JIT time. if its too hard to summarise, lets just discuss.

In connection with bug 456852, attachment 502988 [details] [diff] [review] landed in Spicy. The patch rationalized vector index exception behavior in SWF11 in order to align interpreter and JIT behavior. The patch was designed to leave the behavior of the code unchanged for SWF10 and earlier. Unfortunately, attachment 479524 [details] [diff] [review] had previously landed before branching for Spicy. This patch was buggy, and superseded by attachment 489206 [details] [diff] [review], which landed in TR, but *not* in Spicy. With 479524, Spicy became incompatible with Argo for SWF10 and earlier. Thus, "leaving the behavior of the code unchanged" meant the new buggy behavior injected by 479524, not Argo-compatible behavior. Attachment 508961 [details] [diff] is a patch against tr-spicy as it stands now, and thus retains the versioned behavior of attachment 502988 [details] [diff] [review]. What it does is revert the effect of attachment 479524 [details] [diff] [review], so that pre-SWF11 behavior matches Argo, not Spicy as of the branch from TR. >my $.02, in this case, is that if we had a 1-release bug (an injection that >shipped, but we're quickly patching it), we should only support the >re-injection and post-all-fixed-up behavior; i.e. just fix the bug. I'm only versioning the SWF11+ behavior, reflecting deliberate changes made to align the interpeter and JIT in the Spicy release. The buggy SWF10 behavior injected in Spicy is reverted without versioning. >Also, have we missed a chance to optimize out a version check? if we're doing >a version check in a helper called from JIT code, the only time the check >really needs to happen at runtime is when the JIT code is builtin code -- user >code getting jit compiled has a version known at JIT time. The version check is confined to the exceptional cases, except for a version check in hasProperty() for numerically-named properties, which was introduced in attachment 502988 [details] [diff] [review], previously approved and landed. To allow for a test at runtime in the builtin case, we'd have to provide for two different versions of the helpers. We may have to do this someday when something performance-critical needs to be versioned, but I'm not eager to go there now. For one thing, we are assuming that we know the correct version for non-builtin code at JIT time. While this *should* be true, based on Steven's claims for how versioning is supposed to work, the API does not enforce this, and allows the host (player) to violate it. I'd really like to sort this out (in the context of the anticipated future deployment of "OSR everywhere") before going too far down the patch of JIT-time versioning.

(In reply to comment #15) To again clarify the most important issue here for review purposes: I proposed two different fixes for the SWF10 regression in Spicy. The first is based on Werner's second patch, as landed in TR (comment 2). That patch is flawed, however (comment 5), and, while fixable, I'm proposing another approach (comment 3) as the way forward for both Wasabi and TR. It sacrifices a sliver of compatibility in an obscure corner case in order to deliver fully on the typing guarantee Vector was intended to provide. Comments 3, 6, 7, and 10 further explore this. Part of the argument in favor of this is that we've already shipped the proposed new behavior in Spicy (as well as some that I propose to revert), and if anyone is by some chance depending on this corner case, we are going to find out soon (comment 10). finesses the

Comment on attachment 508961 [details] [diff] [review] Compatible (mostly) fix for the original scheme used in Spicy I have not validated backwards compatibility as your tests appear to confirm all the cases. r+ with nit; - outside the scope of this bug but the code would benefit from some consolidation of common idioms into helpers functions (but as you pointed out off line performance might then become a factor). e.g. _setNativeDoubleProperty and _setDoubleProperty have identical code for most of the method. Likewise, with setDoubleProperty and getDoubleProperty.

Pushed to tr-spicy: http://hg.mozilla.org/users/stejohns_adobe.com/tr-spicy/rev/ae129cb9c75b The Vector code in TR is cleaner and heavily templatized, and I expect that the patch that lands there will be tighter than this one.

Bill,when will this land in TR?

This is a Spicy-only patch, so it will not land in TR. There is an issue in TR noted in comment 5, however, that will be dealt with similarly in the course of addressing bug 456852. This has been held up in review, as the approach taken there should follow what we're doing in Wasabi, which has only now been resolved. (The TR bug noted in comment 5 was introduced by code that attempted to maintain compatibility in an obscure case for which we have now punted in Wasabi in favor of uniformity and performance.) I intend to close this bug as soon as the code actually lands in Perforce. BuildForge has been nothing but trouble, and I have been unable to get a clean sandbox build to run to completion since yesterday afternoon.

re comment #20, looks like this landed in p4 CL 864993. Can resolve->fixed now?

(In reply to comment #22) > re comment #20, looks like this landed in p4 CL 864993. Can resolve->fixed > now? Closing.

marking as verified. will not go into tr but went into spicy branch (wasabi) to preserve compatibility.

I am testing the regression testcase in the sandbox and will submit after verifying it passes everywhere. Also I am reducing the testcases. We do not need all 1270 testcases for this bug. I will reduce to a smaller number.

(In reply to comment #25) > I am testing the regression testcase in the sandbox and will submit after > verifying it passes everywhere. Also I am reducing the testcases. We do not > need all 1270 testcases for this bug. I will reduce to a smaller number. Note that an updated version of the test results, including a golden file for TR, is attached to bug 456852 , https://bugzilla.mozilla.org/attachment.cgi?id=524442 I think it may be a good idea to include this test as one that just compares the output with the golden files, similar to differential tests. All of the cases are important for complete coverage, even if they did not all regress for bug 630717. Note also test as3/Vector/vectorIndexExceptions.as. This is a normal acceptance test, using the AddTestCase method in the harness, however, it checks only for correct behavior in the current SWF version. Unfortunately, the legacy behavior (SWF10) is not as regular, so the strategy I use to generate the test cases programmatically along with the expected result does not apply.