Bug 197535 – [GLib] Expose typed arrays in the public API

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Adrian Perez]

Typed arrays are necessary to pass chunks of data i.e. a byte buffer
as Uint8Array between the C/C++ and JavaScript worlds. Using strings
is not feasible for many uses because where the representation of
the data in memory must be the same, bit-by-bit, on both sides.

[Adrian Perez]

Created attachment 368836 [details]
WIP Patch v1


Here goes an initial version of the patch with the proposed API
additions. It still needs API tests and documentation, at least.

[EWS Watchlist]

Attachment 368836 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1475:  jsc_value_typed_array__from_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1481:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1488:  jsc_value_is_typed_array is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1502:  jsc_value_typed_array_get_length is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1520:  jsc_value_typed_array_get_contents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCValue.h"
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

The following API are missing documentation:
	JSCTypedArrayType
	jsc_value_is_typed_array
	jsc_value_typed_array_get_contents
	jsc_value_typed_array_get_length
	jsc_value_typed_array_get_type
	jsc_value_typed_array_new
	jsc_value_typed_array_new_from_data

[EWS Watchlist]

Comment on attachment 368836 [details]
WIP Patch v1

Attachment 368836 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12069414

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html

[EWS Watchlist]

Created attachment 368852 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Adrian Perez]

(In reply to Michael Catanzaro from comment #3)
> The following API are missing documentation:
> 	JSCTypedArrayType
> 	jsc_value_is_typed_array
> 	jsc_value_typed_array_get_contents
> 	jsc_value_typed_array_get_length
> 	jsc_value_typed_array_get_type
> 	jsc_value_typed_array_new
> 	jsc_value_typed_array_new_from_data

Of course, I am perfectly aware; I just wanted to share an initial
WIP version of the patch to get the discussion started about the
new API and avoid writing documentation for functions which might
get changed or removed before being rubber-stamped ;-)

Now, regarding the API, reviewers may be wondering why GBytes,
GArray and/or GByteArray are *not* used. The reason is that their
semantics do not match typed arrays well: 

 - Typed arrays are fixed size, which discards usage of GArray
   and GByteArray. These GLib types allow the backing memory
   buffer to be resized e.g. via g_array_append_val() or
   g_byte_array_set_size().

 - While GBytes seems like a good match on the surface, the
   expectation is that the contents of the backing memory buffer
   are immutable. This does not hold true for a JS typed array,
   where individual elements can be mutated.

Because of the reasons above, I settled on having _new_from_data()
take a pointer+size pair, and an optional GDestroyNotify. On the
other end of the spectrum, a raw pointer and the size of the backing
buffer in bytes can be obtained directly instead of having those
wrapped into some helper object — like for GMappedFile, which I
presume has the same rationale behind its _get_length() and
_get_contents() methods.

Last but not least, it will be important to note that the pointer
returned by _get_contents() may become invalid between JSC API
calls because JSC might change the underlying array buffer under
the hood. This will be something to stress in the API reference
documentation.

[Michael Catanzaro]

Comment on attachment 368836 [details]
WIP Patch v1

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

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1520
> +char* jsc_value_typed_array_get_contents(JSCValue* value, gsize* len)

Use guchar* to indicate that it's not a string.

(char*/gchar* is guaranteed to be a UTF-8 string in GObject APIs.)

[Adrian Perez]

