address review comments

MikeHolman · MikeHolman · commit 927377139ab5 · 2018-03-15T15:13:14.000-07:00
diff --git a/lib/Runtime/Library/JavascriptObject.cpp b/lib/Runtime/Library/JavascriptObject.cpp
@@ -1507,53 +1507,63 @@ namespace Js
 
         // 4. Let sources be the List of argument values starting with the second argument.
         // 5. For each element nextSource of sources, in ascending index order,
-        for (unsigned int i = 2; i < args.Info.Count; i++)
+        AssignHelper<true>(args[2], to, scriptContext);
+        for (unsigned int i = 3; i < args.Info.Count; i++)
         {
-            //      a. If nextSource is undefined or null, let keys be an empty List.
-            //      b. Else,
-            //          i.Let from be ToObject(nextSource).
-            //          ii.ReturnIfAbrupt(from).
-            //          iii.Let keys be from.[[OwnPropertyKeys]]().
-            //          iv.ReturnIfAbrupt(keys).
+            AssignHelper<false>(args[i], to, scriptContext);
+        }
+
+        // 6. Return to.
+        return to;
+    }
+
+    template <bool tryCopy>
+    void JavascriptObject::AssignHelper(Var fromArg, RecyclableObject* to, ScriptContext* scriptContext)
+    {
+        //      a. If nextSource is undefined or null, let keys be an empty List.
+        //      b. Else,
+        //          i.Let from be ToObject(nextSource).
+        //          ii.ReturnIfAbrupt(from).
+        //          iii.Let keys be from.[[OwnPropertyKeys]]().
+        //          iv.ReturnIfAbrupt(keys).
 
-            RecyclableObject* from = nullptr;
-            if (!JavascriptConversion::ToObject(args[i], scriptContext, &from))
+        RecyclableObject* from = nullptr;
+        if (!JavascriptConversion::ToObject(fromArg, scriptContext, &from))
+        {
+            if (JavascriptOperators::IsUndefinedOrNull(fromArg))
             {
-                if (JavascriptOperators::IsUndefinedOrNull(args[i]))
-                {
-                    continue;
-                }
-                JavascriptError::ThrowTypeError(scriptContext, JSERR_FunctionArgument_NeedObject, _u("Object.assign"));
+                return;
             }
+            JavascriptError::ThrowTypeError(scriptContext, JSERR_FunctionArgument_NeedObject, _u("Object.assign"));
+        }
 
 #if ENABLE_COPYONACCESS_ARRAY
-            JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(from);
+        JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(from);
 #endif
 
-            // if proxy, take slow path by calling [[OwnPropertyKeys]] on source
-            if (JavascriptProxy::Is(from))
-            {
-                AssignForProxyObjects(from, to, scriptContext);
-            }
-            // else use enumerator to extract keys from source
-            else
+        // if proxy, take slow path by calling [[OwnPropertyKeys]] on source
+        if (JavascriptProxy::Is(from))
+        {
+            AssignForProxyObjects(from, to, scriptContext);
+        }
+        // else use enumerator to extract keys from source
+        else
+        {
+            bool copied = false;
+            if (tryCopy)
             {
                 DynamicObject* fromObj = JavascriptOperators::TryFromVar<DynamicObject>(from);
                 DynamicObject* toObj = JavascriptOperators::TryFromVar<DynamicObject>(to);
-                bool cloned = false;
                 if (toObj && fromObj && toObj->GetType() == scriptContext->GetLibrary()->GetObjectType())
                 {
-                    cloned = toObj->TryCopy(fromObj);
-                }
-                if(!cloned)
-                {
-                    AssignForGenericObjects(from, to, scriptContext);
+                    copied = toObj->TryCopy(fromObj);
                 }
             }
+            if (!copied)
+            {
+                AssignForGenericObjects(from, to, scriptContext);
+            }
         }
-
-        // 6. Return to.
-        return to;
     }
 
     void JavascriptObject::AssignForGenericObjects(RecyclableObject* from, RecyclableObject* to, ScriptContext* scriptContext)
diff --git a/lib/Runtime/Library/JavascriptObject.h b/lib/Runtime/Library/JavascriptObject.h
@@ -110,6 +110,8 @@ namespace Js
         static JavascriptString* ToStringTagHelper(Var thisArg, ScriptContext* scriptContext, TypeId type);
 
     private:
