Node 6 crash w/ computed properties

[gotrevor]

Versions: 6.x
Platforms: OS X, Linux
Subsystem: unknown

Running this single crash.js file will cause Node 6, across several
different dot releases, to crash with what appears to be some strange,
ephemeral memory corruption:

https://github.com/gotrevor/LogOfUndefined/blob/master/crash.js

This example does not cause a crash in Node 4 or 8. You can see the
matrix of success/failure here:
https://circleci.com/gh/gotrevor/workflows/LogOfUndefined
and the most recent case here:
https://circleci.com/workflow-run/24e3c016-39b3-442f-a0b2-f045c51381b9

Someone knowledgeable about such things wrote:

I can reproduce and it looks an issue with
V8's tier 2 compiler because the problem goes away with the
--nocrankshaft flag. I'm 90% sure it's failing to propagate an inline
cache when generating optimized code.

[mscdex]

For posterity, here is crash.js:

#!/usr/bin/env node

'use strict';
/* eslint-disable no-useless-computed-key */

crashEntry();
process.exit(0);

function crashEntry() {
  weirdBootstrap();
  const entities = getEntities();

  for (let i = 0; i < 10; i += 1) {
    console.error('calling csvRows iteration:', i);
    csvRows(entities);
  }
}

function csvRows(entities) {
  for (let i = 0; i < 10; i += 1) {
    entityIntervalFn(entities[0], i);
    entityIntervalFn(entities[1], i);
  }
}

function entityIntervalFn(entity, i) {
  const iData = entity.intervalData[i];
  const val1 = iData ? iData.key1 : null;

  const result = {
    ['cruft']: 'foo',
    val1,
  };

  console.log({ i, iData });

  result.key3 = 1;
}

function getEntities() {
  return [{
    intervalData: [
      null,
      { key1: 1,
        key2: undefined },
    ]
  }, {
    intervalData: [
      null,
      { key1: 1,
        key2: undefined },
    ]
  }];
}

function weirdBootstrap() {
  return {
    key1: 0.7,
    key2: undefined,
  };
}

[TimothyGu]

And for posterity, stack trace with Node.js v6.11.1 (from https://circleci.com/gh/gotrevor/LogOfUndefined/14):

/root/Dashboard/crash.js:35
  console.log({ i, iData });
         ^

TypeError: Cannot read property 'log' of undefined
    at entityIntervalFn (/root/Dashboard/crash.js:35:10)
    at csvRows (/root/Dashboard/crash.js:21:5)
    at crashEntry (/root/Dashboard/crash.js:15:5)
    at Object.<anonymous> (/root/Dashboard/crash.js:6:1)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)

[MylesBorins]

So I just checked and this fails all the way back to Node v6.0.0, leaving me to believe that this is likely a problem with V8

/cc @nodejs/v8

[bnoordhuis]

With a debug build:

# Fatal error in ../deps/v8/src/deoptimizer.cc, line 351
# Check failed: safe_to_deopt_topmost_optimized_code.

Stack trace in gdb:

