From 392e6f849b6e9b63c912c4ebf6e0e5412b16a742 Mon Sep 17 00:00:00 2001 From: olehougaard Date: Thu, 18 Dec 2008 11:28:13 +0000 Subject: [PATCH] Fixing a subtle bug in receiver resolution when a thrown and caught function is called from a catch-block. Second attempt - now with better memory efficiency. Review URL: http://codereview.chromium.org/14834 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@999 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/ast.h | 7 ++- src/codegen-arm.cc | 6 ++- src/codegen-ia32.cc | 6 ++- src/contexts.h | 6 +++ src/factory.cc | 8 +++- src/factory.h | 3 +- src/heap.cc | 10 ++++- src/heap.h | 5 ++- src/objects-inl.h | 7 +++ src/objects.h | 1 + src/parser.cc | 16 ++++--- src/runtime.cc | 54 ++++++++++++++++-------- src/runtime.h | 1 + test/mjsunit/throw-and-catch-function.js | 50 ++++++++++++++++++++++ 14 files changed, 148 insertions(+), 32 deletions(-) create mode 100644 test/mjsunit/throw-and-catch-function.js diff --git a/src/ast.h b/src/ast.h index 80e7df8c3..77a6faab9 100644 --- a/src/ast.h +++ b/src/ast.h @@ -409,15 +409,18 @@ class ReturnStatement: public Statement { class WithEnterStatement: public Statement { public: - explicit WithEnterStatement(Expression* expression) - : expression_(expression) { } + explicit WithEnterStatement(Expression* expression, bool is_catch_block) + : expression_(expression), is_catch_block_(is_catch_block) { } virtual void Accept(AstVisitor* v); Expression* expression() const { return expression_; } + bool is_catch_block() const { return is_catch_block_; } + private: Expression* expression_; + bool is_catch_block_; }; diff --git a/src/codegen-arm.cc b/src/codegen-arm.cc index fa2541a68..d95a6adae 100644 --- a/src/codegen-arm.cc +++ b/src/codegen-arm.cc @@ -1276,7 +1276,11 @@ void CodeGenerator::VisitWithEnterStatement(WithEnterStatement* node) { Comment cmnt(masm_, "[ WithEnterStatement"); CodeForStatement(node); Load(node->expression()); - __ CallRuntime(Runtime::kPushContext, 1); + if (node->is_catch_block()) { + __ CallRuntime(Runtime::kPushCatchContext, 1); + } else { + __ CallRuntime(Runtime::kPushContext, 1); + } if (kDebug) { Label verified_true; __ cmp(r0, Operand(cp)); diff --git a/src/codegen-ia32.cc b/src/codegen-ia32.cc index b07b0dad2..4ade88288 100644 --- a/src/codegen-ia32.cc +++ b/src/codegen-ia32.cc @@ -1586,7 +1586,11 @@ void CodeGenerator::VisitWithEnterStatement(WithEnterStatement* node) { Comment cmnt(masm_, "[ WithEnterStatement"); CodeForStatement(node); Load(node->expression()); - __ CallRuntime(Runtime::kPushContext, 1); + if (node->is_catch_block()) { + __ CallRuntime(Runtime::kPushCatchContext, 1); + } else { + __ CallRuntime(Runtime::kPushContext, 1); + } if (kDebug) { Label verified_true; diff --git a/src/contexts.h b/src/contexts.h index 30aa5bb9f..d6567dc24 100644 --- a/src/contexts.h +++ b/src/contexts.h @@ -264,6 +264,12 @@ class Context: public FixedArray { global_context()->set_out_of_memory(Heap::true_value()); } + // The exception holder is the object used as a with object in + // the implementation of a catch block. + bool is_exception_holder(Object* object) { + return IsCatchContext() && extension() == object; + } + #define GLOBAL_CONTEXT_FIELD_ACCESSORS(index, type, name) \ void set_##name(type* value) { \ ASSERT(IsGlobalContext()); \ diff --git a/src/factory.cc b/src/factory.cc index 2c82b1dff..9f6025b22 100644 --- a/src/factory.cc +++ b/src/factory.cc @@ -130,8 +130,12 @@ Handle Factory::NewFunctionContext(int length, Handle Factory::NewWithContext(Handle previous, - Handle extension) { - CALL_HEAP_FUNCTION(Heap::AllocateWithContext(*previous, *extension), Context); + Handle extension, + bool is_catch_context) { + CALL_HEAP_FUNCTION(Heap::AllocateWithContext(*previous, + *extension, + is_catch_context), + Context); } diff --git a/src/factory.h b/src/factory.h index 429c483c4..3cbefd8ad 100644 --- a/src/factory.h +++ b/src/factory.h @@ -126,7 +126,8 @@ class Factory : public AllStatic { // Create a 'with' context. static Handle NewWithContext(Handle previous, - Handle extension); + Handle extension, + bool is_catch_context); // Return the Symbol maching the passed in string. static Handle SymbolFromString(Handle value); diff --git a/src/heap.cc b/src/heap.cc index 53b0774ff..a10e3f997 100644 --- a/src/heap.cc +++ b/src/heap.cc @@ -1015,6 +1015,10 @@ bool Heap::CreateInitialMaps() { if (obj->IsFailure()) return false; context_map_ = Map::cast(obj); + obj = AllocateMap(FIXED_ARRAY_TYPE, HeapObject::kHeaderSize); + if (obj->IsFailure()) return false; + catch_context_map_ = Map::cast(obj); + obj = AllocateMap(FIXED_ARRAY_TYPE, HeapObject::kHeaderSize); if (obj->IsFailure()) return false; global_context_map_ = Map::cast(obj); @@ -2387,11 +2391,13 @@ Object* Heap::AllocateFunctionContext(int length, JSFunction* function) { } -Object* Heap::AllocateWithContext(Context* previous, JSObject* extension) { +Object* Heap::AllocateWithContext(Context* previous, + JSObject* extension, + bool is_catch_context) { Object* result = Heap::AllocateFixedArray(Context::MIN_CONTEXT_SLOTS); if (result->IsFailure()) return result; Context* context = reinterpret_cast(result); - context->set_map(context_map()); + context->set_map(is_catch_context ? catch_context_map() : context_map()); context->set_closure(previous->closure()); context->set_fcontext(previous->fcontext()); context->set_previous(previous); diff --git a/src/heap.h b/src/heap.h index f17a09134..3754c0b2a 100644 --- a/src/heap.h +++ b/src/heap.h @@ -92,6 +92,7 @@ namespace v8 { namespace internal { V(Map, fixed_array_map) \ V(Map, hash_table_map) \ V(Map, context_map) \ + V(Map, catch_context_map) \ V(Map, global_context_map) \ V(Map, code_map) \ V(Map, oddball_map) \ @@ -430,7 +431,9 @@ class Heap : public AllStatic { static Object* AllocateFunctionContext(int length, JSFunction* closure); // Allocate a 'with' context. - static Object* AllocateWithContext(Context* previous, JSObject* extension); + static Object* AllocateWithContext(Context* previous, + JSObject* extension, + bool is_catch_context); // Allocates a new utility object in the old generation. static Object* AllocateStruct(InstanceType type); diff --git a/src/objects-inl.h b/src/objects-inl.h index 5c70463ac..0bad5a160 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -348,10 +348,17 @@ bool Object::IsDescriptorArray() { bool Object::IsContext() { return Object::IsHeapObject() && (HeapObject::cast(this)->map() == Heap::context_map() || + HeapObject::cast(this)->map() == Heap::catch_context_map() || HeapObject::cast(this)->map() == Heap::global_context_map()); } +bool Object::IsCatchContext() { + return Object::IsHeapObject() + && HeapObject::cast(this)->map() == Heap::catch_context_map(); +} + + bool Object::IsGlobalContext() { return Object::IsHeapObject() && HeapObject::cast(this)->map() == Heap::global_context_map(); diff --git a/src/objects.h b/src/objects.h index f400e9971..a1ddd4809 100644 --- a/src/objects.h +++ b/src/objects.h @@ -616,6 +616,7 @@ class Object BASE_EMBEDDED { inline bool IsFixedArray(); inline bool IsDescriptorArray(); inline bool IsContext(); + inline bool IsCatchContext(); inline bool IsGlobalContext(); inline bool IsJSFunction(); inline bool IsCode(); diff --git a/src/parser.cc b/src/parser.cc index 05ed7fefd..1dab710c9 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -120,7 +120,10 @@ class Parser { Statement* ParseContinueStatement(bool* ok); Statement* ParseBreakStatement(ZoneStringList* labels, bool* ok); Statement* ParseReturnStatement(bool* ok); - Block* WithHelper(Expression* obj, ZoneStringList* labels, bool* ok); + Block* WithHelper(Expression* obj, + ZoneStringList* labels, + bool is_catch_block, + bool* ok); Statement* ParseWithStatement(ZoneStringList* labels, bool* ok); CaseClause* ParseCaseClause(bool* default_seen_ptr, bool* ok); SwitchStatement* ParseSwitchStatement(ZoneStringList* labels, bool* ok); @@ -1923,7 +1926,10 @@ Statement* Parser::ParseReturnStatement(bool* ok) { } -Block* Parser::WithHelper(Expression* obj, ZoneStringList* labels, bool* ok) { +Block* Parser::WithHelper(Expression* obj, + ZoneStringList* labels, + bool is_catch_block, + bool* ok) { // Parse the statement and collect escaping labels. ZoneList* label_list = NEW(ZoneList(0)); LabelCollector collector(label_list); @@ -1940,7 +1946,7 @@ Block* Parser::WithHelper(Expression* obj, ZoneStringList* labels, bool* ok) { Block* result = NEW(Block(NULL, 2, false)); if (result) { - result->AddStatement(NEW(WithEnterStatement(obj))); + result->AddStatement(NEW(WithEnterStatement(obj, is_catch_block))); // Create body block. Block* body = NEW(Block(NULL, 1, false)); @@ -1976,7 +1982,7 @@ Statement* Parser::ParseWithStatement(ZoneStringList* labels, bool* ok) { Expression* expr = ParseExpression(true, CHECK_OK); Expect(Token::RPAREN, CHECK_OK); - return WithHelper(expr, labels, CHECK_OK); + return WithHelper(expr, labels, false, CHECK_OK); } @@ -2137,7 +2143,7 @@ TryStatement* Parser::ParseTryStatement(bool* ok) { catch_var = top_scope_->NewTemporary(Factory::catch_var_symbol()); Expression* obj = MakeCatchContext(name, catch_var); { Target target(this, &catch_collector); - catch_block = WithHelper(obj, NULL, CHECK_OK); + catch_block = WithHelper(obj, NULL, true, CHECK_OK); } } else { Expect(Token::LBRACE, CHECK_OK); diff --git a/src/runtime.cc b/src/runtime.cc index 70d9d838f..69e2b6fd3 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -3428,19 +3428,15 @@ static Object* Runtime_NewContext(Arguments args) { return result; // non-failure } - -static Object* Runtime_PushContext(Arguments args) { - NoHandleAllocation ha; - ASSERT(args.length() == 1); - +static Object* PushContextHelper(Object* object, bool is_catch_context) { // Convert the object to a proper JavaScript object. - Object* object = args[0]; - if (!object->IsJSObject()) { - object = object->ToObject(); - if (object->IsFailure()) { - if (!Failure::cast(object)->IsInternalError()) return object; + Object* js_object = object; + if (!js_object->IsJSObject()) { + js_object = js_object->ToObject(); + if (js_object->IsFailure()) { + if (!Failure::cast(js_object)->IsInternalError()) return js_object; HandleScope scope; - Handle handle(args[0]); + Handle handle(object); Handle result = Factory::NewTypeError("with_expression", HandleVector(&handle, 1)); return Top::Throw(*result); @@ -3448,15 +3444,32 @@ static Object* Runtime_PushContext(Arguments args) { } Object* result = - Heap::AllocateWithContext(Top::context(), JSObject::cast(object)); + Heap::AllocateWithContext(Top::context(), + JSObject::cast(js_object), + is_catch_context); if (result->IsFailure()) return result; - Top::set_context(Context::cast(result)); + Context* context = Context::cast(result); + Top::set_context(context); return result; } +static Object* Runtime_PushContext(Arguments args) { + NoHandleAllocation ha; + ASSERT(args.length() == 1); + return PushContextHelper(args[0], false); +} + + +static Object* Runtime_PushCatchContext(Arguments args) { + NoHandleAllocation ha; + ASSERT(args.length() == 1); + return PushContextHelper(args[0], true); +} + + static Object* Runtime_LookupContext(Arguments args) { HandleScope scope; ASSERT(args.length() == 2); @@ -3554,9 +3567,14 @@ static ObjectPair LoadContextSlotHelper(Arguments args, bool throw_error) { if (!holder.is_null() && holder->IsJSObject()) { ASSERT(Handle::cast(holder)->HasProperty(*name)); JSObject* object = JSObject::cast(*holder); - JSObject* receiver = (object->IsGlobalObject()) - ? GlobalObject::cast(object)->global_receiver() - : ComputeReceiverForNonGlobal(object); + JSObject* receiver; + if (object->IsGlobalObject()) { + receiver = GlobalObject::cast(object)->global_receiver(); + } else if (context->is_exception_holder(*holder)) { + receiver = Top::context()->global()->global_receiver(); + } else { + receiver = ComputeReceiverForNonGlobal(object); + } // No need to unhole the value here. This is taken care of by the // GetProperty function. Object* value = object->GetProperty(*name); @@ -5220,7 +5238,9 @@ static Handle CopyWithContextChain(Handle context_chain, Handle previous(context_chain->previous()); Handle extension(JSObject::cast(context_chain->extension())); return Factory::NewWithContext( - CopyWithContextChain(function_context, previous), extension); + CopyWithContextChain(function_context, previous), + extension, + context_chain->IsCatchContext()); } diff --git a/src/runtime.h b/src/runtime.h index 328178f12..baff9f9ee 100644 --- a/src/runtime.h +++ b/src/runtime.h @@ -264,6 +264,7 @@ namespace v8 { namespace internal { /* Contexts */ \ F(NewContext, 1) \ F(PushContext, 1) \ + F(PushCatchContext, 1) \ F(LookupContext, 2) \ F(LoadContextSlot, 2) \ F(LoadContextSlotNoReferenceError, 2) \ diff --git a/test/mjsunit/throw-and-catch-function.js b/test/mjsunit/throw-and-catch-function.js new file mode 100644 index 000000000..fd24f6e66 --- /dev/null +++ b/test/mjsunit/throw-and-catch-function.js @@ -0,0 +1,50 @@ +// Copyright 2008 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. + +var g = this; +var x = new Object(); +x.e = function() { return this; }; +try { + throw x.e; +} catch (e) { + assertTrue(e() === g); +} +try { + throw x.e; +} catch (e) { + with(x) { assertTrue(e() === x); } +} +with(x) { + try { throw e; } catch (e) { assertTrue(e() === g); } +} +var e = 0; +try { + throw x.e; +} catch (e) { + var e = 7; +} +assertEquals(0, e); -- 2.34.1