Address CR comments

chakra-core · Oct 31, 2017 · 274a1e9 · 274a1e9
1 parent ba073f6
commit 274a1e9
Show file tree

Hide file tree

Showing 11 changed files with 128 additions and 60 deletions.
diff --git a/lib/Runtime/Base/Constants.h b/lib/Runtime/Base/Constants.h
@@ -155,15 +155,14 @@ namespace Js
         static const  char16 StringReplace[];
         static const  char16 StringMatch[];
 
-        static const size_t AnonymousFunctionLength = _countof(_u("Anonymous function")) - 1;
-        static const size_t AnonymousLength         = _countof(_u("anonymous")) - 1;
-        static const size_t AnonymousClassLength    = _countof(_u("Anonymous class")) - 1;
-        static const size_t FunctionCodeLength      = _countof(_u("Function code")) - 1;
-        static const size_t GlobalFunctionLength    = _countof(_u("glo")) - 1;
-        static const size_t GlobalCodeLength        = _countof(_u("Global code")) - 1;
-        static const size_t EvalCodeLength          = _countof(_u("eval code")) - 1;
-        static const size_t UnknownScriptCodeLength = _countof(_u("Unknown script code")) - 1;
-
+        static const charcount_t AnonymousFunctionLength = _countof(_u("Anonymous function")) - 1;
+        static const charcount_t AnonymousLength         = _countof(_u("anonymous")) - 1;
+        static const charcount_t AnonymousClassLength    = _countof(_u("Anonymous class")) - 1;
+        static const charcount_t FunctionCodeLength      = _countof(_u("Function code")) - 1;
+        static const charcount_t GlobalFunctionLength    = _countof(_u("glo")) - 1;
+        static const charcount_t GlobalCodeLength        = _countof(_u("Global code")) - 1;
+        static const charcount_t EvalCodeLength          = _countof(_u("eval code")) - 1;
+        static const charcount_t UnknownScriptCodeLength = _countof(_u("Unknown script code")) - 1;
         static const charcount_t NullStringLength = _countof(_u("Null")) - 1;
         static const charcount_t TrueStringLength = _countof(_u("True")) - 1;
         static const charcount_t FalseStringLength = _countof(_u("False")) - 1;

diff --git a/lib/Runtime/Library/JSON.cpp b/lib/Runtime/Library/JSON.cpp
@@ -69,7 +69,7 @@ namespace JSON
             {
                 // Try to reconstruct object based on the data collected during stringify
                 // In case of semantically important gap, this call may fail and we will need to do real parse
-                result = lazyString->Parse();
+                result = lazyString->TryParse();
             }
             if (result == nullptr)
             {

diff --git a/lib/Runtime/Library/JSONStringBuilder.cpp b/lib/Runtime/Library/JSONStringBuilder.cpp
@@ -136,7 +136,7 @@ JSONStringBuilder::AppendObjectString(_In_ JSONObject* valueList)
     }
 
     bool isFirstMember = true;
-    FOREACH_DLISTCOUNTED_ENTRY(JSONProperty, Recycler, entry, valueList)
+    FOREACH_DLISTCOUNTED_ENTRY(JSONObjectProperty, Recycler, entry, valueList)
     {
         if (!isFirstMember)
         {
@@ -158,7 +158,7 @@ JSONStringBuilder::AppendObjectString(_In_ JSONObject* valueList)
             this->AppendCharacter(_u(' '));
         }
 
-        this->AppendJSONPropertyString(&entry);
+        this->AppendJSONPropertyString(&entry.propertyValue);
 
         isFirstMember = false;
     }

diff --git a/lib/Runtime/Library/JSONStringifier.cpp b/lib/Runtime/Library/JSONStringifier.cpp
@@ -412,11 +412,12 @@ JSONStringifier::ReadObjectElement(
     _In_ JSONObject* jsonObject,
     _In_ JSONObjectStack* objectStack)
 {
-    JSONProperty element;
-    this->ReadProperty(propertyName, obj, &element, nullptr, propertyRecord, objectStack);
+    JSONObjectProperty prop;
+    prop.propertyName = propertyName;
+    this->ReadProperty(propertyName, obj, &prop.propertyValue, nullptr, propertyRecord, objectStack);
 
     // Undefined result is not concatenated
-    if (element.type != JSONContentType::Undefined)
+    if (prop.propertyValue.type != JSONContentType::Undefined)
     {
         // Increase length for the name of the property
         this->totalStringLength = UInt32Math::Add(this->totalStringLength, CalculateStringElementLength(propertyName));
@@ -427,7 +428,7 @@ JSONStringifier::ReadObjectElement(
             // If gap is specified, a space is appended
             UInt32Math::Inc(this->totalStringLength);
         }
-        jsonObject->Append(element);
+        jsonObject->Append(prop);
     }
 }
 
