Fixing a subtle bug in receiver resolution when a thrown and caught function is calle...

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

diff --git a/src/ast.h b/src/ast.h
index 80e7df8..77a6faa 100644 (file)
--- 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 fa2541a..d95a6ad 100644 (file)
--- 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 b07b0da..4ade882 100644 (file)
--- 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 30aa5bb..d6567dc 100644 (file)
--- 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 2c82b1d..9f6025b 100644 (file)
--- a/src/factory.cc
+++ b/src/factory.cc
@@ -130,8 +130,12 @@ Handle<Context> Factory::NewFunctionContext(int length,
 
 
 Handle<Context> Factory::NewWithContext(Handle<Context> previous,
-                                        Handle<JSObject> extension) {
-  CALL_HEAP_FUNCTION(Heap::AllocateWithContext(*previous, *extension), Context);
+                                        Handle<JSObject> 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 429c483..3cbefd8 100644 (file)
--- a/src/factory.h
+++ b/src/factory.h
@@ -126,7 +126,8 @@ class Factory : public AllStatic {
 
   // Create a 'with' context.
   static Handle<Context> NewWithContext(Handle<Context> previous,
-                                        Handle<JSObject> extension);
+                                        Handle<JSObject> extension,
+                                        bool is_catch_context);
 
   // Return the Symbol maching the passed in string.
   static Handle<String> SymbolFromString(Handle<String> value);

diff --git a/src/heap.cc b/src/heap.cc
index 53b0774..a10e3f9 100644 (file)
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -1017,6 +1017,10 @@ bool Heap::CreateInitialMaps() {
 
   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);
 
   obj = AllocateMap(JS_FUNCTION_TYPE, JSFunction::kSize);
@@ -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<Context*>(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 f17a091..3754c0b 100644 (file)
--- 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 5c70463..0bad5a1 100644 (file)
--- 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 f400e99..a1ddd48 100644 (file)
--- 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 05ed7fe..1dab710 100644 (file)
--- 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*>* label_list = NEW(ZoneList<Label*>(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 70d9d83..69e2b6f 100644 (file)
--- 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<Object> handle(args[0]);
+      Handle<Object> handle(object);
       Handle<Object> 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<JSObject>::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<Context> CopyWithContextChain(Handle<Context> context_chain,
   Handle<Context> previous(context_chain->previous());
   Handle<JSObject> 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 328178f..baff9f9 100644 (file)
--- 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 (file)
index 0000000..fd24f6e
--- /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);