Should not use variable-length-array (VLA)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

...

[Yusuke Suzuki]

<rdar://problem/59175669>

[Yusuke Suzuki]

Created attachment 393467 [details]
Patch

Add tests later

[Yusuke Suzuki]

Created attachment 393468 [details]
Patch

Add tests later

[Yusuke Suzuki]

Created attachment 393574 [details]
Patch

[Yusuke Suzuki]

Created attachment 393579 [details]
Patch

[Mark Lam]

Comment on attachment 393579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393579&action=review

r=me with one concern about JSAPIValueWrapper on !CPU(ADDRESS64) targets.

> Source/JavaScriptCore/API/MarkedJSValueRefArray.cpp:60
> +        visitor.appendUnbarriered(jsCell); // We should mark the wrapper itself to keep JSValueRef live.

I didn't quite understand what this comment meant in the first place until I read JSC::JSValue toJS(JSC::JSGlobalObject* globalObject, JSValueRef v).

From my reading of the code, I see that the jsCell here may be a JSAPIValueWrapper which is itself a JSCell.  Hence, we should visit the JSAPIValueWrapper itself, and that means appending the jsCell here should do the right thing regardless of whether the jsCell is a JSAPIValueWrapper or some other JSCell.

That said, JSAPIValueWrapper does not appear to have a visitChildren() function.  Hence, I don't see how its m_value will get visited.  Am I missing something?

If the above is accurate, I think we don't need this comment.  We do need to look into how the JSAPIValueWrapper::m_value gets visited, and add a visitChildren() there for the !CPU(ADDRESS64) case.  Should probably do that in a separate bug.

> Source/JavaScriptCore/heap/Heap.cpp:3011
> +void Heap::addmarkedJSValueRefArray(MarkedJSValueRefArray* array)

typo: /addmarkedJSValueRefArray/addMarkedJSValueRefArray/

[Yusuke Suzuki]

Comment on attachment 393579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393579&action=review

>> Source/JavaScriptCore/API/MarkedJSValueRefArray.cpp:60
>> +        visitor.appendUnbarriered(jsCell); // We should mark the wrapper itself to keep JSValueRef live.
> 
> I didn't quite understand what this comment meant in the first place until I read JSC::JSValue toJS(JSC::JSGlobalObject* globalObject, JSValueRef v).
> 
> From my reading of the code, I see that the jsCell here may be a JSAPIValueWrapper which is itself a JSCell.  Hence, we should visit the JSAPIValueWrapper itself, and that means appending the jsCell here should do the right thing regardless of whether the jsCell is a JSAPIValueWrapper or some other JSCell.
> 
> That said, JSAPIValueWrapper does not appear to have a visitChildren() function.  Hence, I don't see how its m_value will get visited.  Am I missing something?
> 
> If the above is accurate, I think we don't need this comment.  We do need to look into how the JSAPIValueWrapper::m_value gets visited, and add a visitChildren() there for the !CPU(ADDRESS64) case.  Should probably do that in a separate bug.

JSAPIValueWrapper is only used for non-cell values (undefined, number etc.). So this means that the wrapped value itself does not require visitChildren.
This wrapper exists only because we need to keep sizeof(JSValueRef) 32bit in 32bit architecture. Since our non-cell values require 64bit value representation, we decided to use JSAPIValueWrapper as a work-around for this problem.

[Yusuke Suzuki]

Created attachment 393610 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Comment on attachment 393579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393579&action=review

>> Source/JavaScriptCore/heap/Heap.cpp:3011
>> +void Heap::addmarkedJSValueRefArray(MarkedJSValueRefArray* array)
> 
> typo: /addmarkedJSValueRefArray/addMarkedJSValueRefArray/

Fixed.

[Yusuke Suzuki]

Committed r258478: <https://trac.webkit.org/changeset/258478>

[Darin Adler]

Comment on attachment 393610 [details]
Patch

WTF::Vector with inline capacity was originally invented to use in local variables to create a safer alternative to variable length arrays that spills out onto the heap when needed, but has similar efficiency when the array is smaller than a fixed predicted size. I’m maybe a tiny bit surprised that we didn’t switch to use them in any of these cases.

[Darin Adler]

Comment on attachment 393610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393610&action=review

> Source/JavaScriptCore/API/JSContext.mm:237
> +            [arguments setObject:[JSValue valueWithJSValueRef:entry->arguments[i] inContext:context] atIndexedSubscript:i];

This should just be calling addObject: rather than setObject:atIndexedSubscript: unless I am missing something.

What’s the benefit of using setObject:atIndexedSubscript: here?

> Source/JavaScriptCore/API/MarkedJSValueRefArray.h:68
> +    unsigned m_size;
> +    JSValueRef m_inlineBuffer[inlineCapacity] { };
> +    BufferUniquePtr m_buffer;

Frustrating that this is basically re-implementing Vector with inline capacity. Is there some way to take advantage of Vector or are there just not enough template arguments?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:396
> +    Vector<GCGLint> params(numValues);

Could use inline capacity here if we want to avoid allocating on the heap when there are small numbers of arguments.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:426
> +    Vector<GCGLint> params(numValues);

Ditto.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:170
> +    Vector<AudioObjectID> deviceIDs(deviceCount);

Ditto.

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:65
> +    Vector<CGDirectDisplayID> activeDisplays(displayCount);

Ditto.

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:297
> +    Vector<CGPoint> locations(touchCount);

Ditto.

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:330
> +    Vector<CGPoint> locations(touchCount);

Ditto.

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:348
> +    Vector<CGPoint> startLocations(touchCount);
> +    Vector<CGPoint> nextLocations(touchCount);

Ditto.