(In reply to Michael Catanzaro from comment #7)
> Comment on attachment 368836 [details]
> WIP Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368836&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1520
> > +char* jsc_value_typed_array_get_contents(JSCValue* value, gsize* len)
> 
> Use guchar* to indicate that it's not a string.
> 
> (char*/gchar* is guaranteed to be a UTF-8 string in GObject APIs.)

Ah, this is a good point, let's change the types to guchar* =)

I have been testing out the new API myself and in general it works
nicely. The only modification I think that would be good to have
is adding an “user_data” parameter to the function that creates a
new typed array from an existing data buffer, like the following:

  JSCValue*
  jsc_value_new_typed_array_from_data (JSCContext       *context,
                                       JSCTypedArrayType type,
                                       gpointer          data,
                                       gsize             data_bytes,
                                       GDestroyNotify    destroy_notify,
                                       gpointer          user_data);

With this modification, what would be passed to the “destroy_notify”
callback would be “user_data”, instead of “data”. This is desirable
because the “data” buffer might belong to some other structure — note
that passing the same value for “data” and “user_data” would still
achieve the goal of passing “data” to the “destroy_notify” callback.

If you want a concrete example where “user_data” actually needs to be
different, consider this example (sans error checking, to keep it brief):

  JSCValue*
  js_map_file (const char* file_path) {
      GMappedFile *f = g_mapped_file_new (file_path, TRUE, NULL);
      return jsc_value_new_typed_array_from_data (jsc_context_get_current (),
                                                  JSC_TYPED_ARRAY_UINT8,
                                                  g_mapped_file_get_contents (f),
                                                  g_mapped_file_get_length (f),
                                                  (GDestroyNotify) g_mapped_file_unref,
                                                  f);
  }

Note how we do *not* want the “destroy_notify” to be applied to the
data buffer pointer; in this case we want to apply “g_mapped_file_unref”
on the GMappedFile itself instead.

I will do this change, which is important for many use cases, then add
tests/documentation, and post an updated patch :-)

[Adrian Perez]

Created attachment 369379 [details]
WIP Patch v2


This version of the patch has the following changes from v1:

- New API functions and types now have documentation.

- jsc_typed_array_new_from_data() now can take an “user_data”, which is
  what gets passed to the “destroy_notify” callback. This provides some
  needed flexibility to allow wrapping something more complex than a raw
  memory buffer as a typed array (check the documentation, which uses a
  GMappedFile to exemplify this).

- jsc_typed_array_get_contents() now returns “gpointer”. I think this is
  better than the “guchar*” suggestion by Michael, because it forces the
  users of the function to think and deliberately choose which pointer
  type they need to cast the returned value to. This is good because
  typed array contents can be elements of different types, and forcing
  users to make an explicit choice avoids a potential footgun.

The patch still needs adding unit tests for the new API, which is the
next thing I will be doing. Any feedback that reviewers can provide in
the meantimea bout the patch will be valuable (thanks!)

[EWS Watchlist]

Attachment 369379 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1493:  jsc_value__typed_array is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1554:  jsc_value__typed_array_from_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1584:  jsc_value_is_typed_array is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1608:  jsc_value_typed_array_get_length is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1650:  jsc_value_typed_array_get_contents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCValue.h"
Total errors found: 5 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 369379 [details]
WIP Patch v2

Attachment 369379 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12141767

New failing tests:
http/tests/css/filters-on-iframes.html

[EWS Watchlist]

Created attachment 369480 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Carlos Garcia Campos]

Comment on attachment 369379 [details]
WIP Patch v2

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

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1484
> + * @len: number of elements in the array

length

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1554
> +JSCValue* jsc_value_new_typed_array_from_data(JSCContext* context, JSCTypedArrayType type, gpointer data, gsize dataBytes, gpointer userData, GDestroyNotify destroyNotify)

I wonder if we should expose ArrayBuffer (we probably need to do it anyway) and then leave the destruction thing to array buffer itself, and expose only new_typed_array_from_array_buffer (not this data one).

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1557
> +

Is it possible to create a typed array with null data? and 0 size then?

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1560
> +        deallocatorContext = static_cast<TypedArrayDeallocatorContext*>(fastMalloc(sizeof(TypedArrayDeallocatorContext)));

You can use WEBKIT_DEFINE_ASYNC_DATA_STRUCT macro.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1602
> + * Obtains the size in bytes of the memory region which holds the contents of a typed array.

This is confusing, I would expect the lenght to be the number of elements in the array, not the data size in bytes.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1604
> + * Returns: size in bytes or zero if @value is not a typed array

Wouldn't we return 0 also if the array is empty?

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1619
> +    size_t len = JSObjectGetTypedArrayLength(jsContext, jsObject, &exception);

This is indeed the amount of elements in array.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1627
> + * jsc_value_typed_array_get_contents:

If we expose array buffer, this would be part of the array buffer api. I think it's would be less confusing, because typed array woudl always be about elements and array buffer about the actual data and size in bytes. Note also that is possible to provide an offset.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1629
> + * @len: (nullable): if not `NULL`, location of a variable where to store the length in bytes

length. %NULL

[Alejandro G. Castro]

