Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/v8/v8/+/44bed6a85e0775a77954ac17c0e551b55e43b32b (TF stubs out of ArrayIndexOf and ArrayInclude builtins).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.

+Sigurd, Jaro, PTAL

 Issue 842143  has been merged into this issue.

I just returned from a sick leave and will be able to also have a look.

The optimised path fails to handle negative |fromIndex|:

var arr = [undefined];

function myFunc() {
  print(arr.indexOf(undefined, -1));
}

myFunc();
myFunc();
%OptimizeFunctionOnNextCall(myFunc);
myFunc();


Returns
0
0
-1


I'll have a look at a fix and a test.

The non-optimised code has this transformation:

   BIND(&is_smi);
    {
      Node* intptr_start_from = SmiUntag(start_from);
      index_var.Bind(intptr_start_from);

      GotoIf(IntPtrGreaterThanOrEqual(index_var.value(), intptr_zero), &done);
      // The fromIndex is negative: add it to the array's length.
      index_var.Bind(IntPtrAdd(array_length_untagged, index_var.value()));
      // Clamp negative results at zero.
      GotoIf(IntPtrGreaterThanOrEqual(index_var.value(), intptr_zero), &done);
      index_var.Bind(intptr_zero);
      Goto(&done);
    }
    BIND(&done);

The JSCallReducer::ReduceArrayIndexOfIncludes does not do that. The stubs expect fromIndex processed and non-negative and are correspondingly confused when they interpret it as an unsigned integer and realise it points after the end of the array.

I need to figure out how to write the transformation in the language of a graph reduction.

After speaking to sigurds@, my next step for here is to mimic the transformation cited in #6 with an if-node, an increment node and a NumberMax.

sigurds@ also spotted the assumption that the array length is an SMI, which does not seem to be checked. However, so far I tried calling Array.prototype.indexOf.call({1: undefined, length: 2.345}, undefined, 1) and it did not help to trigger any errors. sigurds@ is looking into this further while I am preparing the fix to the original issue.

(And just for my record, if we end up needing the check for having a JSArray, we can do what CanInlineArrayIteratingBuiltin does: receiver_map->prototype()->IsJSArray().

CL in progress https://chromium-review.googlesource.com/c/v8/v8/+/1059628

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/be5cfb2295c1449d9a5befefeacf1e48d3911687

commit be5cfb2295c1449d9a5befefeacf1e48d3911687
Author: Vaclav Brozek <vabr@chromium.org>
Date: Wed May 16 07:31:46 2018

Fix array.indexOf for negative fromIndex

Array.indexOf accepts an optional fromIndex argument. When non-negative,
this argument restricts the searched indices to those starting at
fromIndex:
[1, 2, 1].indexOf(1,1) == 2
When negative, it is meant to be added to the array length to provide
such initial index for the search:
[1, 2, 1].indexOf(1, -2) == 2

This transformation has been done by the non-optimised builtin but not
by the reducer. The CL adds this construction to the reducer.

Bug:  chromium:842612 
Change-Id: I0ff089997f4ebb4dc3c2923e52c382a8a96cd711
Reviewed-on: https://chromium-review.googlesource.com/1059628
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53197}
[modify] https://crrev.com/be5cfb2295c1449d9a5befefeacf1e48d3911687/src/compiler/js-call-reducer.cc
[add] https://crrev.com/be5cfb2295c1449d9a5befefeacf1e48d3911687/test/mjsunit/regress/regress-842612.js

r53197 should fix this issue.
If we find some problems concerning the missing check for JSArray, we should tackle that under a separate bug.

ClusterFuzz has detected this issue as fixed in range 53196:53197.

Detailed report: https://clusterfuzz.com/testcase?key=5315050475356160

Fuzzer: foozzie_js_mutation
Job Type: v8_foozzie
Platform Id: linux

Crash Type: V8 correctness failure
Crash Address: 
Crash State:
  configs: x64,ignition:x64,ignition_turbo
  sources: ed7
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=v8_foozzie&range=51728:51729
Fixed: https://clusterfuzz.com/revisions?job=v8_foozzie&range=53196:53197

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5315050475356160

See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 5315050475356160 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

@bmeurer: Could you take a look please and decide if we want to request a merge into M67?

This is a correctness issue that should be back-merged to M67.

This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

+hablich@ for M67 merge review.

Please merge today if possible, then it would be in the next beta release.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/d72c7366d76b5dec9cf785f0626e5276c132e313

commit d72c7366d76b5dec9cf785f0626e5276c132e313
Author: Sigurd Schneider <sigurds@chromium.org>
Date: Tue May 22 15:15:04 2018

Version 6.7.288.42 (cherry-pick)

Merged be5cfb2295c1449d9a5befefeacf1e48d3911687

Fix array.indexOf for negative fromIndex

TBR=bmeurer@chromium.org

Bug:  chromium:842612 
Change-Id: Ia8058ffbde1a8ad88ba21ad83f4baffdace7d15e
Reviewed-on: https://chromium-review.googlesource.com/1069130
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.7@{#81}
Cr-Branched-From: 8457e810efd34381448d51d93f50079cf1f6a812-refs/heads/6.7.288@{#2}
Cr-Branched-From: e921be5c4f2c6407936bde750992dedbf47c1016-refs/heads/master@{#52547}
[modify] https://crrev.com/d72c7366d76b5dec9cf785f0626e5276c132e313/include/v8-version.h
[modify] https://crrev.com/d72c7366d76b5dec9cf785f0626e5276c132e313/src/compiler/js-call-reducer.cc
[add] https://crrev.com/d72c7366d76b5dec9cf785f0626e5276c132e313/test/mjsunit/regress/regress-842612.js

Done!