Closed Bug 876114 Opened 12 years ago Closed 12 years ago

Regression: proxy on an array gives a bad index to set() when get() is not implemented

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Version:
21 Branch
Platform:
x86_64
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 895223
Tracking Flags:
Tracking Status
firefox21 --- affected
firefox22 - affected
firefox23 - affected
firefox24 - affected

People

(Reporter: laurent, Unassigned)

References

(
URL
)

Details

(Keywords: dev-doc-complete, regression)

The set() function of a proxy when the object is an array, receives a bad index when we use the push() function on the array Example (see also the URL field on the bug): <html><script type="text/javascript"> var p = new Proxy(["hello"], { set: function(arr, idx, v) { console.log("idx="+idx); arr[idx] = v; return true; }}) p.push("aa"); p.push("bb"); p.push("cc"); console.log(p); </script></html> Results on the console is: idx=0 idx=length idx=0 idx=length idx=0 idx=length ["cc"] Expected result: idx=1 idx=length idx=2 idx=length idx=3 idx=length ["hello", "aa", "bb", "cc"] The issue appears also if the initial array is empty. It worked perfectly in Firefox 20/XulRunner 20. The issue appears in Firefox 21/XulRunner 21 and is still there in the nightly (v24.0a1) The issue does not appear if we use explicit index: (p[1] = "cc") (It is a very annoying issue for my project SlimerJS :-/)
I discovered that the issue appears only if the get() function is not implemented in the proxy. So the get() function and set() function should be implemented both or not at all, to have a good behavior of push().
Summary: Regression: proxy on an array give bad index to set() → Regression: proxy on an array gives a bad index to set() when get() is not implemented
Last Good: a2d7ee172d6b\ First Bad: f4671ccc4502 Triggered by: f4671ccc4502 Brian Hackett — Bug 827490 - Allow native objects to have both slots and dense elements, rm dense/slow array distinction, r=billm, dvander.
Blocks: 827490
Keywords: regressionwindow-wanted
OS: Linux → All
Blocks: 703537
Brian do you think you can have a low risk forward fix in the next week for our fourth Beta? Alternately is backing out 827490 an option?
Assignee: general → bhackett1024
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox22: ? → +
tracking-firefox23: ? → +
tracking-firefox24: ? → +
Pretty sure this is a dupe of bug 827449, which is a preexisting issue in the proxy implementation that was exposed in more cases by bug 827490. Backing out bug 827490 isn't an option, and this bug would still be present even if it was.
Assignee: bhackett1024 → general
(In reply to Brian Hackett (:bhackett) from comment #5) > Pretty sure this is a dupe of bug 827449 Note that we basically decided to WONTFIX that bug because it only affected same-compartment wrappers, which were a gecko concept that we didn't care too much about. The solutions are more or less to implement some kind of nativeCall for PropertyOps, or to move the array stuff to JSNatives.
So, that bug was about PropertyOp only. This doesn't implicate PropertyOp/StrictPropertyOp at all, as far as I can tell. Nor does it implicate JSNative. Any chance you could very quickly explain this, for those without the time to jump into a debugger and see exactly what's up?
Oh, it's the push() method that's doing GetLengthProperty that's going through the wrapping stuff to array_length_getter, which triggers the PropertyOp fail. Silly me, should have seen that myself.
This bug broke my Web app. If an array is proxyified, * console.log(array) returns [] * array.length returns 0 * the set trap of the proxy is not get called A simple workaround was adding the get trap like this > get: function(obj, prop) { return obj[prop]; }, If this behavior is by design, please clarify it on the MDC doc: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy Thanks!
Brian, does this bug need to be closed as wontfix according to your comment #5?
Flags: needinfo?(bhackett1024)
Yes.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: needinfo?(bhackett1024)
Resolution: --- → WONTFIX
Reopening. This is a straight-up Proxy bug. Closing it is a mistake. I think it is a dup of a bug I've been working on, too.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Yep, it's a duplicate. We will fix this (though it's proving difficult).
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.
OSZAR »