@@ -537,6 +538,7 @@ JSONStringifier::ReadObject(_In_ RecyclableObject* obj, _In_ JSONObjectStack* ob
         JavascriptStaticEnumerator enumerator;
         if (obj->GetEnumerator(&enumerator, EnumeratorFlags::SnapShotSemantics | EnumeratorFlags::EphemeralReference, this->scriptContext))
         {
+            enumerator.GetInitialPropertyCount();
             JavascriptString* propertyName = nullptr;
             PropertyId nextKey = Constants::NoProperty;
             while ((propertyName = enumerator.MoveAndGetNext(nextKey)) != nullptr)
@@ -730,7 +732,13 @@ RecheckValue(_In_ Var value, _Out_ RecyclableObject** valueObj, _Out_ bool* isOb
 }
 
 void
-JSONStringifier::ReadProperty(_In_ JavascriptString* key, _In_opt_ RecyclableObject* holder, _Out_ JSONProperty* prop, _In_opt_ Var value, _In_opt_ const PropertyRecord* propertyRecord, _In_ JSONObjectStack* objectStack)
+JSONStringifier::ReadProperty(
+    _In_ JavascriptString* key,
+    _In_opt_ RecyclableObject* holder,
+    _Out_ JSONProperty* prop,
+    _In_opt_ Var value,
+    _In_opt_ const PropertyRecord* propertyRecord,
+    _In_ JSONObjectStack* objectStack)
 {
     PROBE_STACK(this->scriptContext, Constants::MinStackDefault);
     if (propertyRecord == nullptr)
@@ -739,13 +747,11 @@ JSONStringifier::ReadProperty(_In_ JavascriptString* key, _In_opt_ RecyclableObj
     }
     if (value == nullptr)
     {
-        Assert(holder != nullptr);
+        // If we don't have a value, we must have an object from which we can read it
+        AnalysisAssert(holder != nullptr);
         value = this->ReadValue(key, propertyRecord, holder);
     }
 
-    prop->propertyRecord = propertyRecord;
-    prop->propertyName = key;
-
     // Save these to avoid having to recheck conditions unless value is modified by ToJSON or a replacer function
     RecyclableObject* valueObj = nullptr;
     bool isObject = false;

diff --git a/lib/Runtime/Library/JSONStringifier.h b/lib/Runtime/Library/JSONStringifier.h
@@ -51,7 +51,6 @@ class JSONStringifier
     charcount_t gapLength;
     char16* gap;
 
-    // TODO: refactor
     _Ret_notnull_ const PropertyRecord* GetPropertyRecord(_In_ JavascriptString* string);
     Var TryConvertPrimitiveObject(_In_ RecyclableObject* value);
     Var ToJSON(_In_ JavascriptString* key, _In_ RecyclableObject* valueObject);
@@ -100,6 +99,8 @@ class JSONStringifier
     charcount_t GetGapLength() const { return this->gapLength; }
     bool HasReplacerFunction() const { return this->replacerFunction != nullptr; }
 
+    bool HasComplexGap() const;
+
     static LazyJSONString* Stringify(_In_ ScriptContext* scriptContext, _In_ Var value, _In_opt_ Var replacer, _In_opt_ Var space);
 
 }; // class JSONStringifier

diff --git a/lib/Runtime/Library/LazyJSONString.cpp b/lib/Runtime/Library/LazyJSONString.cpp
@@ -18,48 +18,82 @@ LazyJSONString::LazyJSONString(_In_ JSONProperty* jsonContent, charcount_t lengt
     SetLength(length);
 }
 
+bool
+LazyJSONString::HasComplexGap() const
+{
+    for (charcount_t i = 0; i < this->gapLength; ++i)
+    {
+        switch (this->gap[i])
+        {
+        // This is not exhaustive, just a useful subset of semantics preserving characters
+        case _u(' '):
+        case _u('\t'):
+        case _u('\n'):
+            continue;
+        default:
+            return true;
+        }
+    }
+    return false;
+}
+
 DynamicObject*
-LazyJSONString::ParseObject(_In_ JSONObject* valueList) const
+LazyJSONString::ReconstructObject(_In_ JSONObject* valueList) const
 {
     const uint elementCount = valueList->Count();
+
+    // This is just a heuristic, so overflow doesn't matter
     PropertyIndex requestedInlineSlotCapacity = static_cast<PropertyIndex>(elementCount);
-    DynamicObject* obj = this->GetLibrary()->CreateObject(true, requestedInlineSlotCapacity);
-    FOREACH_DLISTCOUNTED_ENTRY(JSONProperty, Recycler, entry, valueList)
+
+    DynamicObject* obj = this->GetLibrary()->CreateObject(
+        true, // allowObjectHeaderInlining
+        requestedInlineSlotCapacity);
+
+    FOREACH_DLISTCOUNTED_ENTRY(JSONObjectProperty, Recycler, entry, valueList)
     {
-        Var propertyValue = Parse(&entry);
+        Var propertyValue = ReconstructVar(&entry.propertyValue);
         PropertyValueInfo info;
         PropertyString* propertyString = PropertyString::TryFromVar(entry.propertyName);
         if (!propertyString || !propertyString->TrySetPropertyFromCache(obj, propertyValue, this->GetScriptContext(), PropertyOperation_None, &info))
         {
-            JavascriptOperators::SetProperty(obj, obj, entry.propertyRecord->GetPropertyId(), propertyValue, &info, this->GetScriptContext());
+            const PropertyRecord* propertyRecord = nullptr;
+            if (propertyString)
+            {
+                propertyRecord = propertyString->GetPropertyRecord();
+            }
+            else
+            {
+                this->GetScriptContext()->GetOrAddPropertyRecord(entry.propertyName, &propertyRecord);
+            }
+            JavascriptOperators::SetProperty(obj, obj, propertyRecord->GetPropertyId(), propertyValue, &info, this->GetScriptContext());
         }
     }
     NEXT_DLISTCOUNTED_ENTRY;
     return obj;
 }
 
 JavascriptArray*
-LazyJSONString::ParseArray(_In_ JSONArray* jsonArray) const
+LazyJSONString::ReconstructArray(_In_ JSONArray* jsonArray) const
 {
     const uint32 length = jsonArray->length;
     JavascriptArray* arr = this->GetLibrary()->CreateArrayLiteral(length);
     JSONProperty* prop = jsonArray->arr;
     for (uint i = 0; i < length; ++i)
     {
-        Var element = Parse(&prop[i]);
+        Var element = ReconstructVar(&prop[i]);
         BOOL result = arr->SetItem(i, element, PropertyOperation_None);
         Assert(result); // Setting item in an array we just allocated should always succeed
     }
     return arr;
 }
 
 Var
-LazyJSONString::Parse(_In_ JSONProperty* prop) const
+LazyJSONString::ReconstructVar(_In_ JSONProperty* prop) const
 {
     switch (prop->type)
     {
     case JSONContentType::Array:
-        return ParseArray(prop->arr);
+        return ReconstructArray(prop->arr);
     case JSONContentType::Null:
         return this->GetLibrary()->GetNull();
     case JSONContentType::True:
@@ -71,37 +105,43 @@ LazyJSONString::Parse(_In_ JSONProperty* prop) const
     case JSONContentType::String:
         return prop->stringValue;
     case JSONContentType::Object:
-        return ParseObject(prop->obj);
+        return ReconstructObject(prop->obj);
     default:
         Assume(UNREACHED);
         return nullptr;
     }
 }
 
 Var
-LazyJSONString::Parse() const
+LazyJSONString::TryParse() const
 {
-    // If the gap is a non-space character, then parsing will be non-trivial transformation
-    for (charcount_t i = 0; i < this->gapLength; ++i)
+    // If we have thrown away our metadata, we won't be able to Parse
+    if (this->jsonContent == nullptr)
     {
-        switch (this->gap[i])
-        {
-        // This is not exhaustive, just a useful subset of semantics preserving characters
-        case _u(' '):
-        case _u('\t'):
-        case _u('\n'):
-            continue;
-        default:
-            return nullptr;
-        }
+        return nullptr;
     }
-    return Parse(this->jsonContent);
+
+    // If the gap is complex, this means that parse will be a non-trivial transformation,
+    // so fall back to the real parse
+    if (this->HasComplexGap())
+    {
+        return nullptr;
+    }
+
+    Var result = ReconstructVar(this->jsonContent);
+
+    return result;
 }
 
 const char16*
 LazyJSONString::GetSz()
 {
-    const charcount_t allocSize = SafeSzSize();
+    if (this->IsFinalized())
+    {
+        return this->UnsafeGetBuffer();
+    }
+
+    const charcount_t allocSize = this->SafeSzSize();
 
     Recycler* recycler = GetScriptContext()->GetRecycler();
     char16* target = RecyclerNewArrayLeaf(recycler, char16, allocSize);
@@ -116,7 +156,14 @@ LazyJSONString::GetSz()
 
     builder.Build();
 
-    SetBuffer(target);
+    this->SetBuffer(target);
+
+    if (this->HasComplexGap())
+    {
+        // If we have a complex gap, there is no reason to keep content around after flattening
+        this->jsonContent = nullptr;
+    }
+
     return target;
 }
 

diff --git a/lib/Runtime/Library/LazyJSONString.h b/lib/Runtime/Library/LazyJSONString.h
@@ -7,7 +7,7 @@
 
 namespace Js
 {
-
+struct JSONObjectProperty;
 struct JSONProperty;
 struct JSONArray;
 
@@ -23,30 +23,40 @@ enum class JSONContentType : uint8
     String
 };
 
-typedef DListCounted<JSONProperty, Recycler> JSONObject;
+typedef DListCounted<JSONObjectProperty, Recycler> JSONObject;
+
 
 struct JSONNumberData
 {
     Field(Var) value;
     Field(JavascriptString*) string;
 };
 
+
 struct JSONProperty
 {
     Field(JSONContentType) type;
-    Field(const PropertyRecord*) propertyRecord;
-    Field(JavascriptString*) propertyName;
     union
     {
         Field(JSONNumberData) numericValue;
         Field(JavascriptString*) stringValue;
         Field(JSONObject*) obj;
         Field(JSONArray*) arr;
     };
+};
 
-    JSONProperty()
+struct JSONObjectProperty
+{
+    Field(JavascriptString*) propertyName;
+    Field(JSONProperty) propertyValue;
+
+    JSONObjectProperty()
     {
-        memset(this, 0, sizeof(JSONProperty));
+        memset(this, 0, sizeof(JSONObjectProperty));
+    }
+    JSONObjectProperty(const JSONObjectProperty& other)
+    {
+        memcpy_s(this, sizeof(JSONObjectProperty), &other, sizeof(JSONObjectProperty));
     }
 };
 
@@ -63,16 +73,19 @@ class LazyJSONString : JavascriptString
     Field(JSONProperty*) jsonContent;
     Field(const char16*) gap;
 
-    DynamicObject* ParseObject(_In_ JSONObject* valueList) const;
-    JavascriptArray* ParseArray(_In_ JSONArray* valueArray) const;
-    Var Parse(_In_ JSONProperty* content) const;
+    DynamicObject* ReconstructObject(_In_ JSONObject* valueList) const;
+    JavascriptArray* ReconstructArray(_In_ JSONArray* valueArray) const;
+    Var ReconstructVar(_In_ JSONProperty* content) const;
 
 protected:
     DEFINE_VTABLE_CTOR(LazyJSONString, JavascriptString);
 
 public:
     LazyJSONString(_In_ JSONProperty* content, charcount_t length, _In_opt_ const char16* gap, charcount_t gapLength, _In_ StaticType* type);
-    Var Parse() const;
+    Var TryParse() const;
+
+    // Tells if the string has a gap with characters that might impact JSON.parse
+    bool HasComplexGap() const;
 
     const char16* GetSz() override sealed;
 

diff --git a/lib/Runtime/Types/DynamicObjectPropertyEnumerator.h b/lib/Runtime/Types/DynamicObjectPropertyEnumerator.h
@@ -45,6 +45,7 @@ namespace Js
         bool GetSnapShotSemantics() const;
         bool GetUseCache() const;
         ScriptContext * GetScriptContext() const { return scriptContext; }
+        BigPropertyIndex GetInitialPropertyCount() const { return this->initialPropertyCount; }
 
         bool Initialize(DynamicObject * object, EnumeratorFlags flags, ScriptContext * requestContext, ForInCache * forInCache);
         bool IsNullEnumerator() const;

diff --git a/lib/Runtime/Types/JavascriptStaticEnumerator.h b/lib/Runtime/Types/JavascriptStaticEnumerator.h
@@ -39,6 +39,7 @@ namespace Js
         void Reset();
         uint32 GetCurrentItemIndex();
         JavascriptString * MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr);
+        BigPropertyIndex GetInitialPropertyCount() const { return this->propertyEnumerator.GetInitialPropertyCount(); }
 
         static uint32 GetOffsetOfCurrentEnumerator() { return offsetof(JavascriptStaticEnumerator, currentEnumerator); }
         static uint32 GetOffsetOfPrefixEnumerator() { return offsetof(JavascriptStaticEnumerator, prefixEnumerator); }