Skip to content

Commit 071d03a

Browse files
jakobkummerowCommit bot
jakobkummerow
authored and
Commit bot
committed
Add instrumentation to track down a crasher
LoadICs must always return a JS-accessible value (nothing internal). Dictionary property keys are guaranteed to be unique names. BUG=chromium:527994 LOG=n Review URL: https://codereview.chromium.org/1334673003 Cr-Commit-Position: refs/heads/master@{#30683}
1 parent 0cc8eaa commit 071d03a

3 files changed

Lines changed: 62 additions & 8 deletions

File tree

‎src/full-codegen/x64/full-codegen-x64.cc‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2243,10 +2243,46 @@ void FullCodeGenerator::EmitNamedPropertyLoad(Property* prop) {
22432243
Literal* key = prop->key()->AsLiteral();
22442244
DCHECK(!prop->IsSuperAccess());
22452245

2246+
// See comment below.
2247+
if (FeedbackVector()->GetIndex(prop->PropertyFeedbackSlot()) == 6) {
2248+
__ Push(LoadDescriptor::ReceiverRegister());
2249+
}
2250+
22462251
__ Move(LoadDescriptor::NameRegister(), key->value());
22472252
__ Move(LoadDescriptor::SlotRegister(),
22482253
SmiFromSlot(prop->PropertyFeedbackSlot()));
22492254
CallLoadIC(NOT_INSIDE_TYPEOF, language_mode());
2255+
2256+
// Sanity check: The loaded value must be a JS-exposed kind of object,
2257+
// not something internal (like a Map, or FixedArray). Check this here
2258+
// to chase after a rare but recurring crash bug. It seems to always
2259+
// occur for functions beginning with "this.foo.bar()", so be selective
2260+
// and only insert the check for the first LoadIC (identified by slot).
2261+
// TODO(jkummerow): Remove this when it has generated a few crash reports.
2262+
// Don't forget to remove the Push() above as well!
2263+
if (FeedbackVector()->GetIndex(prop->PropertyFeedbackSlot()) == 6) {
2264+
__ Pop(LoadDescriptor::ReceiverRegister());
2265+
2266+
Label ok;
2267+
__ JumpIfSmi(rax, &ok, Label::kNear);
2268+
__ movp(rbx, FieldOperand(rax, HeapObject::kMapOffset));
2269+
__ CmpInstanceType(rbx, LAST_PRIMITIVE_TYPE);
2270+
__ j(below_equal, &ok, Label::kNear);
2271+
__ CmpInstanceType(rbx, FIRST_JS_RECEIVER_TYPE);
2272+
__ j(above_equal, &ok, Label::kNear);
2273+
2274+
__ Push(Smi::FromInt(0xaabbccdd));
2275+
__ Push(LoadDescriptor::ReceiverRegister());
2276+
__ movp(rbx, FieldOperand(LoadDescriptor::ReceiverRegister(),
2277+
HeapObject::kMapOffset));
2278+
__ Push(rbx);
2279+
__ movp(rbx, FieldOperand(LoadDescriptor::ReceiverRegister(),
2280+
JSObject::kPropertiesOffset));
2281+
__ Push(rbx);
2282+
__ int3();
2283+
2284+
__ bind(&ok);
2285+
}
22502286
}
22512287

22522288

‎src/ic/ic.cc‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2377,6 +2377,17 @@ RUNTIME_FUNCTION(Runtime_LoadIC_Miss) {
23772377
LoadIC ic(IC::NO_EXTRA_FRAME, isolate, &nexus);
23782378
ic.UpdateState(receiver, key);
23792379
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result, ic.Load(receiver, key));
2380+
2381+
// Sanity check: The loaded value must be a JS-exposed kind of object,
2382+
// not something internal (like a Map, or FixedArray). Check this here
2383+
// to chase after a rare but recurring crash bug.
2384+
// TODO(jkummerow): Remove this when it has generated a few crash reports.
2385+
if (!result->IsSmi()) {
2386+
InstanceType type =
2387+
Handle<HeapObject>::cast(result)->map()->instance_type();
2388+
CHECK(type <= LAST_PRIMITIVE_TYPE || type >= FIRST_JS_RECEIVER_TYPE);
2389+
}
2390+
23802391
} else {
23812392
DCHECK(vector->GetKind(vector_slot) == Code::KEYED_LOAD_IC);
23822393
KeyedLoadICNexus nexus(vector, vector_slot);
@@ -3115,6 +3126,17 @@ RUNTIME_FUNCTION(Runtime_LoadIC_MissFromStubFailure) {
31153126
LoadIC ic(IC::EXTRA_CALL_FRAME, isolate, &nexus);
31163127
ic.UpdateState(receiver, key);
31173128
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result, ic.Load(receiver, key));
3129+
3130+
// Sanity check: The loaded value must be a JS-exposed kind of object,
3131+
// not something internal (like a Map, or FixedArray). Check this here
3132+
// to chase after a rare but recurring crash bug.
3133+
// TODO(jkummerow): Remove this when it has generated a few crash reports.
3134+
if (!result->IsSmi()) {
3135+
InstanceType type =
3136+
Handle<HeapObject>::cast(result)->map()->instance_type();
3137+
CHECK(type <= LAST_PRIMITIVE_TYPE || type >= FIRST_JS_RECEIVER_TYPE);
3138+
}
3139+
31183140
} else {
31193141
DCHECK(vector->GetKind(vector_slot) == Code::KEYED_LOAD_IC);
31203142
KeyedLoadICNexus nexus(vector, vector_slot);

‎src/objects.cc‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5001,16 +5001,12 @@ void JSObject::MigrateSlowToFast(Handle<JSObject> object,
50015001
int index = Smi::cast(iteration_order->get(i))->value();
50025002
Object* k = dictionary->KeyAt(index);
50035003
DCHECK(dictionary->IsKey(k));
5004+
// Dictionary keys are internalized upon insertion.
5005+
// TODO(jkummerow): Turn this into a DCHECK if it's not hit in the wild.
5006+
CHECK(k->IsUniqueName());
5007+
Handle<Name> key(Name::cast(k), isolate);
50045008

50055009
Object* value = dictionary->ValueAt(index);
5006-
Handle<Name> key;
5007-
if (k->IsSymbol()) {
5008-
key = handle(Symbol::cast(k));
5009-
} else {
5010-
// Ensure the key is a unique name before writing into the
5011-
// instance descriptor.
5012-
key = factory->InternalizeString(handle(String::cast(k)));
5013-
}
50145010

50155011
PropertyDetails details = dictionary->DetailsAt(index);
50165012
int enumeration_index = details.dictionary_index();

0 commit comments

Comments
 (0)