Having default value for cor_x and cor_y doesn't fix this, which leads to me believe this that isn't a hole check bug. But changing cor_x and cor_y to be var does fix the bug for me. Seems like an issue with one of the backends.

 Issue v8:5584  has been merged into this issue.

Does this reproduce on Chrome 54 (which is the current stable version)? I looked at the jsfiddle and wasn't able to reproduce (clicked at least 20 times).

If there are more specific repro steps that'd be helpful too.

My version: 53.0.2785.101 (64-bit)

Okay, with faster clicking I'm now able to reproduce on 54.

This is very strange. I don't know why you do't have error.
Version 53.0.2785.101 (64-bit), Ubuntu 16.04

Here's a --trace-opt --trace-turbo-inlining run from when the page breaks:

[marking 0xf02ca5572b9 <JS Function onclick (SharedFunctionInfo 0x2a0b4db67e71)> for optimized recompilation, reason: small function, ICs with typeinfo: 1/1 (100%), generic ICs: 0/1 (0%)]
[compiling method 0xf02ca5572b9 <JS Function onclick (SharedFunctionInfo 0x2a0b4db67e71)> using TurboFan]
Candidates for inlining (size=1):
  id:22, calls:40, size[source]:151, size[ast]:33 / moveUp
Inlining moveUp into onclick
Candidates for inlining (size=2):
  id:59, calls:40, size[source]:207, size[ast]:46 / updateCoordinate
  id:228, calls:40, size[source]:106, size[ast]:25 / drawCat
Inlining updateCoordinate into onclick
Candidates for inlining (size=1):
  id:228, calls:40, size[source]:106, size[ast]:25 / drawCat
Inlining drawCat into onclick
[optimizing 0xf02ca5572b9 <JS Function onclick (SharedFunctionInfo 0x2a0b4db67e71)> - took 4.080, 3.039, 0.626 ms]
[completed optimizing 0xf02ca5572b9 <JS Function onclick (SharedFunctionInfo 0x2a0b4db67e71)>]

so it's the inline event handler that's being optimized, inlining the other functions, and somehow that's resulting in the coordinates getting "stuck". From the console logs, it looks like updateCoordinate closes over the "correct" versions of cor_x and cor_y, while moveUp treats them as constants (they always keep whatever values they had when the function got optimized).