I've been checking the patch because we found some crashes in a demo using this API. The memory we are passing to WebKit can not be reserved outside with malloc if WebKit is using gigacage. I don't know much about gigacage but it seems a solution would be to reserve memory inside gigacage and copy the data passed to the jsc_value_new_typed_array_from_data API. But it is true that if the whole point of the API is to adopt the memory we have to change the name to add the copy somewhere in the name to make it clear.

Also we should add an assert or some way to control this situation and fail nicely with some informative message to the developer.

I would not even propose a solution in case gigacage is not activated to make sure no one tries to do it because it performs better.

I hope it helps!

[Adrian Perez]

(In reply to Alejandro G. Castro from comment #14)
> I've been checking the patch because we found some crashes in a demo using
> this API. The memory we are passing to WebKit can not be reserved outside
> with malloc if WebKit is using gigacage. I don't know much about gigacage
> but it seems a solution would be to reserve memory inside gigacage and copy
> the data passed to the jsc_value_new_typed_array_from_data API. But it is
> true that if the whole point of the API is to adopt the memory we have to
> change the name to add the copy somewhere in the name to make it clear.
> 
> Also we should add an assert or some way to control this situation and fail
> nicely with some informative message to the developer.
> 
> I would not even propose a solution in case gigacage is not activated to
> make sure no one tries to do it because it performs better.
> 
> I hope it helps!

Thanks Alex, that indeed is useful information and makes complete sense.
I completely agree that we do not want to give any hint that would suggest
disabling gigacages as an option because it's an important safety feature.

I am going to restart working on this now, and I will try to come up with
a better API that exposes ArrayBuffer as suggested by Carlos (which can be
useful in many cases, like using Blob.arrayBuffer when a C function gets
a Blob instance passed to it) and also some way of asking JSC itself to
allocate a chunk of memory so it can be inside a suitable gigacage, and
returning that via the JSC API so client code can put data in buffers that
JSC can then use as ArrayBuffer backing. That will be probably returning
GBytes instances that wrap the memory allocated by JSC and have a custom
deallocation callback that takes care of properly freeing memory.

[Adrian Perez]

I am splitting the part of exposing ArrayBuffer in the public API
to bug #237088, and then adding functions for typed arrays will
build upon that.

[Adrian Perez]

Created attachment 454040 [details]
WIP Patch v3


This version of the patch builds upon the one from bug #237088,
and I think it should address all the concerns that Carlos Garcia
had.

As for the allocation issue mentionex by Alex, it seems that creating
an ArrayBuffer first, and then using it as the memory area backing
a typed array works as expected even with non-gigacage allocations
(which IMO makes sense).

I will be uploading a version of the patch with added API tests, but
if anybody has feedback in the meantime, it would be appreciated.

[Adrian Perez]

Created attachment 454112 [details]
Patch


Now with API tests. I am quite satisfied with how the API turned out,
and it has definitely been a good idea to add support for ArrayBuffer
as suggested ;-)

[Adrian Perez]

Of course the EWS will be red because we need to land the patch from
bug #237088 first.

[Carlos Garcia Campos]

Comment on attachment 454112 [details]
Patch

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

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1677
> +    case kJSTypedArrayTypeNone:
> +    case kJSTypedArrayTypeArrayBuffer: return JSC_TYPED_ARRAY_NONE;

I think this is easier to read if returns are in new lines. At first I thought you had forgotten to add a return for the None, then I realized we return NONE also for ArrayBuffer. Again, I think the C API is confusing, we should either add a comment explaining what happens with the ArrayBuffer type, or not use JSValueGetTypedArrayType and simply check toJSTypedArrayType(object->classInfo(vm)->typedArrayStorageType) directly

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1705
> + * The @type must *not* be #JSC_TYPED_ARRAY_NONE.

#JSC_TYPED_ARRAY_NONE -> %JSC_TYPED_ARRAY_NONE

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1714
> +    g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr);
> +

g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr);

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1742
> +JSCValue* jsc_value_new_typed_array_with_buffer(JSCValue* arrayBuffer, JSCTypedArrayType type)

I think we want to expose the constructor taking an offset too. The JS API allows to pass only a buffer, buffer + offset and buffer + offset + length. Adding 3 constructors would be too much so we can probably leave this and add with_offset taking both offset and length, but length being optional, or simply add offset and length here, making both optional.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1783
> +    return type != kJSTypedArrayTypeNone && type != kJSTypedArrayTypeArrayBuffer;

