Bug 284132 – JSC::JSValue convert of JSConverter<IDLRecord<K, V>> crashes when converting an input WebCore::IDLRecord<WebCore::IDLUSVString, WebCore::IDLUnion<WebCore::IDLUndefined, WebCore::IDLUSVString>>

• Component: New Bugs
• Status: RESOLVED FIXED

The converter seems to mistakenly interpret a String as a number. (The String is in fact the string "0"). 

The input dictionary / union originates from <https://github.com/WebKit/WebKit/pull/37467>. It is `struct URLPatternComponentResult` defined in URLPattern.h / URLPatternResult.idl

Created attachment 473475 [details]
Crashlog

(In reply to jlee53 from comment #1)
> Created attachment 473475 [details]
> Crashlog

This is the incorrect attachment. Please ignore.

Created attachment 473476 [details]
crashlog-correct

[Chris Dumez]

Sam Weinig would be the most familiar but basically this looks like a bug in `JSConverter<IDLRecord<K, V>>` in `JSDOMRecordConvert.h`. On this line in particular:
```
            // 3. Let created be ! CreateDataProperty(result, esKey, esValue).
            bool created = result->putDirect(vm, JSC::Identifier::fromString(vm, keyValuePair.key), esValue);
```

Maybe because it doesn't deal nicely with `(undefined or USVString)` in the IDLRecord.

I think support for `undefined or USVString` is very new and I guess converting from native to JS isn't quite fully working yet.

I added a print statement where the assertion / crash happens and the property name is 0. 

Source/JavaScriptCore/runtime/JSObjectInlines.h
@@ -369,6 +369,8 @@ ALWAYS_INLINE ASCIILiteral JSObject::putDirectInternal(VM& vm, PropertyName prop
   ASSERT(!Heap::heap(value) || Heap::heap(value) == Heap::heap(this));
+  String printPropertyName = static_cast<String>(propertyName.publicName()); // add print statement
+  WTFLogAlways("JSObject::putDirectInternal printPropertyName:%s", printPropertyName.utf8().data());
   ASSERT(!parseIndex(propertyName));


stderr.txt 
JSObject::putDirectInternal printPropertyName:0 // what was printed is 0. 
ASSERTION FAILED: !parseIndex(propertyName)

[Chris Dumez]

looks like I'm wrong about the crash reason. Maybe this code should be calling `putDirectMayBeIndex()` instead of `putDirect()` since I think the property name is an index in your case.

[Radar WebKit Bug Importer]

<rdar://problem/141017497>

Pull request: https://github.com/WebKit/WebKit/pull/37522

Thanks to Chris and Sihui, this was root-cased to usage of the function putDirect in /WebCore/bindings/js/JSDOMConvertRecord.h when converting a native object to an IDL Record. 

PutDirect() assumes that the property name is not an index but in fact, it is. Conversion should use <bool createDataProperty(JSGlobalObject*, PropertyName, JSValue, bool shouldThrow)> which is what the spec states. Spec: https://webidl.spec.whatwg.org/#js-record

I confirmed that these tests are sufficient to test the change. If I remove the fix and I run the updated test, I see that it crashes with the same signature. 

Attaching crashlog

Created attachment 473491 [details]
Crash signature from updated layout test

[EWS]

Committed 287506@main (d78f8ae748e3): <https://commits.webkit.org/287506@main>

Reviewed commits have been landed. Closing PR #37522 and removing active labels.