From 4c6f79ecec48ac9773b18454d79793804f70572f Mon Sep 17 00:00:00 2001 From: "ager@chromium.org" Date: Wed, 24 Nov 2010 06:26:36 +0000 Subject: [PATCH] Fix crashes during GC caused by partially initialized objects. The inline allocation code used the expected number of properties to calculate the number of inobject properties for an object instead of getting the actual number from the initial map. It is safer to use the inobject property count from the initial map in any case because that is the amount the instances will get. I think this disconnect got introduced when adding shrinking of objects. Unfortuntely I haven't been able to create a simple reproduction for a test case but this fixes the webpage that exhibits the crash. I'll see if I can create a reproduction tomorrow. Review URL: http://codereview.chromium.org/5278003 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@5879 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/stub-cache-arm.cc | 7 ++++--- src/handles.cc | 2 +- src/ia32/macro-assembler-ia32.cc | 8 +++----- src/ia32/macro-assembler-ia32.h | 1 - src/ia32/stub-cache-ia32.cc | 7 ++++--- src/runtime.cc | 3 +-- src/stub-cache.h | 2 +- src/x64/macro-assembler-x64.cc | 8 +++----- src/x64/macro-assembler-x64.h | 3 --- src/x64/stub-cache-x64.cc | 7 ++++--- 10 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/arm/stub-cache-arm.cc b/src/arm/stub-cache-arm.cc index f3f7a5d4b3..0a5eac27f6 100644 --- a/src/arm/stub-cache-arm.cc +++ b/src/arm/stub-cache-arm.cc @@ -2902,8 +2902,7 @@ MaybeObject* KeyedStoreStubCompiler::CompileStoreField(JSObject* object, } -MaybeObject* ConstructStubCompiler::CompileConstructStub( - SharedFunctionInfo* shared) { +MaybeObject* ConstructStubCompiler::CompileConstructStub(JSFunction* function) { // ----------- S t a t e ------------- // -- r0 : argc // -- r1 : constructor @@ -2987,6 +2986,7 @@ MaybeObject* ConstructStubCompiler::CompileConstructStub( // r7: undefined // Fill the initialized properties with a constant value or a passed argument // depending on the this.x = ...; assignment in the function. + SharedFunctionInfo* shared = function->shared(); for (int i = 0; i < shared->this_property_assignments_count(); i++) { if (shared->IsThisPropertyAssignmentArgument(i)) { Label not_passed, next; @@ -3011,8 +3011,9 @@ MaybeObject* ConstructStubCompiler::CompileConstructStub( } // Fill the unused in-object property fields with undefined. + ASSERT(function->has_initial_map()); for (int i = shared->this_property_assignments_count(); - i < shared->CalculateInObjectProperties(); + i < function->initial_map()->inobject_properties(); i++) { __ str(r7, MemOperand(r5, kPointerSize, PostIndex)); } diff --git a/src/handles.cc b/src/handles.cc index 3dfb886e99..37a5011ce7 100644 --- a/src/handles.cc +++ b/src/handles.cc @@ -143,7 +143,7 @@ Handle ReinitializeJSGlobalProxy( void SetExpectedNofProperties(Handle func, int nof) { // If objects constructed from this function exist then changing - // 'estimated_nof_properties' is dangerous since the previois value might + // 'estimated_nof_properties' is dangerous since the previous value might // have been compiled into the fast construct stub. More over, the inobject // slack tracking logic might have adjusted the previous value, so even // passing the same value is risky. diff --git a/src/ia32/macro-assembler-ia32.cc b/src/ia32/macro-assembler-ia32.cc index 61aadf7ebd..cbf93dd6a1 100644 --- a/src/ia32/macro-assembler-ia32.cc +++ b/src/ia32/macro-assembler-ia32.cc @@ -537,7 +537,6 @@ void MacroAssembler::CheckAccessGlobalProxy(Register holder_reg, void MacroAssembler::LoadAllocationTopHelper(Register result, - Register result_end, Register scratch, AllocationFlags flags) { ExternalReference new_space_allocation_top = @@ -559,7 +558,6 @@ void MacroAssembler::LoadAllocationTopHelper(Register result, if (scratch.is(no_reg)) { mov(result, Operand::StaticVariable(new_space_allocation_top)); } else { - ASSERT(!scratch.is(result_end)); mov(Operand(scratch), Immediate(new_space_allocation_top)); mov(result, Operand(scratch, 0)); } @@ -608,7 +606,7 @@ void MacroAssembler::AllocateInNewSpace(int object_size, ASSERT(!result.is(result_end)); // Load address of new object into result. - LoadAllocationTopHelper(result, result_end, scratch, flags); + LoadAllocationTopHelper(result, scratch, flags); Register top_reg = result_end.is_valid() ? result_end : result; @@ -664,7 +662,7 @@ void MacroAssembler::AllocateInNewSpace(int header_size, ASSERT(!result.is(result_end)); // Load address of new object into result. - LoadAllocationTopHelper(result, result_end, scratch, flags); + LoadAllocationTopHelper(result, scratch, flags); // Calculate new top and bail out if new space is exhausted. ExternalReference new_space_allocation_limit = @@ -705,7 +703,7 @@ void MacroAssembler::AllocateInNewSpace(Register object_size, ASSERT(!result.is(result_end)); // Load address of new object into result. - LoadAllocationTopHelper(result, result_end, scratch, flags); + LoadAllocationTopHelper(result, scratch, flags); // Calculate new top and bail out if new space is exhausted. ExternalReference new_space_allocation_limit = diff --git a/src/ia32/macro-assembler-ia32.h b/src/ia32/macro-assembler-ia32.h index cea7a70183..d208dbe3fa 100644 --- a/src/ia32/macro-assembler-ia32.h +++ b/src/ia32/macro-assembler-ia32.h @@ -631,7 +631,6 @@ class MacroAssembler: public Assembler { // Allocation support helpers. void LoadAllocationTopHelper(Register result, - Register result_end, Register scratch, AllocationFlags flags); void UpdateAllocationTopHelper(Register result_end, Register scratch); diff --git a/src/ia32/stub-cache-ia32.cc b/src/ia32/stub-cache-ia32.cc index 3120ff9da7..adcb5219ec 100644 --- a/src/ia32/stub-cache-ia32.cc +++ b/src/ia32/stub-cache-ia32.cc @@ -3021,8 +3021,7 @@ MaybeObject* KeyedLoadStubCompiler::CompileLoadFunctionPrototype(String* name) { // Specialized stub for constructing objects from functions which only have only // simple assignments of the form this.x = ...; in their body. -MaybeObject* ConstructStubCompiler::CompileConstructStub( - SharedFunctionInfo* shared) { +MaybeObject* ConstructStubCompiler::CompileConstructStub(JSFunction* function) { // ----------- S t a t e ------------- // -- eax : argc // -- edi : constructor @@ -3098,6 +3097,7 @@ MaybeObject* ConstructStubCompiler::CompileConstructStub( // edi: undefined // Fill the initialized properties with a constant value or a passed argument // depending on the this.x = ...; assignment in the function. + SharedFunctionInfo* shared = function->shared(); for (int i = 0; i < shared->this_property_assignments_count(); i++) { if (shared->IsThisPropertyAssignmentArgument(i)) { // Check if the argument assigned to the property is actually passed. @@ -3125,8 +3125,9 @@ MaybeObject* ConstructStubCompiler::CompileConstructStub( } // Fill the unused in-object property fields with undefined. + ASSERT(function->has_initial_map()); for (int i = shared->this_property_assignments_count(); - i < shared->CalculateInObjectProperties(); + i < function->initial_map()->inobject_properties(); i++) { __ mov(Operand(edx, i * kPointerSize), edi); } diff --git a/src/runtime.cc b/src/runtime.cc index 08f1865e71..c43a1ab327 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -6392,7 +6392,7 @@ static void TrySettingInlineConstructStub(Handle function) { } if (function->shared()->CanGenerateInlineConstructor(*prototype)) { ConstructStubCompiler compiler; - MaybeObject* code = compiler.CompileConstructStub(function->shared()); + MaybeObject* code = compiler.CompileConstructStub(*function); if (!code->IsFailure()) { function->shared()->set_construct_stub( Code::cast(code->ToObjectUnchecked())); @@ -6460,7 +6460,6 @@ static MaybeObject* Runtime_NewObject(Arguments args) { // track one initial_map at a time, so we force the completion before the // function is called as a constructor for the first time. shared->CompleteInobjectSlackTracking(); - TrySettingInlineConstructStub(function); } bool first_allocation = !shared->live_objects_may_exist(); diff --git a/src/stub-cache.h b/src/stub-cache.h index 4886c7eb29..cef5481c3f 100644 --- a/src/stub-cache.h +++ b/src/stub-cache.h @@ -740,7 +740,7 @@ class ConstructStubCompiler: public StubCompiler { public: explicit ConstructStubCompiler() {} - MUST_USE_RESULT MaybeObject* CompileConstructStub(SharedFunctionInfo* shared); + MUST_USE_RESULT MaybeObject* CompileConstructStub(JSFunction* function); private: MaybeObject* GetCode(); diff --git a/src/x64/macro-assembler-x64.cc b/src/x64/macro-assembler-x64.cc index 834a6f6ef5..d9198338b2 100644 --- a/src/x64/macro-assembler-x64.cc +++ b/src/x64/macro-assembler-x64.cc @@ -1889,7 +1889,6 @@ void MacroAssembler::CheckAccessGlobalProxy(Register holder_reg, void MacroAssembler::LoadAllocationTopHelper(Register result, - Register result_end, Register scratch, AllocationFlags flags) { ExternalReference new_space_allocation_top = @@ -1911,7 +1910,6 @@ void MacroAssembler::LoadAllocationTopHelper(Register result, // Move address of new object to result. Use scratch register if available, // and keep address in scratch until call to UpdateAllocationTopHelper. if (scratch.is_valid()) { - ASSERT(!scratch.is(result_end)); movq(scratch, new_space_allocation_top); movq(result, Operand(scratch, 0)); } else if (result.is(rax)) { @@ -1972,7 +1970,7 @@ void MacroAssembler::AllocateInNewSpace(int object_size, ASSERT(!result.is(result_end)); // Load address of new object into result. - LoadAllocationTopHelper(result, result_end, scratch, flags); + LoadAllocationTopHelper(result, scratch, flags); // Calculate new top and bail out if new space is exhausted. ExternalReference new_space_allocation_limit = @@ -2029,7 +2027,7 @@ void MacroAssembler::AllocateInNewSpace(int header_size, ASSERT(!result.is(result_end)); // Load address of new object into result. - LoadAllocationTopHelper(result, result_end, scratch, flags); + LoadAllocationTopHelper(result, scratch, flags); // Calculate new top and bail out if new space is exhausted. ExternalReference new_space_allocation_limit = @@ -2071,7 +2069,7 @@ void MacroAssembler::AllocateInNewSpace(Register object_size, ASSERT(!result.is(result_end)); // Load address of new object into result. - LoadAllocationTopHelper(result, result_end, scratch, flags); + LoadAllocationTopHelper(result, scratch, flags); // Calculate new top and bail out if new space is exhausted. ExternalReference new_space_allocation_limit = diff --git a/src/x64/macro-assembler-x64.h b/src/x64/macro-assembler-x64.h index 5b082fd627..0b7e6018f5 100644 --- a/src/x64/macro-assembler-x64.h +++ b/src/x64/macro-assembler-x64.h @@ -950,12 +950,9 @@ class MacroAssembler: public Assembler { // Allocation support helpers. // Loads the top of new-space into the result register. - // If flags contains RESULT_CONTAINS_TOP then result_end is valid and - // already contains the top of new-space, and scratch is invalid. // Otherwise the address of the new-space top is loaded into scratch (if // scratch is valid), and the new-space top is loaded into result. void LoadAllocationTopHelper(Register result, - Register result_end, Register scratch, AllocationFlags flags); // Update allocation top with value in result_end register. diff --git a/src/x64/stub-cache-x64.cc b/src/x64/stub-cache-x64.cc index dbf93f5ff2..7ba482c865 100644 --- a/src/x64/stub-cache-x64.cc +++ b/src/x64/stub-cache-x64.cc @@ -2890,8 +2890,7 @@ void StubCompiler::GenerateLoadConstant(JSObject* object, // Specialized stub for constructing objects from functions which only have only // simple assignments of the form this.x = ...; in their body. -MaybeObject* ConstructStubCompiler::CompileConstructStub( - SharedFunctionInfo* shared) { +MaybeObject* ConstructStubCompiler::CompileConstructStub(JSFunction* function) { // ----------- S t a t e ------------- // -- rax : argc // -- rdi : constructor @@ -2964,6 +2963,7 @@ MaybeObject* ConstructStubCompiler::CompileConstructStub( // r9: first in-object property of the JSObject // Fill the initialized properties with a constant value or a passed argument // depending on the this.x = ...; assignment in the function. + SharedFunctionInfo* shared = function->shared(); for (int i = 0; i < shared->this_property_assignments_count(); i++) { if (shared->IsThisPropertyAssignmentArgument(i)) { // Check if the argument assigned to the property is actually passed. @@ -2983,8 +2983,9 @@ MaybeObject* ConstructStubCompiler::CompileConstructStub( } // Fill the unused in-object property fields with undefined. + ASSERT(function->has_initial_map()); for (int i = shared->this_property_assignments_count(); - i < shared->CalculateInObjectProperties(); + i < function->initial_map()->inobject_properties(); i++) { __ movq(Operand(r9, i * kPointerSize), r8); }