Make bindings aware of exposed members/partial interfaces #23500

sreeise · 2019-06-03T01:12:08Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #23332

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

This change is

highfive · 2019-06-03T01:12:15Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @avadacatavra (or someone else) soon.

highfive · 2019-06-03T01:12:16Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/guard.rs
@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/guard.rs

highfive · 2019-06-03T01:12:18Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2019-06-03T01:13:37Z

@bors-servo try=wpt

bors-servo · 2019-06-03T01:13:41Z

⌛ Trying commit 47e3db6 with merge 3a639e3...


        Auto merge of #23500 - sreeise:exposed_binding_gen, r=<try>

Make bindings aware of exposed members/partial interfaces  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #23332  - [ ] 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/23500)

sreeise · 2019-06-03T01:15:13Z

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

    return "Condition::Satisfied"


+def ExposedCondition(cond):


This function is just temporary. I wans't sure if there was a better way to get the mappings for the global bit sets. Also, what do 'Worklet' and 'Worker' map to? There is no obvious global bit set for those versus something like 'ServiceWorker'

Worklet and Worker map to any interface that has a [Global=(Worker)] or [Global=(Worklet)] annotation, respectively. For example, both ServiceWorkerGlobalScope and DedicatedWorkerGlobalScope should match Worker.

sreeise · 2019-06-03T01:20:54Z

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

@@ -2593,6 +2621,12 @@ def definition_body(self):
            assert isinstance(func, list) and len(func) == 1
            conditions.append("%s(aCx, aObj)" % func[0])

+        exposed = iface.getExtendedAttribute("Exposed")


@jdm Looking at the generated bindings the constructors for some of the interfaces are duplicated. PerformanceBinding.rs shows:

unsafe fn ConstructorEnabled(aCx: *mut JSContext, aObj: HandleObject) -> bool { is_exposed_in(aObj, InterfaceObjectMap::Globals::DEDICATED_WORKER_GLOBAL_SCOPE | InterfaceObjectMap::Globals::SERVICE_WORKER_GLOBAL_SCOPE | InterfaceObjectMap::Globals::WINDOW) && is_exposed_in(aObj, InterfaceObjectMap::Globals::DEDICATED_WORKER_GLOBAL_SCOPE | InterfaceObjectMap::Globals::WINDOW) }

These include both the iface.exposureSet and the Exposed attribute which seems to be why they are duplicated.
I am not sure on this, should one of these be changed or left out?

What if we merge the exposureSet and exposed attribute checks by creating a set made of the values and iterating over them? Something like set(camel_to_upper_snake(i) for i in iface.exposureSet) + set(camel_to_upper_snake(i) for i in exposed), then generate a single is_exposed_in from the resulting set?

Possibly we should be taking the intersection of the sets, rather than the union of them.

After looking at both the exposed list and iface.exposureSet, there is actually no difference between the two so there may not be a need to generate anything for the exposed list here. The naming is different where the exposed list only includes 'worker' and 'worklet' instead of the global names for worker and worklet but once you map them to the correct names they are the same as the exposureSet for every interface.

sreeise · 2019-06-03T01:32:59Z

The changes don't expose all of the annotations. I was facing some issues when there are multiple annotations exposed versus just one. If I change MemberCondition to return a list of exposed attributes when there are multiple annotations then I could possibly change generateGuardedArray to check for a list and append each condition in the prefableSpecs list:

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

Line 1606 in 03f2236

prefableSpecs.append(

Unfortunately, when I attempted to do so it caused either GL errors or crash errors from Pipeline.rs. I am not exactly sure at this point what is causing the errors or if there is another way to go about this.

bors-servo · 2019-06-03T01:38:54Z

💔 Test failed - linux-rel-css

jdm

Good start!

jdm · 2019-06-03T20:47:27Z

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

    return "Condition::Satisfied"


+def ExposedCondition(cond):


Worklet and Worker map to any interface that has a [Global=(Worker)] or [Global=(Worklet)] annotation, respectively. For example, both ServiceWorkerGlobalScope and DedicatedWorkerGlobalScope should match Worker.

jdm · 2019-06-03T20:47:27Z

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

+        "DedicatedWorker": "DedicatedWorkerGlobalScope",
+        "PaintWorklet": "PaintWorkletGlobalScope",
+        "TestWorklet": "TestWorkletGlobalScope"
+    })


