1323782 - Check for OBJECT_FLAG_ITERATED in Array.prototype.unshift

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Test case:
---
function f(array, method, args) {
  var called = false;
  var keys = [];
  for (var key in array) {
    keys.push(key);
    if (!called) {
      called = true;
      Reflect.apply(method, array, args);
    }
  }
  print(keys);
}

f([1, , 3], Array.prototype.unshift, [0]);
f([1, , 3], Array.prototype.splice, [0, 0, 0]);
---

Expected: Same output for unshift and splice
Actual: Different output (unshift prints "0,2", whereas splice prints "0")

Comment 1

[André Bargull [:anba]]

Attachment: bug1323782.patch

Applies on top of bug 1282104. 

I've used JS::Result to have a single shared function for splice(), shift(), and unshift() and to get rid of the manual recoverFromOutOfMemory() call in CanOptimizeForDenseStorage().

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8819344 [details] [diff] [review]
bug1323782.patch

Review of attachment 8819344 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: js/src/jsarray.cpp
@@ +2193,5 @@
>      ShiftMoveBoxedOrUnboxedDenseElementsFunctor functor(obj);
>      JS_ALWAYS_TRUE(CallBoxedOrUnboxedSpecialization(functor, obj) == DenseElementResult::Success);
>  }
>  
> +static inline JS::Result<bool, JS::OOM&>

Seems like the inline should go away, and we should just depend on the compiler, not give a hint it could easily discard.

@@ +2237,1 @@
>          return DenseElementResult::Incomplete;

So it looks like [].shift previously mishandled the case where the array being shifted onto was on the prototype chain of something being iterated.  Can we add a testcase for that, that fails with the current code but passes with this change in place?

@@ +2415,5 @@
>   * through its contiguous vector of elements without fear of getters, setters,
>   * etc. along the prototype chain, or of enumerators requiring notification of
>   * modifications.
>   */
> +static inline JS::Result<bool, JS::OOM&>

Probably could/should remove the inline here as well.

@@ +2431,5 @@
>      if (arr->is<ArrayObject>() && arr->as<ArrayObject>().denseElementsAreFrozen())
>          return false;
>  
> +    /* Also pick the slow path if the object is being iterated over. */
> +    bool inIteration;

Maybe s/inIteration/possiblyBeingIterated/g?  This isn't definitively saying the object's being iterated, only that it *might* be, so a different name that doesn't over-promise seems desirable.

Comment 3

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> @@ +2237,1 @@
> >          return DenseElementResult::Incomplete;
> 
> So it looks like [].shift previously mishandled the case where the array
> being shifted onto was on the prototype chain of something being iterated. 
> Can we add a testcase for that, that fails with the current code but passes
> with this change in place?

I tried to come up with a test case, but I'm not sure that's possible with the current suppress-deleted limitations. According to the comments in SuppressDeletedPropertyHelper (jsiter.cpp), deleted properties on the prototype chain are never updated. This is visible with the following test case:
---
var proto = [0, 1];
var arr = [0];
Object.setPrototypeOf(arr, proto);

for (var k in arr) {
    // Should always print "<index>, true", but prints "1, false" in SM.
    print(k, k in arr);
    if (k === "0") {
        proto.shift();
    }
}
---

Also: When I removed the isDelegate() check in the new MaybeInIteration function, I didn't get any jstests or jit-tests failures.

Comment 4

[André Bargull [:anba]]

Attachment: bug1323782.patch

Updated per review comments, carrying r+ from Waldo.

Comment 5

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8108f3ad4aace2f8454fae063768e8a6078cdd2

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c756f91d7c09
Check iterated-flag before proceeding to fast path in Array.prototype.unshift. r=Waldo

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c756f91d7c09