I've been trying to further reduce this case, but haven't yet succeeded. Repro instructions are to: click rapidly on a single button :(

Assigning to bmeurer@ for further TurboFan investigation.

cc epertoso because this might have to do with v8::ScriptCompiler::CompileFunctionInContext, which is the special thing about this "onclick" handler.

My hunch is that this definitely has something to do with CompileFunctionInContext, since that code in api.cc looks really fishy (creating "with" contexts by hand...my guess is that the context chain ends up wrong somehow, maybe by missing the ScriptContext in which the let-bound variables reside).

I have no idea how this CompileFunctionInContext and the with scope was supposed to work in the ScriptCompiler, but it doesn't seem to play well with the assignment analysis. I added tracing to AstGraphBuilder::BuildVariableLoad() to print what the assignment analysis thinks is immutable, and it yields:

=========================================================================================================
[compiling method 0x2ed06d4d29a1 <JS Function onclick (SharedFunctionInfo 0xdbe75dd8949)> using TurboFan]
Candidates for inlining (size=1):
  id:22, calls:131, size[source]:152, size[ast]:33 / moveDown
Inlining moveDown into onclick
*** IMMUTABLE: 0xb01587f20a1 <String[16]: updateCoordinate>
*** IMMUTABLE: 0xb0158789f91 <String[5]: cor_x>
*** IMMUTABLE: 0xb0158789fb9 <String[5]: cor_y>
*** IMMUTABLE: 0xb0158789901 <String[3]: ctx>
*** IMMUTABLE: 0xb015877fe99 <String[7]: surface>
*** IMMUTABLE: 0xb015877fe99 <String[7]: surface>
*** IMMUTABLE: 0xb01587f0be9 <String[7]: drawCat>
*** IMMUTABLE: 0xb0158789f91 <String[5]: cor_x>
*** IMMUTABLE: 0xb0158789fb9 <String[5]: cor_y>
Candidates for inlining (size=2):
  id:62, calls:131, size[source]:207, size[ast]:46 / updateCoordinate
  id:246, calls:131, size[source]:106, size[ast]:25 / drawCat
Inlining updateCoordinate into onclick
Candidates for inlining (size=1):
  id:246, calls:131, size[source]:106, size[ast]:25 / drawCat
Inlining drawCat into onclick
*** IMMUTABLE: 0xb0158789901 <String[3]: ctx>
*** IMMUTABLE: 0xb0158789901 <String[3]: ctx>
*** IMMUTABLE: 0xb0158789901 <String[3]: ctx>
*** IMMUTABLE: 0xb0158789901 <String[3]: ctx>
=========================================================================================================

So basically inside the inlined moveDown the analysis tells TurboFan that cor_x and cor_y are immutable, thus TurboFan properly constant-folds that later (which works because we have a concrete context for the updateCoordinate inlinee here).

The fact that we even consider results from the ALAA for script scope seems kinda wrong to me.
But the ScriptCompiler also definitely looks fishy, and was probably never tested/thought through with an optimizing compiler so far as Crankshaft always bailed out for LookupSlots (which is what you with "with").

Of course it's not the ALAA, but the scope resolution that somehow marks these variables as immutable.

Can you check whether scope analysis is sane? --print-scopes

CompileFunctionInContext("return a", 1, "a", 1, "x') is supposed to return the compiled function

with (x) { function (a) { return a; } }

I suspect that the issue is in the scope analysis. We probably do not expect a with scope outside of a script scope (created for the let-variable), and mess up later on with its nature of dynamic lookup.

This has nothing to do with {CompileFunctionInContext} it's a plain old bug in the mutability analysis done during variable resolution. The following is a stand-alone repro. I have also attached the necessary --print-scopes dump that shows how {x} is not marked as "maybe assigned" when the function doing the mutation are lazily parsed.

===== Repro:

// Copyright 2016 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// Flags: --allow-natives-syntax --min-preparse-length=10

let x;
function f(a) {
  x += a;
}
function g(a) {
  f(a); return x;
}
function h(a) {
  x = a; return x;
}

function boom() { return g(1) }

assertEquals(1, h(1));
assertEquals(2, boom());
assertEquals(3, boom());
%OptimizeFunctionOnNextCall(boom);
assertEquals(4, boom());

===== Dump of --print-scopes:

global { // (0, 5268)
  // will be compiled
  // 1 stack slots
  // 5 heap slots
  // local vars:
  VAR g;  // 
  VAR f;  // 
  VAR h;  // 
  VAR boom;  // 
  LET x;  // context[4]
  // dynamic vars:
  DYNAMIC_GLOBAL assertEquals;  // 

  function boom () { // (396, 414)
    // lazily parsed
    // 4 heap slots
  }

  function h () { // (355, 381)
    // lazily parsed
    // 4 heap slots
  }

  function g () { // (319, 344)
    // lazily parsed
    // 4 heap slots
  }

  function f () { // (291, 308)
    // lazily parsed
    // 4 heap slots
  }
}

Also, while we are at it, the "maybe assigned" flag is also borked when it comes to destructuring. Kudos go to Toon for hinting at this.

(function() {
  var x = 23;
  function f() { return x; }
  function g() { [x] = [x + 1]; }
  function h() { g(); return x; }
  function boom() { return h() }
  assertEquals(24, boom());
  assertEquals(25, boom());
  assertEquals(26, boom());
  %OptimizeFunctionOnNextCall(boom);
  assertEquals(27, boom());
})()

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

commit b02e7fb86ec3831bf688b5550b1e97b8b7891df1
Author: mstarzinger <mstarzinger@chromium.org>
Date: Thu Nov 03 10:23:37 2016

[turbofan] Disable usage of {maybe_assigned} variable flag.

This disables the usage of the {maybe_assigned} flag that the variable
resolution computes for each variable on non-asm.js code. Note that the
analysis is fundamentally broken for destructuring and top-level lexical
variables. Also note that this still uses the analysis for asm.js code
even though it is not validated. One can still trigger the bug by using
invalid constructs within a function marked with "use asm". The fix is
intentionally minimal so that it can be merged to release branches.

R=bmeurer@chromium.org
TEST=mjsunit/regress/regress-crbug-659915
BUG= chromium:659915 

Review-Url: https://codereview.chromium.org/2471523005
Cr-Commit-Position: refs/heads/master@{#40716}

[modify] https://crrev.com/b02e7fb86ec3831bf688b5550b1e97b8b7891df1/src/compiler/ast-graph-builder.cc
[add] https://crrev.com/b02e7fb86ec3831bf688b5550b1e97b8b7891df1/test/mjsunit/regress/regress-crbug-659915a.js
[add] https://crrev.com/b02e7fb86ec3831bf688b5550b1e97b8b7891df1/test/mjsunit/regress/regress-crbug-659915b.js