#0  v8::base::OS::Abort () at ../deps/v8/src/base/platform/platform-posix.cc:240
#1  0x00000000020fd0c5 in V8_Fatal (file=0x22e8cf1 "../deps/v8/src/deoptimizer.cc", line=351, format=0x22e7b7a "Check failed: %s.") at ../deps/v8/src/base/logging.cc:116
#2  0x00000000017e5346 in v8::internal::Deoptimizer::DeoptimizeMarkedCodeForContext (context=0xbd1f6f3e951) at ../deps/v8/src/deoptimizer.cc:351
#3  0x00000000017e580d in v8::internal::Deoptimizer::DeoptimizeMarkedCode (isolate=0x2edc350) at ../deps/v8/src/deoptimizer.cc:405
#4  0x0000000001a3f6e6 in v8::internal::DependentCode::DeoptimizeDependentCodeGroup (this=0x3e3ad49aa71, isolate=0x2edc350, group=v8::internal::DependentCode::kTransitionGroup) at ../deps/v8/src/objects.cc:15599
#5  0x0000000001a084cb in v8::internal::Map::DeprecateTransitionTree (this=0x818de342421) at ../deps/v8/src/objects.cc:3230
#6  0x0000000001a0848e in v8::internal::Map::DeprecateTransitionTree (this=0x818de341d99) at ../deps/v8/src/objects.cc:3226
#7  0x0000000001a0c020 in v8::internal::Map::Reconfigure (old_map=..., new_elements_kind=v8::internal::FAST_HOLEY_ELEMENTS, modify_index=1, new_kind=v8::internal::kData, new_attributes=v8::internal::NONE, new_representation=..., new_field_type=..., store_mode=v8::internal::FORCE_FIELD) at ../deps/v8/src/objects.cc:3958
#8  0x00000000019f2314 in v8::internal::Map::ReconfigureProperty (map=..., modify_index=1, new_kind=v8::internal::kData, new_attributes=v8::internal::NONE, new_representation=..., new_field_type=..., store_mode=v8::internal::FORCE_FIELD) at ../deps/v8/src/objects-inl.h:2826
#9  0x0000000001a275c4 in v8::internal::(anonymous namespace)::UpdateDescriptorForValue (map=..., descriptor=1, value=...) at ../deps/v8/src/objects.cc:9876
#10 0x0000000001a27840 in v8::internal::Map::TransitionToDataProperty (map=..., name=..., value=..., attributes=v8::internal::NONE, store_mode=v8::internal::Object::CERTAINLY_NOT_STORE_FROM_KEYED) at ../deps/v8/src/objects.cc:9911
#11 0x00000000019d5fe6 in v8::internal::LookupIterator::PrepareTransitionToDataProperty (this=0x7fffffffc8a0, receiver=..., value=..., attributes=v8::internal::NONE, store_mode=v8::internal::Object::CERTAINLY_NOT_STORE_FROM_KEYED) at ../deps/v8/src/lookup.cc:317
#12 0x0000000001a0f3b8 in v8::internal::Object::AddDataProperty (it=0x7fffffffc8a0, value=..., attributes=v8::internal::NONE, should_throw=v8::internal::Object::DONT_THROW, store_mode=v8::internal::Object::CERTAINLY_NOT_STORE_FROM_KEYED) at ../deps/v8/src/objects.cc:4614
#13 0x0000000001a139a1 in v8::internal::JSObject::DefineOwnPropertyIgnoreAttributes (it=0x7fffffffc8a0, value=..., attributes=v8::internal::NONE, should_throw=v8::internal::Object::DONT_THROW, handling=v8::internal::JSObject::DONT_FORCE_FIELD) at ../deps/v8/src/objects.cc:5476
#14 0x0000000001bca905 in v8::internal::__RT_impl_Runtime_DefineDataPropertyInLiteral (args=..., isolate=0x2edc350) at ../deps/v8/src/runtime/runtime-object.cc:874
#15 0x0000000001bca4ab in v8::internal::Runtime_DefineDataPropertyInLiteral (args_length=5, args_object=0x7fffffffca28, isolate=0x2edc350) at ../deps/v8/src/runtime/runtime-object.cc:855
#16 0x00000b95a5c060c7 in ?? ()
<snip>

[bnoordhuis]

I tracked this down to a TurboFan issue. node --nonative_context_specialization side-steps it, as does the patch below.

The function is sent to TurboFan because Crankshaft doesn't deal with computed properties but when native context specialization is enabled (the default), TF computes wrong deoptimization data. The function has deoptimization data, just not for that particular safepoint.

Looks like "IC propagation bug" wasn't too far off the mark because I'm reasonably confident the bug is in JSNativeContextSpecialization::ReduceNamedAccess(), which deals with just that.

diff --git a/deps/v8/src/compiler/pipeline.cc b/deps/v8/src/compiler/pipeline.cc
index 1d7e967..63a83e8 100644
--- a/deps/v8/src/compiler/pipeline.cc
+++ b/deps/v8/src/compiler/pipeline.cc
@@ -588,13 +588,12 @@ struct InliningPhase {
     if (data->info()->is_frame_specializing()) {
       AddReducer(data, &graph_reducer, &frame_specialization);
     }
     if (data->info()->is_deoptimization_enabled()) {
       AddReducer(data, &graph_reducer, &global_object_specialization);
     }
-    AddReducer(data, &graph_reducer, &native_context_specialization);
     AddReducer(data, &graph_reducer, &context_specialization);
     AddReducer(data, &graph_reducer, &call_reducer);
     AddReducer(data, &graph_reducer, &inlining);
     graph_reducer.ReduceGraph();
   }
 };

[bmeurer]

@fhinkel that looks like one of the bugs we had in DefineDataPropertyInLiteral in the beginning. Probably missing back-merge. Can you take a look?

[bnoordhuis]

Ping @fhinkel. Is this something that can be fixed easily or should we consider defaulting to --nonative_context_specialization?

[fhinkel]

Sorry, no idea. 51 (Node 6) was cut before I joined the team.

[targos]

Fix in #17290