From 9c2d52eb0e30e6bcc544c577ce952c40ace89fe6 Mon Sep 17 00:00:00 2001 From: "kmillikin@chromium.org" Date: Mon, 24 Jan 2011 14:03:30 +0000 Subject: [PATCH] Fix a bug in delete for lookup slots. The function Runtime_LookupContext searches the context chain for a LOOKUP slot and returns the object holding the slot. It returned the global context if the slot was not found or if it was found in a function's context or arguments object. This is not the correct object to use for 'delete'. Since this lookup function is only ever used when deleting LOOKUP slots (those that have to go through a with or a scope with eval), it is simply replaced with a Runtime_DeleteContextSlot function that does the appropriate thing for all kinds of context lookups. This fixes Chromium bug 70066. http://code.google.com/p/chromium/issues/detail?id=70066 Review URL: http://codereview.chromium.org/6280013 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6442 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/codegen-arm.cc | 8 +- src/arm/full-codegen-arm.cc | 12 ++- src/ia32/codegen-ia32.cc | 10 +-- src/ia32/full-codegen-ia32.cc | 11 ++- src/runtime.cc | 37 ++++++--- src/runtime.h | 2 +- src/x64/codegen-x64.cc | 10 +-- src/x64/full-codegen-x64.cc | 11 ++- test/mjsunit/regress/regress-70066.js | 141 ++++++++++++++++++++++++++++++++++ 9 files changed, 188 insertions(+), 54 deletions(-) create mode 100644 test/mjsunit/regress/regress-70066.js diff --git a/src/arm/codegen-arm.cc b/src/arm/codegen-arm.cc index 2fa0711..c5a2b2b 100644 --- a/src/arm/codegen-arm.cc +++ b/src/arm/codegen-arm.cc @@ -5849,14 +5849,10 @@ void CodeGenerator::VisitUnaryOperation(UnaryOperation* node) { frame_->EmitPush(r0); } else if (slot != NULL && slot->type() == Slot::LOOKUP) { - // lookup the context holding the named variable + // Delete from the context holding the named variable. frame_->EmitPush(cp); frame_->EmitPush(Operand(variable->name())); - frame_->CallRuntime(Runtime::kLookupContext, 2); - // r0: context - frame_->EmitPush(r0); - frame_->EmitPush(Operand(variable->name())); - frame_->InvokeBuiltin(Builtins::DELETE, CALL_JS, 2); + frame_->CallRuntime(Runtime::kDeleteContextSlot, 2); frame_->EmitPush(r0); } else { diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc index ddc74e2..eb66527 100644 --- a/src/arm/full-codegen-arm.cc +++ b/src/arm/full-codegen-arm.cc @@ -2992,22 +2992,20 @@ void FullCodeGenerator::VisitUnaryOperation(UnaryOperation* expr) { if (prop != NULL) { VisitForStackValue(prop->obj()); VisitForStackValue(prop->key()); + __ InvokeBuiltin(Builtins::DELETE, CALL_JS); } else if (var->is_global()) { __ ldr(r1, GlobalObjectOperand()); __ mov(r0, Operand(var->name())); __ Push(r1, r0); + __ InvokeBuiltin(Builtins::DELETE, CALL_JS); } else { - // Non-global variable. Call the runtime to look up the context - // where the variable was introduced. + // Non-global variable. Call the runtime to delete from the + // context where the variable was introduced. __ push(context_register()); __ mov(r2, Operand(var->name())); __ push(r2); - __ CallRuntime(Runtime::kLookupContext, 2); - __ push(r0); - __ mov(r2, Operand(var->name())); - __ push(r2); + __ CallRuntime(Runtime::kDeleteContextSlot, 2); } - __ InvokeBuiltin(Builtins::DELETE, CALL_JS); context()->Plug(r0); } break; diff --git a/src/ia32/codegen-ia32.cc b/src/ia32/codegen-ia32.cc index 29c8c0e..28ca5d0 100644 --- a/src/ia32/codegen-ia32.cc +++ b/src/ia32/codegen-ia32.cc @@ -8230,19 +8230,13 @@ void CodeGenerator::VisitUnaryOperation(UnaryOperation* node) { return; } else if (slot != NULL && slot->type() == Slot::LOOKUP) { - // Call the runtime to look up the context holding the named + // Call the runtime to delete from the context holding the named // variable. Sync the virtual frame eagerly so we can push the // arguments directly into place. frame_->SyncRange(0, frame_->element_count() - 1); frame_->EmitPush(esi); frame_->EmitPush(Immediate(variable->name())); - Result context = frame_->CallRuntime(Runtime::kLookupContext, 2); - ASSERT(context.is_register()); - frame_->EmitPush(context.reg()); - context.Unuse(); - frame_->EmitPush(Immediate(variable->name())); - Result answer = frame_->InvokeBuiltin(Builtins::DELETE, - CALL_FUNCTION, 2); + Result answer = frame_->CallRuntime(Runtime::kDeleteContextSlot, 2); frame_->Push(&answer); return; } diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc index 772eb8f..95213d1 100644 --- a/src/ia32/full-codegen-ia32.cc +++ b/src/ia32/full-codegen-ia32.cc @@ -3689,19 +3689,18 @@ void FullCodeGenerator::VisitUnaryOperation(UnaryOperation* expr) { if (prop != NULL) { VisitForStackValue(prop->obj()); VisitForStackValue(prop->key()); + __ InvokeBuiltin(Builtins::DELETE, CALL_FUNCTION); } else if (var->is_global()) { __ push(GlobalObjectOperand()); __ push(Immediate(var->name())); + __ InvokeBuiltin(Builtins::DELETE, CALL_FUNCTION); } else { - // Non-global variable. Call the runtime to look up the context - // where the variable was introduced. + // Non-global variable. Call the runtime to delete from the + // context where the variable was introduced. __ push(context_register()); __ push(Immediate(var->name())); - __ CallRuntime(Runtime::kLookupContext, 2); - __ push(eax); - __ push(Immediate(var->name())); + __ CallRuntime(Runtime::kDeleteContextSlot, 2); } - __ InvokeBuiltin(Builtins::DELETE, CALL_FUNCTION); context()->Plug(eax); } break; diff --git a/src/runtime.cc b/src/runtime.cc index 2f1f54c..1ef2dad 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -7049,7 +7049,7 @@ static MaybeObject* Runtime_PushCatchContext(Arguments args) { } -static MaybeObject* Runtime_LookupContext(Arguments args) { +static MaybeObject* Runtime_DeleteContextSlot(Arguments args) { HandleScope scope; ASSERT(args.length() == 2); @@ -7059,16 +7059,31 @@ static MaybeObject* Runtime_LookupContext(Arguments args) { int index; PropertyAttributes attributes; ContextLookupFlags flags = FOLLOW_CHAINS; - Handle holder = - context->Lookup(name, flags, &index, &attributes); + Handle holder = context->Lookup(name, flags, &index, &attributes); + + // If the slot was not found the result is true. + if (holder.is_null()) { + return Heap::true_value(); + } - if (index < 0 && !holder.is_null()) { - ASSERT(holder->IsJSObject()); - return *holder; + // If the slot was found in a context, it should be DONT_DELETE. + if (holder->IsContext()) { + return Heap::false_value(); } - // No intermediate context found. Use global object by default. - return Top::context()->global(); + // The slot was found in a JSObject, either a context extension object, + // the global object, or an arguments object. Try to delete it + // (respecting DONT_DELETE). For consistency with V8's usual behavior, + // which allows deleting all parameters in functions that mention + // 'arguments', we do this even for the case of slots found on an + // arguments object. The slot was found on an arguments object if the + // index is non-negative. + Handle object = Handle::cast(holder); + if (index >= 0) { + return object->DeleteElement(index, JSObject::NORMAL_DELETION); + } else { + return object->DeleteProperty(*name, JSObject::NORMAL_DELETION); + } } @@ -7141,8 +7156,7 @@ static ObjectPair LoadContextSlotHelper(Arguments args, bool throw_error) { int index; PropertyAttributes attributes; ContextLookupFlags flags = FOLLOW_CHAINS; - Handle holder = - context->Lookup(name, flags, &index, &attributes); + Handle holder = context->Lookup(name, flags, &index, &attributes); // If the index is non-negative, the slot has been found in a local // variable or a parameter. Read it from the context object or the @@ -7209,8 +7223,7 @@ static MaybeObject* Runtime_StoreContextSlot(Arguments args) { int index; PropertyAttributes attributes; ContextLookupFlags flags = FOLLOW_CHAINS; - Handle holder = - context->Lookup(name, flags, &index, &attributes); + Handle holder = context->Lookup(name, flags, &index, &attributes); if (index >= 0) { if (holder->IsContext()) { diff --git a/src/runtime.h b/src/runtime.h index dbd8d64..d8b3457 100644 --- a/src/runtime.h +++ b/src/runtime.h @@ -284,7 +284,7 @@ namespace internal { F(NewContext, 1, 1) \ F(PushContext, 1, 1) \ F(PushCatchContext, 1, 1) \ - F(LookupContext, 2, 1) \ + F(DeleteContextSlot, 2, 1) \ F(LoadContextSlot, 2, 2) \ F(LoadContextSlotNoReferenceError, 2, 2) \ F(StoreContextSlot, 3, 1) \ diff --git a/src/x64/codegen-x64.cc b/src/x64/codegen-x64.cc index 57720a8..e13b2e90 100644 --- a/src/x64/codegen-x64.cc +++ b/src/x64/codegen-x64.cc @@ -7235,19 +7235,13 @@ void CodeGenerator::VisitUnaryOperation(UnaryOperation* node) { return; } else if (slot != NULL && slot->type() == Slot::LOOKUP) { - // Call the runtime to look up the context holding the named + // Call the runtime to delete from the context holding the named // variable. Sync the virtual frame eagerly so we can push the // arguments directly into place. frame_->SyncRange(0, frame_->element_count() - 1); frame_->EmitPush(rsi); frame_->EmitPush(variable->name()); - Result context = frame_->CallRuntime(Runtime::kLookupContext, 2); - ASSERT(context.is_register()); - frame_->EmitPush(context.reg()); - context.Unuse(); - frame_->EmitPush(variable->name()); - Result answer = frame_->InvokeBuiltin(Builtins::DELETE, - CALL_FUNCTION, 2); + Result answer = frame_->CallRuntime(Runtime::kDeleteContextSlot, 2); frame_->Push(&answer); return; } diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc index 896e53d..da1a7ae 100644 --- a/src/x64/full-codegen-x64.cc +++ b/src/x64/full-codegen-x64.cc @@ -3006,19 +3006,18 @@ void FullCodeGenerator::VisitUnaryOperation(UnaryOperation* expr) { if (prop != NULL) { VisitForStackValue(prop->obj()); VisitForStackValue(prop->key()); + __ InvokeBuiltin(Builtins::DELETE, CALL_FUNCTION); } else if (var->is_global()) { __ push(GlobalObjectOperand()); __ Push(var->name()); + __ InvokeBuiltin(Builtins::DELETE, CALL_FUNCTION); } else { - // Non-global variable. Call the runtime to look up the context - // where the variable was introduced. + // Non-global variable. Call the runtime to delete from the + // context where the variable was introduced. __ push(context_register()); __ Push(var->name()); - __ CallRuntime(Runtime::kLookupContext, 2); - __ push(rax); - __ Push(var->name()); + __ CallRuntime(Runtime::kDeleteContextSlot, 2); } - __ InvokeBuiltin(Builtins::DELETE, CALL_FUNCTION); context()->Plug(rax); } break; diff --git a/test/mjsunit/regress/regress-70066.js b/test/mjsunit/regress/regress-70066.js new file mode 100644 index 0000000..704090b --- /dev/null +++ b/test/mjsunit/regress/regress-70066.js @@ -0,0 +1,141 @@ +// Copyright 2011 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Regression test for Chromium issue 70066. Delete should work properly +// from inside 'with' scopes. +// http://code.google.com/p/chromium/issues/detail?id=70066 + +x = 0; + +// Delete on a slot from a function's own context. +function test1() { + var value = 1; + var status; + with ({}) { status = delete value; } + return value + ":" + status; +} + +assertEquals("1:false", test1(), "test1"); +assertEquals(0, x, "test1"); // Global x is undisturbed. + + +// Delete on a slot from an outer context. +function test2() { + function f() { + with ({}) { return delete value; } + } + var value = 2; + var status = f(); + return value + ":" + status; +} + +assertEquals("2:false", test2(), "test2"); +assertEquals(0, x, "test2"); // Global x is undisturbed. + + +// Delete on an argument (should be the same code paths as test1 and test2). +function test3(value) { + var status; + with ({}) { status = delete value; } + return value + ":" + status; +} + +assertEquals("3:false", test3(3), "test3"); +assertEquals(0, x, "test3"); // Global x is undisturbed. + +function test4(value) { + function f() { + with ({}) { return delete value; } + } + var status = f(); + return value + ":" + status; +} + +assertEquals("4:false", test4(4), "test4"); +assertEquals(0, x, "test4"); // Global x is undisturbed. + + +// Delete on an argument found in the arguments object. Such properties are +// normally DONT_DELETE in JavaScript but deletion is allowed by V8. +function test5(value) { + var status; + with ({}) { status = delete value; } + return arguments[0] + ":" + status; +} + +assertEquals("undefined:true", test5(5), "test5"); +assertEquals(0, x, "test5"); // Global x is undisturbed. + +function test6(value) { + function f() { + with ({}) { return delete value; } + } + var status = f(); + return arguments[0] + ":" + status; +} + +assertEquals("undefined:true", test6(6), "test6"); +assertEquals(0, x, "test6"); // Global x is undisturbed. + + +// Delete on a property found on 'with' object. +function test7(object) { + with (object) { return delete value; } +} + +var o = {value: 7}; +assertEquals(true, test7(o), "test7"); +assertEquals(void 0, o.value, "test7"); +assertEquals(0, x, "test7"); // Global x is undisturbed. + + +// Delete on a global property. +function test8() { + with ({}) { return delete x; } +} + +assertEquals(true, test8(), "test8"); +assertThrows("x", "test8"); // Global x should be deleted. + + +// Delete on a property that is not found anywhere. +function test9() { + with ({}) { return delete x; } +} + +assertThrows("x", "test9"); // Make sure it's not there. +assertEquals(true, test9(), "test9"); + + +// Delete on a DONT_DELETE property of the global object. +var y = 10; +function test10() { + with ({}) { return delete y; } +} + +assertEquals(false, test10(), "test10"); +assertEquals(10, y, "test10"); -- 2.7.4