+        template <bool tryCopy>
+        static void AssignHelper(Var fromArg, RecyclableObject* to, ScriptContext* scriptContext);
         static void AssignForGenericObjects(RecyclableObject* from, RecyclableObject* to, ScriptContext* scriptContext);
         static void AssignForProxyObjects(RecyclableObject* from, RecyclableObject* to, ScriptContext* scriptContext);
         static JavascriptArray* CreateKeysHelper(RecyclableObject* object, ScriptContext* scriptContext, BOOL enumNonEnumerable, bool includeSymbolProperties, bool includeStringProperties, bool includeSpecialProperties);
diff --git a/lib/Runtime/Types/DynamicObject.cpp b/lib/Runtime/Types/DynamicObject.cpp
@@ -760,9 +760,8 @@ namespace Js
         return reinterpret_cast<Field(Var)*>(reinterpret_cast<size_t>(this) + this->GetOffsetOfInlineSlots());
     }
 
-    bool DynamicObject::TryCopy(DynamicObject* from)
+    bool DynamicObject::IsCompatibleForCopy(DynamicObject* from) const
     {
-        // Validate that objects are compatible
         if (this->GetTypeHandler()->GetInlineSlotCapacity() != from->GetTypeHandler()->GetInlineSlotCapacity())
         {
             if (PHASE_TRACE1(ObjectCopyPhase))
@@ -805,8 +804,35 @@ namespace Js
             }
             return false;
         }
+        if (from->IsExternal())
+        {
+            if (PHASE_TRACE1(ObjectCopyPhase))
+            {
+                Output::Print(_u("ObjectCopy: Can't copy: from obj is External\n"));
+            }
+            return false;
+        }
+        if (this->GetScriptContext() != from->GetScriptContext())
+        {
+            if (PHASE_TRACE1(ObjectCopyPhase))
+            {
+                Output::Print(_u("ObjectCopy: Can't copy: from obj is from different ScriptContext\n"));
+            }
+            return false;
+        }
+
+        return true;
+    }
 
+    bool DynamicObject::TryCopy(DynamicObject* from)
+    {
+        // Validate that objects are compatible
+        if (!this->IsCompatibleForCopy(from))
+        {
+            return false;
+        }
         // Share the type
+        // Note: this will mark type as shared in case of success
         if (!from->GetDynamicType()->ShareType())
         {
             if (PHASE_TRACE1(ObjectCopyPhase))
diff --git a/lib/Runtime/Types/DynamicObject.h b/lib/Runtime/Types/DynamicObject.h
@@ -157,6 +157,8 @@ namespace Js
 #endif
 
     private:
+        bool IsCompatibleForCopy(DynamicObject* from) const;
+
         bool IsObjectHeaderInlinedTypeHandlerUnchecked() const;
     public:
         bool IsObjectHeaderInlinedTypeHandler() const;
diff --git a/test/Object/assign.js b/test/Object/assign.js
@@ -11,11 +11,14 @@ var tests = [
         body: function ()
         {
             let orig = {};
+            let sym = Symbol("c");
             orig.a = 1;
             orig.b = "asdf";
+            orig[sym] = "qwert";
             let newObj = Object.assign({}, orig);
             assert.areEqual(newObj.b, orig.b);
             assert.areEqual(newObj.a, orig.a);
+            assert.areEqual(newObj[sym], orig[sym]);
         }
     },
     {

Original file line number	Diff line number	Diff line change
`@@ -11,11 +11,14 @@ var tests = [`
`11`	`11`	`body: function ()`
`12`	`12`	`{`
`13`	`13`	`let orig = {};`
	`14`	`+ let sym = Symbol("c");`
`14`	`15`	`orig.a = 1;`
`15`	`16`	`orig.b = "asdf";`
	`17`	`+ orig[sym] = "qwert";`
`16`	`18`	`let newObj = Object.assign({}, orig);`
`17`	`19`	`assert.areEqual(newObj.b, orig.b);`
`18`	`20`	`assert.areEqual(newObj.a, orig.a);`
	`21`	`+ assert.areEqual(newObj[sym], orig[sym]);`
`19`	`22`	`}`
`20`	`23`	`},`
`21`	`24`	`{`