We should be able to generate this dictionary programmatically:

global_descriptors = config.getDescriptors(isGlobal=True) gives us all of the interfaces that have a Global annotation

descriptor.interface.getExtendedAttribute("Global") gives us the list of values for a particular interface

jdm · 2019-06-03T20:47:27Z

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

@@ -1565,7 +1591,9 @@ def getControllingCondition(interfaceMember, descriptor):
            PropertyDefiner.getStringAttr(interfaceMember,
                                          "Pref"),
            PropertyDefiner.getStringAttr(interfaceMember,
-                                          "Func"))
+                                          "Func"),
+            PropertyDefiner.getStringAttr(interfaceMember,


I suspect that we will want a different method than getStringAttr because Exposed can contain multiple values?

jdm · 2019-06-03T20:47:27Z

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

@@ -2593,6 +2621,12 @@ def definition_body(self):
            assert isinstance(func, list) and len(func) == 1
            conditions.append("%s(aCx, aObj)" % func[0])

+        exposed = iface.getExtendedAttribute("Exposed")


What if we merge the exposureSet and exposed attribute checks by creating a set made of the values and iterating over them? Something like set(camel_to_upper_snake(i) for i in iface.exposureSet) + set(camel_to_upper_snake(i) for i in exposed), then generate a single is_exposed_in from the resulting set?

jdm · 2019-06-03T20:48:11Z

I would be really interested in learning more about the errors you saw when you changed generateGuardedArray.

jdm · 2019-06-03T20:49:09Z

The current test failures don't look good:

  │ called `Result::unwrap()` on an `Err` value: () (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(1) }, at src/libcore/result.rs:999)
  │ stack backtrace:
  │    0:     0x55829411289d - backtrace::backtrace::trace::hebbee589b71fa4b4
  │    1:     0x558294111702 - <backtrace::capture::Backtrace as core::default::Default>::default::hefd0ac5d542e2f26
  │    2:     0x55829104d24f - servo::main::{{closure}}::h46466e6e3dca0931
  │    3:     0x5582942f2c98 - rust_panic_with_hook
  │                         at src/libstd/panicking.rs:478
  │    4:     0x5582942f2731 - continue_panic_fmt
  │                         at src/libstd/panicking.rs:381
  │    5:     0x5582942f2615 - rust_begin_unwind
  │    6:     0x5582943115dc - panic_fmt
  │                         at src/libcore/panicking.rs:85
  │    7:     0x558291f2848a - core::result::unwrap_failed::h68c616a3589a1d60
  │    8:     0x5582922f99f6 - _ZN6script3dom8bindings5guard9Condition12is_satisfied17h5112ef86b15e156bE.llvm.2759114191324085468
  │    9:     0x55829208b8fd - script::dom::bindings::interface::create_object::h347e1a14dc508d68
  │   10:     0x55829208b009 - script::dom::bindings::interface::create_interface_prototype_object::ha1fc74c00cf7c8ce

jdm · 2019-06-03T21:24:56Z

Ok, the failures are coming from passing the prototype objects to is_exposed_in (ie. the objects being created by create_interface_prototype_object in CreateInterfaceObjects), which are created with a basic JSClass instead of a JSClass that is part of a DOMJSClass.

I think we can solve this by changing the create_object function to accept a new global: HandleObject argument, pass that to the define_guarded_[methods|properties|constants] functions, and passing the global object when calling is_exposed_in.

sreeise · 2019-06-04T17:45:17Z

Ok, the failures are coming from passing the prototype objects to is_exposed_in (ie. the objects being created by create_interface_prototype_object in CreateInterfaceObjects), which are created with a basic JSClass instead of a JSClass that is part of a DOMJSClass.

I think we can solve this by changing the create_object function to accept a new global: HandleObject argument, pass that to the define_guarded_[methods|properties|constants] functions, and passing the global object when calling is_exposed_in.

I am honestly not sure if this is fixed or not (I guess we will see). Running the tests in a debugger, at least for me, is crashing my screen with out of memory errors so I am assuming this still needs some work.

