From: ager@chromium.org Date: Thu, 22 Jan 2009 13:53:06 +0000 (+0000) Subject: Fix handling of const initialization. We did not handle the fact that X-Git-Tag: upstream/4.7.83~24767 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=c23dbc1928869b5968877e2fae79834081f4a025;p=platform%2Fupstream%2Fv8.git Fix handling of const initialization. We did not handle the fact that a const variable can be deleted between its declaration and its initialization. This fixes issue 189: http://code.google.com/p/v8/issues/detail?id=189 Review URL: http://codereview.chromium.org/18660 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1127 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- diff --git a/src/runtime.cc b/src/runtime.cc index b53b9cd..32df57b 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -758,56 +758,94 @@ static Object* Runtime_InitializeConstContextSlot(Arguments args) { int index; PropertyAttributes attributes; - ContextLookupFlags flags = DONT_FOLLOW_CHAINS; + ContextLookupFlags flags = FOLLOW_CHAINS; Handle holder = context->Lookup(name, flags, &index, &attributes); - // The property should always be present. It is always declared - // before being initialized through DeclareContextSlot. - ASSERT(attributes != ABSENT && (attributes & READ_ONLY) != 0); - - // If the slot is in the context, we set it but only if it hasn't - // been set before. + // In most situations, the property introduced by the const + // declaration should be present in the context extension object. + // However, because declaration and initialization are separate, the + // property might have been deleted (if it was introduced by eval) + // before we reach the initialization point. + // + // Example: + // + // function f() { eval("delete x; const x;"); } + // + // In that case, the initialization behaves like a normal assignment + // to property 'x'. if (index >= 0) { - // The constant context slot should always be in the function - // context; not in any outer context nor in the arguments object. - ASSERT(holder.is_identical_to(context)); - if (context->get(index)->IsTheHole()) { - context->set(index, *value); + // Property was found in a context. + if (holder->IsContext()) { + // The holder cannot be the function context. If it is, there + // should have been a const redeclaration error when declaring + // the const property. + ASSERT(!holder.is_identical_to(context)); + if ((attributes & READ_ONLY) == 0) { + Handle::cast(holder)->set(index, *value); + } + } else { + // The holder is an arguments object. + ASSERT((attributes & READ_ONLY) == 0); + Handle::cast(holder)->SetElement(index, *value); } return *value; } - // Otherwise, the slot must be in a JS object extension. - Handle context_ext(JSObject::cast(*holder)); + // The property could not be found, we introduce it in the global + // context. + if (attributes == ABSENT) { + Handle global = Handle(Top::context()->global()); + SetProperty(global, name, value, NONE); + return *value; + } - // We must initialize the value only if it wasn't initialized - // before, e.g. for const declarations in a loop. The property has - // the hole value if it wasn't initialized yet. NOTE: We cannot use - // GetProperty() to get the current value as it 'unholes' the value. - LookupResult lookup; - context_ext->LocalLookupRealNamedProperty(*name, &lookup); - ASSERT(lookup.IsProperty()); // the property was declared - ASSERT(lookup.IsReadOnly()); // and it was declared as read-only + // The property was present in a context extension object. + Handle context_ext = Handle::cast(holder); - PropertyType type = lookup.type(); - if (type == FIELD) { - FixedArray* properties = context_ext->properties(); - int index = lookup.GetFieldIndex(); - if (properties->get(index)->IsTheHole()) { - properties->set(index, *value); - } - } else if (type == NORMAL) { - Dictionary* dictionary = context_ext->property_dictionary(); - int entry = lookup.GetDictionaryEntry(); - if (dictionary->ValueAt(entry)->IsTheHole()) { - dictionary->ValueAtPut(entry, *value); + if (*context_ext == context->extension()) { + // This is the property that was introduced by the const + // declaration. Set it if it hasn't been set before. NOTE: We + // cannot use GetProperty() to get the current value as it + // 'unholes' the value. + LookupResult lookup; + context_ext->LocalLookupRealNamedProperty(*name, &lookup); + ASSERT(lookup.IsProperty()); // the property was declared + ASSERT(lookup.IsReadOnly()); // and it was declared as read-only + + PropertyType type = lookup.type(); + if (type == FIELD) { + FixedArray* properties = context_ext->properties(); + int index = lookup.GetFieldIndex(); + if (properties->get(index)->IsTheHole()) { + properties->set(index, *value); + } + } else if (type == NORMAL) { + Dictionary* dictionary = context_ext->property_dictionary(); + int entry = lookup.GetDictionaryEntry(); + if (dictionary->ValueAt(entry)->IsTheHole()) { + dictionary->ValueAtPut(entry, *value); + } + } else { + // We should not reach here. Any real, named property should be + // either a field or a dictionary slot. + UNREACHABLE(); } } else { - // We should not reach here. Any real, named property should be - // either a field or a dictionary slot. - UNREACHABLE(); + // The property was found in a different context extension object. + // Set it if it is not a read-only property. + if ((attributes & READ_ONLY) == 0) { + Handle set = SetProperty(context_ext, name, value, attributes); + // Setting a property might throw an exception. Exceptions + // are converted to empty handles in handle operations. We + // need to convert back to exceptions here. + if (set.is_null()) { + ASSERT(Top::has_pending_exception()); + return Failure::Exception(); + } + } } + return *value; } diff --git a/test/mjsunit/const-eval-init.js b/test/mjsunit/const-eval-init.js new file mode 100644 index 0000000..d3636de --- /dev/null +++ b/test/mjsunit/const-eval-init.js @@ -0,0 +1,111 @@ +// Copyright 2009 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. + +// Test the handling of initialization of deleted const variables. +// This only makes sense in local scopes since the declaration and +// initialization of consts in the global scope happen at the same +// time. + +function testIntroduceGlobal() { + var source = + // Deleting 'x' removes the local const property. + "delete x;" + + // Initialization turns into assignment to global 'x'. + "const x = 3; assertEquals(3, x);" + + // No constness of the global 'x'. + "x = 4; assertEquals(4, x);"; + eval(source); +} + +testIntroduceGlobal(); +assertEquals(4, x); + +function testAssignExistingGlobal() { + var source = + // Delete 'x' to remove the local const property. + "delete x;" + + // Initialization turns into assignment to global 'x'. + "const x = 5; assertEquals(5, x);" + + // No constness of the global 'x'. + "x = 6; assertEquals(6, x);"; + eval(source); +} + +testAssignExistingGlobal(); +assertEquals(6, x); + +function testAssignmentArgument(x) { + function local() { + var source = "delete x; const x = 7; assertEquals(7, x)"; + eval(source); + } + local(); + assertEquals(7, x); +} + +testAssignmentArgument(); +assertEquals(6, x); + +__defineSetter__('x', function() { throw 42; }); +function testAssignGlobalThrows() { + // Initialization turns into assignment to global 'x' which + // throws an exception. + var source = "delete x; const x = 8"; + eval(source); +} + +assertThrows("testAssignGlobalThrows()"); + +function testInitFastCaseExtension() { + var source = "const x = 9; assertEquals(9, x); x = 10; assertEquals(9, x)"; + eval(source); +} + +testInitFastCaseExtension(); + +function testInitSlowCaseExtension() { + var source = ""; + // Introduce 100 properties on the context extension object to force + // it in slow case. + for (var i = 0; i < 100; i++) source += ("var a" + i + " = i;"); + source += "const x = 10; assertEquals(10, x); x = 11; assertEquals(10, x)"; + eval(source); +} + +testInitSlowCaseExtension(); + +function testAssignSurroundingContextSlot() { + var x = 12; + function local() { + var source = "delete x; const x = 13; assertEquals(13, x)"; + eval(source); + } + local(); + assertEquals(13, x); +} + +testAssignSurroundingContextSlot(); diff --git a/test/mjsunit/const.js b/test/mjsunit/const.js index 5ad0c9c..a48e82d 100644 --- a/test/mjsunit/const.js +++ b/test/mjsunit/const.js @@ -52,17 +52,19 @@ var valueOfCount = 0; function g() { const o = { valueOf: function() { valueOfCount++; return 42; } } assertEquals(42, o); - assertEquals(0, valueOfCount); + assertEquals(1, valueOfCount); o++; assertEquals(42, o); - assertEquals(1, valueOfCount); + assertEquals(3, valueOfCount); ++o; assertEquals(42, o); - assertEquals(2, valueOfCount); + assertEquals(5, valueOfCount); o--; assertEquals(42, o); - assertEquals(3, valueOfCount); + assertEquals(7, valueOfCount); --o; assertEquals(42, o); - assertEquals(4, valueOfCount); + assertEquals(9, valueOfCount); } + +g(); diff --git a/test/mjsunit/regress/regress-189.js b/test/mjsunit/regress/regress-189.js new file mode 100644 index 0000000..a84b620 --- /dev/null +++ b/test/mjsunit/regress/regress-189.js @@ -0,0 +1,36 @@ +// Copyright 2009 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. + +// Test that we can handle initialization of a deleted const variable. + +// See http://code.google.com/p/v8/issues/detail?id=189. + +function f() { + eval("delete x; const x = 32"); +} + +f();