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

diff --git a/src/arm/stub-cache-arm.cc b/src/arm/stub-cache-arm.cc
index f3f7a5d4b31b6525ea098a0c8454dd4f78733ce2..0a5eac27f6da96240eb917c63278bcd760a45c82 100644 (file)
--- 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 3dfb886e99085e8fdf762880e22c61f4f78b24af..37a5011ce761ca759de780786d9a069312ffae04 100644 (file)
--- a/src/handles.cc
+++ b/src/handles.cc
@@ -143,7 +143,7 @@ Handle<JSGlobalProxy> ReinitializeJSGlobalProxy(
 
 void SetExpectedNofProperties(Handle<JSFunction> 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 61aadf7ebd432734239813468e60e7373ee8509d..cbf93dd6a187264bc5a18f7669150e010a7401b3 100644 (file)
--- 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 cea7a70183ae13b44cf1ef6501642b17731f5063..d208dbe3fa88eb2d92fd4e6e9cde170d2b117798 100644 (file)
--- 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 3120ff9da7e43a4a164129548754ae9c2947757a..adcb5219eced1d6b619d5b82419c2fc1f50d54a5 100644 (file)
--- 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 08f1865e71846ca6bdf0884e0e74d295ef73135a..c43a1ab327c5e4952f454b2bb5d20f070e1fcb30 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -6392,7 +6392,7 @@ static void TrySettingInlineConstructStub(Handle<JSFunction> 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 4886c7eb29d2d74875932c338145443e7d1176e1..cef5481c3f5ce23618f06f892d7b5d5983b268df 100644 (file)
--- 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 834a6f6ef593db08f7c4d3eeb8ffe6ba5bd6ac59..d9198338b23df146600aec0cae39f03fa0c75092 100644 (file)
--- 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 5b082fd627e22fe62e41fa3a68e2474e2de44d59..0b7e6018f5609acfc9be339aaea75e0fe4bd6bf0 100644 (file)
--- 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 dbf93f5ff27be464456b9d01f9603da14ddf70e6..7ba482c8656ea067224cbaa73b2540becf242ac9 100644 (file)
--- 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);
   }