This is confusing too.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1850
> +    auto* ptr = JSObjectGetTypedArrayBytesPtr(jsContext, jsObject, &exception);

ptr -> bytes, data or something like that

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1860
> +        const auto c = JSObjectGetTypedArrayLength(jsContext, jsObject, &exception);

c -> length, size or something like that

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1870
> + * jsc_value_typed_array_get_count:

I would use length instead of count

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1899
> + * Obtain the %ArrayBuffer for the memory region of the typed array elements.

I guess this returns an array buffer even when the types array was not created with a buffer, right? I would document it and add a test case if there isn't.

> Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3039
> +        g_assert_false(jsc_value_is_typed_array(value.get()));
> +        g_assert_cmpuint(jsc_value_typed_array_get_type(value.get()), ==, JSC_TYPED_ARRAY_NONE);

You can probably move to the next test that creates an array buffer

> Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3050
> +        auto value = adoptGRef(jsc_value_new_array_buffer(context.get(), point3d, sizeof(point3d), nullptr, nullptr));
> +        checker.watch(value.get());

here

> Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3160
> +    }
> +}

I miss a test checking any case of exceptions.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #20)
> Comment on attachment 454112 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454112&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1677
> > +    case kJSTypedArrayTypeNone:
> > +    case kJSTypedArrayTypeArrayBuffer: return JSC_TYPED_ARRAY_NONE;
> 
> I think this is easier to read if returns are in new lines. At first I
> thought you had forgotten to add a return for the None, then I realized we
> return NONE also for ArrayBuffer. Again, I think the C API is confusing, we
> should either add a comment explaining what happens with the ArrayBuffer
> type, or not use JSValueGetTypedArrayType and simply check
> toJSTypedArrayType(object->classInfo(vm)->typedArrayStorageType) directly

I think I'll change this to use the internal API directly, for the sake
of clarity. In the end it will not be much longer, just a few lines more.

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1705
> > + * The @type must *not* be #JSC_TYPED_ARRAY_NONE.
> 
> #JSC_TYPED_ARRAY_NONE -> %JSC_TYPED_ARRAY_NONE

👌️

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1714
> > +    g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr);
> > +
> 
> g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr);

👌️

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1742
> > +JSCValue* jsc_value_new_typed_array_with_buffer(JSCValue* arrayBuffer, JSCTypedArrayType type)
> 
> I think we want to expose the constructor taking an offset too. The JS API
> allows to pass only a buffer, buffer + offset and buffer + offset + length.
> Adding 3 constructors would be too much so we can probably leave this and
> add with_offset taking both offset and length, but length being optional, or
> simply add offset and length here, making both optional.

Let's have only jsc_value_new_typed_array_with_buffer(bufer, type, offset, length),
to avoid proliferation of constructors. It's easy enough to provide zero as the
offset for the default behaviour, and we can use ssize_t/gssize for the length
to allow passing “-1” to mean “calculate the length from the size of the buffer”.

Using a signed ssize_t/gssize for the length is fine because the JS specification
says that 32-bit unsigned integers must be used, and ssize_t/gssize is 64-bit in
all the architectures we support, which can always fit an unsigned 32-bit integer
without loss.

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1783
> > +    return type != kJSTypedArrayTypeNone && type != kJSTypedArrayTypeArrayBuffer;
> 
> This is confusing too.

Let's use here the internal C++ API as well, for clarity.

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1850
> > +    auto* ptr = JSObjectGetTypedArrayBytesPtr(jsContext, jsObject, &exception);
> 
> ptr -> bytes, data or something like that

👌️

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1860
> > +        const auto c = JSObjectGetTypedArrayLength(jsContext, jsObject, &exception);
> 
> c -> length, size or something like that

👌️
 
> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1870
> > + * jsc_value_typed_array_get_count:
> 
> I would use length instead of count

👌️

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1899
> > + * Obtain the %ArrayBuffer for the memory region of the typed array elements.
> 
> I guess this returns an array buffer even when the types array was not
> created with a buffer, right? I would document it and add a test case if
> there isn't.

All typed arrays are backed by an ArrayBuffer. Typed arrays are technically a view
on the contents of their buffers (and the JS spec says so) :-)

> > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3039
> > +        g_assert_false(jsc_value_is_typed_array(value.get()));
> > +        g_assert_cmpuint(jsc_value_typed_array_get_type(value.get()), ==, JSC_TYPED_ARRAY_NONE);
> 
> You can probably move to the next test that creates an array buffer

Will do.

> > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3050
> > +        auto value = adoptGRef(jsc_value_new_array_buffer(context.get(), point3d, sizeof(point3d), nullptr, nullptr));
> > +        checker.watch(value.get());
> 
> here
> 
> > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3160
> > +    }
> > +}
> 
> I miss a test checking any case of exceptions.

Good point.

Also I will add getters for the offset and byte length (as _get_size,
keeping _get_length for the number of elements), as we have discussed
in a chat earlier today.

[Adrian Perez]

Created attachment 454538 [details]
Patch

This patch should address all the review comments =)

[Adrian Perez]

Comment on attachment 454538 [details]
Patch

The ios-wk2 EWS failure is definitely unrelated, this patch only
touches the JSC GLib API, which is not used on iOS at all :]

[Carlos Garcia Campos]

Comment on attachment 454538 [details]
Patch

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

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1748
> +    g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr);
> +

g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr);

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1772
> + * The @type must *not* be #JSC_TYPED_ARRAY_NONE.

#JSC_TYPED_ARRAY_NONE -> %JSC_TYPED_ARRAY_NONE

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1790
> +    g_return_val_if_fail(JSC_IS_VALUE(arrayBuffer), nullptr);
> +    g_return_val_if_fail(jsc_value_is_array_buffer(arrayBuffer), nullptr);
> +    g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr);
> +

We explicitly say that -1 is used for omitting length, so I would add g_return_val_if_fail(length >= -1, nullptr);

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1849
> +    using namespace JSC;

Adding JSC:: is not that much.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1901
> + * ##JSCTypedArrayType), and has the `offset` over the underlying array

##JSCTypedArrayType -> #JSCTypedArrayType

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1934
> +    auto* ptr = JSObjectGetTypedArrayBytesPtr(jsContext, jsObject, &exception);

prt -> bytes

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #24)
> Comment on attachment 454538 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454538&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1748
> > +    g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr);
> > +
> 
> g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr);

Ah, good catch. I actually wrote the prerequisite in the documentation
comment but then forgot to write it in the code 😇️

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1772
> > + * The @type must *not* be #JSC_TYPED_ARRAY_NONE.
> 
> #JSC_TYPED_ARRAY_NONE -> %JSC_TYPED_ARRAY_NONE

👌️

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1790
> > +    g_return_val_if_fail(JSC_IS_VALUE(arrayBuffer), nullptr);
> > +    g_return_val_if_fail(jsc_value_is_array_buffer(arrayBuffer), nullptr);
> > +    g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr);
> > +
> 
> We explicitly say that -1 is used for omitting length, so I would add
> g_return_val_if_fail(length >= -1, nullptr);

Good point, let's add this check in the code.

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1849
> > +    using namespace JSC;
> 
> Adding JSC:: is not that much.

Sure, I'll change to use namespace-prefixed types before landing.

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1901
> > + * ##JSCTypedArrayType), and has the `offset` over the underlying array
> 
> ##JSCTypedArrayType -> #JSCTypedArrayType

👌️

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1934
> > +    auto* ptr = JSObjectGetTypedArrayBytesPtr(jsContext, jsObject, &exception);
> 
> prt -> bytes

👌️

[Adrian Perez]

Created attachment 454578 [details]
Patch for landing

[EWS]

Committed r291229 (248384@main): <https://commits.webkit.org/248384@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454578 [details].

[Radar WebKit Bug Importer]

<rdar://problem/90248012>

[Philippe Normand]

This introduced a new clang warning. 

/app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]
}
^
1 warning generated.

[Adrian Perez]

(In reply to Philippe Normand from comment #29)
> This introduced a new clang warning. 
> 
> /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1:
> warning: non-void function does not return a value in all control paths
> [-Wreturn-type]
> }
> ^
> 1 warning generated.

Fixed in r291543 — thanks for the heads up :)

[Diego Pino]