sreeise · 2019-06-04T17:51:14Z

I would be really interested in learning more about the errors you saw when you changed generateGuardedArray.

They were 502 GL errors and 0 for the pipeline namespace id, but these are no longer there after I made the changes you suggested for create_object and related methods. I believe the errors were related to the same issues in the test failure. If it happens again ill get the back trace to show here.

jdm · 2019-06-04T19:04:48Z

@bors-servo try=wpt

bors-servo · 2019-06-04T19:04:53Z

⌛ Trying commit bc42087 with merge 2e8306c...


        Auto merge of #23500 - sreeise:exposed_binding_gen, r=<try>

Make bindings aware of exposed members/partial interfaces  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #23332  - [ ] 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/23500)

jdm · 2019-07-14T12:14:19Z

@bors-servo try=wpt

bors-servo · 2019-07-14T12:14:25Z

⌛ Trying commit f0cb6af with merge 90ed027...


        Auto merge of #23500 - sreeise:exposed_binding_gen, r=<try>

Make bindings aware of exposed members/partial interfaces  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #23332  - [ ] 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/23500)

bors-servo · 2019-07-14T12:58:12Z

💔 Test failed - linux-rel-wpt

CYBAI · 2019-07-14T13:03:03Z

tests/wpt/web-platform-tests/xhr/abort-after-send.any.js

