Bug 1330699 part 13. Implement the spec provision for handling repeated keys in records by updating the existing value. r=qdot
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 15 Feb 2017 00:01:41 -0500
changeset 484467 85387004d58710aa131909a3e8806f9db1b8e654
parent 484466 289f25464d09aa9dc6d8e24d126938725e46f06f
child 484468 01dd2928a1b7b37f3ca69ea3fe70b4499f065659
push id45482
push userbmo:kmckinley@mozilla.com
push dateWed, 15 Feb 2017 09:52:37 +0000
reviewersqdot
bugs1330699
milestone54.0a1
Bug 1330699 part 13. Implement the spec provision for handling repeated keys in records by updating the existing value. r=qdot
dom/bindings/Codegen.py
dom/bindings/Record.h
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -4885,23 +4885,26 @@ def getJSToNativeConversionInfo(type, de
             # We only need holderName here to handle isExternal()
             # interfaces, which use an internal holder for the
             # conversion even when forceOwningType ends up true.
             "holderName": "tempHolder",
             "passedToJSImpl": "${passedToJSImpl}"
         })
 
         keyType = recordKeyType(recordType)
-        if recordType.keyType.isDOMString():
-            keyConversionFunction = "ConvertJSValueToString"
-        elif recordType.keyType.isUSVString():
-            keyConversionFunction = "ConvertJSValueToUSVString"
-        else:
-            assert recordType.keyType.isByteString()
+        if recordType.keyType.isByteString():
             keyConversionFunction = "ConvertJSValueToByteString"
+            hashKeyType = "nsCStringHashKey"
+        else:
+            hashKeyType = "nsStringHashKey"
+            if recordType.keyType.isDOMString():
+                keyConversionFunction = "ConvertJSValueToString"
+            else:
+                assert recordType.keyType.isUSVString()
+                keyConversionFunction = "ConvertJSValueToUSVString"
 
         templateBody = fill(
             """
             auto& recordEntries = ${recordRef}.Entries();
 
             JS::Rooted<JSObject*> recordObj(cx, &$${val}.toObject());
             JS::AutoIdVector ids(cx);
             // Keep skipping symbols until
@@ -4913,16 +4916,22 @@ def getJSToNativeConversionInfo(type, de
             if (!recordEntries.SetCapacity(ids.length(), mozilla::fallible)) {
               JS_ReportOutOfMemory(cx);
               $*{exceptionCode}
             }
             JS::Rooted<JS::Value> propNameValue(cx);
             JS::Rooted<JS::Value> temp(cx);
             JS::Rooted<jsid> curId(cx);
             JS::Rooted<JS::Value> idVal(cx);
+            // Use a hashset to keep track of ids seen, to avoid
+            // introducing nasty O(N^2) behavior scanning for them all the
+            // time.  Ideally we'd use a data structure with O(1) lookup
+            // _and_ ordering for the MozMap, but we don't have one lying
+            // around.
+            nsTHashtable<${hashKeyType}> idsSeen;
             for (size_t i = 0; i < ids.length(); ++i) {
               curId = ids[i];
 
               MOZ_ASSERT(!JSID_IS_SYMBOL(curId), "No symbols, we said!");
 
               JS::Rooted<JS::PropertyDescriptor> desc(cx);
               if (!JS_GetOwnPropertyDescriptorById(cx, recordObj, curId,
                                                    &desc)) {
@@ -4939,26 +4948,43 @@ def getJSToNativeConversionInfo(type, de
               if (!${keyConversionFunction}(cx, idVal, propName)) {
                 $*{exceptionCode}
               }
 
               if (!JS_GetPropertyById(cx, recordObj, curId, &temp)) {
                 $*{exceptionCode}
               }
 
-              // Safe to do an infallible append here, because we did a
-              // SetCapacity above to the right capacity.
-              ${typeName}::EntryType* entry = recordEntries.AppendElement();
+              ${typeName}::EntryType* entry;
+              if (idsSeen.Contains(propName)) {
+                // Find the existing entry.
+                auto idx = recordEntries.IndexOf(propName);
+                MOZ_ASSERT(idx != recordEntries.NoIndex,
+                           "Why is it not found?");
+                // Now blow it away to make it look like it was just added
+                // to the array, because it's not obvious that it's
+                // safe to write to its already-initialized mValue via our
+                // normal codegen conversions.  For example, the value
+                // could be a union and this would change its type, but
+                // codegen assumes we won't do that.
+                entry = recordEntries.ReconstructElementAt(idx);
+              } else {
+                // Safe to do an infallible append here, because we did a
+                // SetCapacity above to the right capacity.
+                entry = recordEntries.AppendElement();
+                idsSeen.PutEntry(propName);
+              }
               entry->mKey = propName;
               ${valueType}& slot = entry->mValue;
               $*{valueConversion}
             }
             """,
             exceptionCode=exceptionCode,
             recordRef=recordRef,
+            hashKeyType=hashKeyType,
             keyType=keyType,
             keyConversionFunction=keyConversionFunction,
             typeName=typeName,
             valueType=valueInfo.declType.define(),
             valueConversion=valueConversion)
 
         templateBody = wrapObjectTemplate(templateBody, type,
                                           "${declName}.SetNull();\n",
--- a/dom/bindings/Record.h
+++ b/dom/bindings/Record.h
@@ -72,9 +72,20 @@ public:
 
 private:
   nsTArray<EntryType> mEntries;
 };
 
 } // namespace dom
 } // namespace mozilla
 
+template<typename K, typename V>
+class nsDefaultComparator<mozilla::dom::binding_detail::RecordEntry<K, V>, K>
+{
+public:
+  bool Equals(const mozilla::dom::binding_detail::RecordEntry<K, V>& aEntry,
+              const K& aKey) const
+  {
+    return aEntry.mKey == aKey;
+  }
+};
+
 #endif // mozilla_dom_Record_h