Support JS typed arrays as arguments and in WebIDL unions #20267

Xanewok · 2018-03-10T17:11:27Z

Supersedes #20205.
This brings support to receiving typed arrays as function arguments (those are stack-rooted with CustomAutoRooter) and also being a member of a union (which is basically heap-rooted? similarly to other webidl unions).

This is based on my other PR #20265 (which means it has to pull an external rust-mozjs branch and contains some also slightly unrelated changes here) since it shares RootedTraceableBox::from_box here and some other additional changes at rust-mozjs, but it can be easily separated if needed.

I tried adding support to nullable typed arrays but couldn't work around an issue of codegen always sticking a "OrNull" at the end of my type (presumably because of this?). How would I go about avoiding the suffix with nullable arguments?

If we were to add also support for nullable typed arrays then I think we wouldn't be blocked anymore on this in WebGL 1.0.

r? @jdm

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2018-03-10T17:11:34Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/bindings/trace.rs, components/script/dom/webidls/TestBinding.webidl
@fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/bindings/trace.rs, components/script/dom/webidls/TestBinding.webidl
@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/bindings/trace.rs, components/script/dom/webidls/TestBinding.webidl

highfive · 2018-03-10T17:11:35Z

Warning

These commits modify unsafe code. Please review it carefully!

bors-servo · 2018-03-12T14:33:26Z