@@ -9,7 +9,9 @@
        client.addEventListener("readystatechange", test.step_func(function() {
          if(client.readyState == 4) {
            control_flag = true
-            assert_equals(client.responseXML, null)
+            if (test.GLOBAL.isWindow()) {


I think you can fix the failure by using self.GLOBAL.isWindow() 👀

Ref: https://web-platform-tests.org/writing-tests/testharness.html#multi-global-tests

Oh ok, I was thinking it was referring to the actual name used for the test object.

CYBAI · 2019-07-14T13:03:58Z

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WorkerNavigator interface: member productSub", 
    "test": "/html/dom/interfaces.worker.html", 
    "line": 295183, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WorkerNavigator interface: member vendor", 
    "test": "/html/dom/interfaces.worker.html", 
    "line": 295185, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WorkerNavigator interface: member vendorSub", 
    "test": "/html/dom/interfaces.worker.html", 
    "line": 295186, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WorkerNavigator interface: self.navigator must not have property \"productSub\"", 
    "test": "/html/dom/interfaces.worker.html", 
    "line": 295200, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WorkerNavigator interface: self.navigator must not have property \"vendor\"", 
    "test": "/html/dom/interfaces.worker.html", 
    "line": 295202, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WorkerNavigator interface: self.navigator must not have property \"vendorSub\"", 
    "test": "/html/dom/interfaces.worker.html", 
    "line": 295203, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "FAIL", 
    "group": "default", 
    "message": "test.GLOBAL is undefined", 
    "stack": "@http://web-platform.test:8000/xhr/abort-after-send.any.js:12:17\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25\nTest.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35\n@http://web-platform.test:8000/xhr/abort-after-send.any.js:24:16\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25\n@http://web-platform.test:8000/xhr/abort-after-send.any.js:5:12\n@http://web-platform.test:8000/xhr/abort-after-send.any.worker.js:9:1\n", 
    "subtest": "XMLHttpRequest: abort() after send()", 
    "test": "/xhr/abort-after-send.any.worker.html", 
    "line": 326120, 
    "action": "test_result", 
    "expected": "PASS"
}

sreeise · 2019-07-14T13:13:07Z

html/dom has an .ini file for WorkerNavigator I didnt see but I am not sure why abort-after-send.any.js is saying that test.GLOBAL is undefined.


        Change bindings generation to make Exposed annotation aware of member…

…s/partial interfaces

servo-wpt-sync · 2019-07-14T13:36:11Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#17812.

sreeise · 2019-07-14T13:39:27Z

Hopefully that is all of them. Sorry about causing the tests to have to be rerun. I am getting a segfault crash if I try to run abort-after-send.any.js locally which also happens on a few other tests so I can't be sure if it actually passes.

jdm · 2019-07-14T14:43:28Z

Could you file a separate issue with the backtrace from the segfault when running that test? That sounds interesting.

jdm · 2019-07-14T14:44:58Z

@bors-servo try=wpt

bors-servo · 2019-07-14T14:45:02Z

⌛ Trying commit 871239a with merge 43fb4dd...


        Auto merge of #23500 - sreeise:exposed_binding_gen, r=<try>

Make bindings aware of exposed members/partial interfaces  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #23332  - [ ] 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/23500)

bors-servo · 2019-07-14T15:29:17Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

jdm · 2019-07-14T15:32:29Z

@bors-servo r+
Thanks for all the work to fix this long-standing problem!

bors-servo · 2019-07-14T15:32:29Z

📌 Commit 871239a has been approved by jdm

sreeise · 2019-07-14T16:36:17Z

Could you file a separate issue with the backtrace from the segfault when running that test? That sounds interesting.

Done! #23772

bors-servo · 2019-07-14T16:53:19Z

⌛ Testing commit 871239a with merge 95b304b...


        Auto merge of #23500 - sreeise:exposed_binding_gen, r=jdm

Make bindings aware of exposed members/partial interfaces  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #23332  - [ ] 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/23500)

bors-servo · 2019-07-14T19:15:53Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 95b304b to master...

highfive assigned avadacatavra Jun 3, 2019

highfive added the S-awaiting-review label Jun 3, 2019

sreeise reviewed Jun 3, 2019

View changes

highfive added the S-tests-failed label Jun 3, 2019

jdm requested changes Jun 3, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jun 3, 2019

jdm assigned jdm and unassigned avadacatavra Jun 3, 2019

sreeise force-pushed the sreeise:exposed_binding_gen branch from 47e3db6 to bc42087 Jun 4, 2019

highfive added S-awaiting-review and removed S-tests-failed S-needs-code-changes labels Jun 4, 2019

highfive added the S-tests-failed label Jul 14, 2019

CYBAI reviewed Jul 14, 2019

View changes

Change bindings generation to make Exposed annotation aware of member…

Loading status checks…

871239a

…s/partial interfaces

sreeise force-pushed the sreeise:exposed_binding_gen branch from f0cb6af to 871239a Jul 14, 2019

highfive removed the S-tests-failed label Jul 14, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 14, 2019

bors-servo merged commit 871239a into servo:master Jul 14, 2019
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 14, 2019

bors-servo mentioned this pull request Jul 14, 2019

Implemented support for DataView in parsing WebIDL #22770

Closed

2 of 5 tasks complete

jdm mentioned this pull request Dec 16, 2019

ToJSON unconditionally serializes all conditionally-exposed interface members #25281

Closed

servo / servo

Join GitHub today

Make bindings aware of exposed members/partial interfaces #23500

Make bindings aware of exposed members/partial interfaces #23500

Conversation

sreeise commented Jun 3, 2019 • edited by SimonSapin

highfive commented Jun 3, 2019

highfive commented Jun 3, 2019

highfive commented Jun 3, 2019

jdm commented Jun 3, 2019

bors-servo commented Jun 3, 2019

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.

sreeise commented Jun 3, 2019

bors-servo commented Jun 3, 2019

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Jun 3, 2019

jdm commented Jun 3, 2019

jdm commented Jun 3, 2019

sreeise commented Jun 4, 2019

sreeise commented Jun 4, 2019

jdm commented Jun 4, 2019

bors-servo commented Jun 4, 2019

jdm commented Jul 14, 2019

bors-servo commented Jul 14, 2019

bors-servo commented Jul 14, 2019

This comment has been minimized.

This comment has been minimized.

CYBAI commented Jul 14, 2019

sreeise commented Jul 14, 2019

servo-wpt-sync commented Jul 14, 2019

sreeise commented Jul 14, 2019

jdm commented Jul 14, 2019

jdm commented Jul 14, 2019

bors-servo commented Jul 14, 2019

bors-servo commented Jul 14, 2019

jdm commented Jul 14, 2019

bors-servo commented Jul 14, 2019

sreeise commented Jul 14, 2019

bors-servo commented Jul 14, 2019

bors-servo commented Jul 14, 2019

sreeise commented Jun 3, 2019 •

edited by SimonSapin