(In reply to Adrian Perez from comment #30)
> (In reply to Philippe Normand from comment #29)
> > This introduced a new clang warning. 
> > 
> > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1:
> > warning: non-void function does not return a value in all control paths
> > [-Wreturn-type]
> > }
> > ^
> > 1 warning generated.
> 
> Fixed in r291543 — thanks for the heads up :)


Originally the patch had 
This is going to break compilation in GCC 8.3.
(In reply to Adrian Perez from comment #30)
> (In reply to Philippe Normand from comment #29)
> > This introduced a new clang warning. 
> > 
> > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1:
> > warning: non-void function does not return a value in all control paths
> > [-Wreturn-type]
> > }
> > ^
> > 1 warning generated.
> 
> Fixed in r291543 — thanks for the heads up :)

This change broke compilation in GCC 8.3 (Debian Stable bot and Ubuntu 18.04)

https://build.webkit.org/#/builders/46/builds/12338

Originally the patch was like this, but I moved RELEASE_ASSERT_NOT_REACHED(); inside "switch" precisely because it broke the Debian Stable build. See: https://github.com/WebKit/webkit/compare/9d5e85c84fe6%5E..9d5e85c84fe6

[Diego Pino]

(In reply to Diego Pino from comment #31)
> (In reply to Adrian Perez from comment #30)
> > (In reply to Philippe Normand from comment #29)
> > > This introduced a new clang warning. 
> > > 
> > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1:
> > > warning: non-void function does not return a value in all control paths
> > > [-Wreturn-type]
> > > }
> > > ^
> > > 1 warning generated.
> > 
> > Fixed in r291543 — thanks for the heads up :)
> 
> 
> Originally the patch had 
> This is going to break compilation in GCC 8.3.
> (In reply to Adrian Perez from comment #30)
> > (In reply to Philippe Normand from comment #29)
> > > This introduced a new clang warning. 
> > > 
> > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1:
> > > warning: non-void function does not return a value in all control paths
> > > [-Wreturn-type]
> > > }
> > > ^
> > > 1 warning generated.
> > 
> > Fixed in r291543 — thanks for the heads up :)
> 
> This change broke compilation in GCC 8.3 (Debian Stable bot and Ubuntu 18.04)
> 
> https://build.webkit.org/#/builders/46/builds/12338
> 
> Originally the patch was like this, but I moved
> RELEASE_ASSERT_NOT_REACHED(); inside "switch" precisely because it broke the
> Debian Stable build. See:
> https://github.com/WebKit/webkit/compare/9d5e85c84fe6%5E..9d5e85c84fe6

Nevermind, fixed in https://trac.webkit.org/changeset/291551/webkit.

[Adrian Perez]

(In reply to Diego Pino from comment #32)
> (In reply to Diego Pino from comment #31)
> > (In reply to Adrian Perez from comment #30)
> > > (In reply to Philippe Normand from comment #29)
> > > > This introduced a new clang warning. 
> > > > 
> > > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1:
> > > > warning: non-void function does not return a value in all control paths
> > > > [-Wreturn-type]
> > > > }
> > > > ^
> > > > 1 warning generated.
> > > 
> > > Fixed in r291543 — thanks for the heads up :)
> > 
> > 
> > Originally the patch had 
> > This is going to break compilation in GCC 8.3.
> > (In reply to Adrian Perez from comment #30)
> > > (In reply to Philippe Normand from comment #29)
> > > > This introduced a new clang warning. 
> > > > 
> > > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1:
> > > > warning: non-void function does not return a value in all control paths
> > > > [-Wreturn-type]
> > > > }
> > > > ^
> > > > 1 warning generated.
> > > 
> > > Fixed in r291543 — thanks for the heads up :)
> > 
> > This change broke compilation in GCC 8.3 (Debian Stable bot and Ubuntu 18.04)
> > 
> > https://build.webkit.org/#/builders/46/builds/12338
> > 
> > Originally the patch was like this, but I moved
> > RELEASE_ASSERT_NOT_REACHED(); inside "switch" precisely because it broke the
> > Debian Stable build. See:
> > https://github.com/WebKit/webkit/compare/9d5e85c84fe6%5E..9d5e85c84fe6
> 
> Nevermind, fixed in https://trac.webkit.org/changeset/291551/webkit.

The fix makes sense, thanks for taking care of it 🙏️