☔ The latest upstream changes (presumably #20216) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2018-03-12T23:08:27Z

Gecko's solution for the nullable name is here. We should be able to do the same thing at https://github.com/servo/servo/pull/20267/files#diff-60d01595cff328c165842fea9e4ccbc2R885.

jdm · 2018-03-12T23:08:42Z

components/script/dom/bindings/codegen/CodegenRust.py

@@ -870,8 +870,50 @@ def wrapObjectTemplate(templateBody, nullValue, isDefinitelyObject, type,

        return handleOptional(templateBody, declType, handleDefaultNull("None"))

-    if type.isSpiderMonkeyInterface():
-        raise TypeError("Can't handle SpiderMonkey interface arguments yet")
+    if type.isTypedArray() or type.isArrayBuffer() or type.isArrayBufferView() or type.isSharedArrayBuffer():


This particular check is repeated in enough places that we should put it behind a descriptive is_typed_array function.

jdm · 2018-03-12T23:08:42Z

components/script/dom/bindings/codegen/CodegenRust.py

-        # Any code to convert to Object is unused, since we're already converting
-        # from an Object value.
-        if t.name == 'Object':
-            return CGGeneric('')


What is the reason for this change?

Same as above: the type of object members in Unions are currently RootedTraceableBox<Heap<*mut JSObject>> and so we need to convert the incoming raw *mut JSObject somehow; with this we generate a used TryConvertToObject with proper body (constructing a RootedTraceableBox from a boxed Heap value with the raw object pointer)

jdm · 2018-03-12T23:08:42Z

components/script/dom/bindings/codegen/CodegenRust.py

        if hasObjectTypes:
            # "object" is not distinguishable from other types
            assert not object or not (interfaceObject or arrayObject or dateObject or callbackObject or mozMapObject)
            templateBody = CGList([], "\n")
+            if object:
+                templateBody.append(object)


What is the reason for this change?

Related to #20265, but I'll leave a note here: without this the contents of if value.get().is_object() { ... } are not generated in from_jsval conversion function

jdm · 2018-03-12T23:08:42Z

components/script/dom/testbinding.rs

@@ -428,6 +429,9 @@ impl TestBindingMethods for TestBinding {
    fn PassByteString(&self, _: ByteString) {}
    fn PassEnum(&self, _: TestEnum) {}
    fn PassInterface(&self, _: &Blob) {}
+    fn PassTypedArray(&self, _: CustomAutoRooterGuard<typedarray::Int8Array>) {}
+    fn PassTypedArray2(&self, _: CustomAutoRooterGuard<typedarray::ArrayBuffer>) {}
+    fn PassTypedArray3(&self, _: CustomAutoRooterGuard<typedarray::ArrayBufferView>) {}


It might be nice to typedef these into Int8Array, etc.

How should we deal with nullable CustomAutoRooterGuard<Option<Typedarray::Int8Array>> types then? Should we simply pass only reference to rooted arguments, hiding the CustomAutoRooterGuard 'implementation detail'?

Hmm. Ideally we would have Option<CustomAutoRooterGuard<Typedarray::Int8Array>> instead. Possibly that just requires rearranging the code that applies the CustomAutoRooterGuard templated type?

Although really passing &typedarray::Int8Array sounds really nice too.

jdm · 2018-03-12T23:08:42Z

components/script/dom/bindings/codegen/CodegenRust.py


-        if isMember in ("Dictionary", "Union"):
+        # TODO: Do we need to do the same for dictionaries?


(This is related to #20265, I should've separated the changes in both PRs clearly, I'm sorry)

So I just checked and currently dictionaries are flagged as #[must_root] and their constructor first allocates a dict using RootedTraceableBox with defaults and only then sets values.
With this approach having Heap<*mut JSObject> as dictionary members suffices and moving the dict box is safe, as long as it's not consumed to retrieve the inner value.

Should I still pursue this, then?

See 3d532e1 for an attempt.

Let's leave the current implementation then, since it is less unsafe than previously believed.

It seems it didn't require additional work and even simplified the codegen slightly, but I can then not include the change for object in dictionaries here

bors-servo · 2018-03-13T12:37:34Z

☔ The latest upstream changes (presumably #20288) made this pull request unmergeable. Please resolve the merge conflicts.

jdm

If we can extract the changes from #20265, I think this will be good to merge!

jdm · 2018-03-13T20:37:37Z

components/script/dom/bindings/codegen/CodegenRust.py

@@ -6418,6 +6468,9 @@ def type_needs_tracing(t):
        if t.isUnion():
            return any(type_needs_tracing(member) for member in t.flatMemberTypes)

+        if t.isTypedArray() or t.isArrayBuffer() or t.isArrayBufferView() or t.isSharedArrayBuffer():


This can still use the helper method.

Ah, missed it, thanks!

jdm · 2018-03-13T20:37:57Z

Waiting on merging the changes upstream in rust-mozjs, of course.


        Auto merge of #397 - Xanewok:typed-arrays-stack-heap-variants, r=jdm

@jdm

Add TypedArray API using Heap-wrapped objects Needed for servo/servo#20267. This changes TypedArray wrapper type to only contain typed array-related logic and not handle rooting, and introduces 2 different possible storage options for wrapped objects - `*mut JSObject`, can be rooted on stack (e.g. using CustomAutoRooter) - `Box<Heap<*mut JSObject>>`, can be rooted on heap with `JSTraceable` trait (implemented on Servo side) Since the API changes are breaking, I bumped minor version so that Servo will not break when updating. r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/397)

bors-servo · 2018-03-14T19:12:39Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: #20224

bors-servo · 2018-03-14T19:12:39Z

📌 Commit c29fcb8 has been approved by Xanewok

bors-servo · 2018-03-14T19:43:20Z

⌛ Testing commit c29fcb8 with merge f7ee1be...


        Auto merge of #20267 - Xanewok:typed-arrays-stack-heap-variants, r=Xa…

@jdm

…newok Support JS typed arrays as arguments and in WebIDL unions  Supersedes #20205. This brings support to receiving typed arrays as function arguments (those are stack-rooted with CustomAutoRooter) and also being a member of a union (which is basically heap-rooted? similarly to other webidl unions). This is based on my other PR #20265 (which means it has to pull an external rust-mozjs branch and contains some also slightly unrelated changes here) since it shares `RootedTraceableBox::from_box` here and some other additional changes at rust-mozjs, but it can be easily separated if needed. I tried adding support to nullable typed arrays but couldn't work around an issue of codegen always sticking a "OrNull" at the end of my type (presumably because of [this](https://github.com/servo/servo/blob/master/components/script/dom/bindings/codegen/parser/WebIDL.py#L2241)?). How would I go about avoiding the suffix with nullable arguments? If we were to add also support for nullable typed arrays then I think we wouldn't be blocked anymore on this in WebGL 1.0. r? @jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20267)

bors-servo · 2018-03-14T20:10:08Z

💔 Test failed - linux-rel-css

Xanewok · 2018-03-14T20:31:30Z

Error when running ./mach filter-intermittents (output here)

Error running mach:

    ['filter-intermittents', 'wpt-errorsummary.log', '--log-intermittents', 'intermittents.log', '--log-filteredsummary', 'filtered-wpt-errorsummary.log', '--tracker-api', 'default', '--reporter-api', 'default']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

HTTPError: HTTP Error 307: Temporary Redirect

  File "/home/servo/buildbot/slave/linux-rel-css/build/python/servo/testing_commands.py", line 540, in filter_intermittents
    response = urllib2.urlopen(request)
  File "/usr/lib/python2.7/urllib2.py", line 127, in urlopen
    return _opener.open(url, data, timeout)
  File "/usr/lib/python2.7/urllib2.py", line 410, in open
    response = meth(req, response)
  File "/usr/lib/python2.7/urllib2.py", line 523, in http_response
    'http', request, response, code, msg, hdrs)
  File "/usr/lib/python2.7/urllib2.py", line 442, in error
    result = self._call_chain(*args)
  File "/usr/lib/python2.7/urllib2.py", line 382, in _call_chain
    result = func(*args)
  File "/usr/lib/python2.7/urllib2.py", line 608, in http_error_302
    new = self.redirect_request(req, fp, code, msg, headers, newurl)
  File "/usr/lib/python2.7/urllib2.py", line 569, in redirect_request
    raise HTTPError(req.get_full_url(), code, msg, headers, fp)

Xanewok · 2018-03-14T20:35:40Z

@bors-servo retry

bors-servo · 2018-03-14T20:35:52Z

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css1, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only linux-rel-css, linux-rel-wpt, mac-rel-css2...

bors-servo · 2018-03-14T21:14:45Z

💔 Test failed - mac-rel-css2

Xanewok · 2018-03-14T21:20:10Z

😭

cargo:warning=gfx/angle/checkout/src/common/angleutils.h:33:5: error: unknown type name 'constexpr'

Retrying as in #20265 (comment):

cc @jdm

@bors-servo retry

#20281

bors-servo · 2018-03-14T21:20:13Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only mac-rel-css2...

bors-servo · 2018-03-14T21:55:21Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: Xanewok
Pushing f7ee1be to master...

highfive assigned jdm Mar 10, 2018

highfive added the S-awaiting-review label Mar 10, 2018

Xanewok mentioned this pull request Mar 10, 2018

[Do not merge] WIP: Accept typed array arguments in codegen #20205

Closed

0 of 5 tasks complete

highfive added the S-needs-rebase label Mar 12, 2018

Xanewok force-pushed the Xanewok:typed-arrays-stack-heap-variants branch from ea99f23 to 65c6011 Mar 12, 2018

jdm requested changes Mar 12, 2018

View changes

jdm added S-needs-code-changes S-blocked-on-external and removed S-awaiting-review S-needs-rebase labels Mar 12, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 13, 2018

highfive added the S-needs-rebase label Mar 13, 2018

Xanewok force-pushed the Xanewok:typed-arrays-stack-heap-variants branch from 6254c68 to 477c7ff Mar 13, 2018

jdm requested changes Mar 13, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Mar 13, 2018

Xanewok mentioned this pull request Mar 13, 2018

Add TypedArray API using Heap-wrapped objects servo/rust-mozjs#397

Merged

Xanewok mentioned this pull request Mar 13, 2018

Fix JS object conversion in unions #20265

Merged

2 of 5 tasks complete

Xanewok force-pushed the Xanewok:typed-arrays-stack-heap-variants branch from 477c7ff to 857e484 Mar 13, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 13, 2018

highfive added S-awaiting-merge and removed S-needs-rebase labels Mar 14, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 14, 2018

highfive added the S-tests-failed label Mar 14, 2018

bors-servo merged commit c29fcb8 into servo:master Mar 14, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Mar 14, 2018

Implement bindings for WebIDL typed arrays #5014

Closed

Make typed arrays safe #19241

Closed

servo / servo

Support JS typed arrays as arguments and in WebIDL unions #20267

Support JS typed arrays as arguments and in WebIDL unions #20267

Xanewok commented Mar 10, 2018 •

edited

highfive commented Mar 10, 2018

highfive commented Mar 10, 2018

bors-servo commented Mar 12, 2018

jdm commented Mar 12, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Mar 13, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 13, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

Xanewok commented Mar 14, 2018

Xanewok commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

Xanewok commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018


		if isMember in ("Dictionary", "Union"):
		# TODO: Do we need to do the same for dictionaries?

servo / servo

Join GitHub today

Support JS typed arrays as arguments and in WebIDL unions #20267

Support JS typed arrays as arguments and in WebIDL unions #20267

Conversation

Xanewok commented Mar 10, 2018 • edited

highfive commented Mar 10, 2018

highfive commented Mar 10, 2018

bors-servo commented Mar 12, 2018

jdm commented Mar 12, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xanewok Mar 13, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Mar 13, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 13, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

Xanewok commented Mar 14, 2018

Xanewok commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

Xanewok commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

Xanewok commented Mar 10, 2018 •

edited

Xanewok Mar 13, 2018 •

edited