W3C tests fail verifying IDL attributes that are promises |
|||||||||
Issue descriptionChrome Version: 60.0.3112.101 (Official Build) (64-bit) OS: Linux (Also fails on ToT and on Windows) What steps will reproduce the problem? (1) Go to https://w3c-test.org/encrypted-media/idlharness.html What is the expected result? All tests pass What happens instead? "MediaKeySession interface: attribute closed" test fails. TypeError: Illegal invocation at IdlInterface.<anonymous> (https://w3c-test.org/resources/idlharness.js:1675:69) at Test.step (https://w3c-test.org/resources/testharness.js:1485:25) at IdlInterface.test_member_attribute (https://w3c-test.org/resources/idlharness.js:1612:12) at IdlInterface.test_members (https://w3c-test.org/resources/idlharness.js:2016:22) at IdlInterface.test (https://w3c-test.org/resources/idlharness.js:1056:10) at self.IdlArray.IdlArray.test (https://w3c-test.org/resources/idlharness.js:542:28) at https://w3c-test.org/encrypted-media/idlharness.html:42:27 at <anonymous> However, running this test on Firefox (Nightly 57.0a1) passes. (This also happens on https://w3c-test.org/html/dom/interfaces.html, test "PromiseRejectionEvent interface: attribute promise", but there are 5323 tests run, with lots of other failures, via this web page.) These 2 tests that fail are checking an attribute that is a promise as defined in the IDL. interface MediaKeySession : EventTarget { readonly attribute Promise<void> closed; }; interface PromiseRejectionEvent : Event { readonly attribute Promise<any> promise; }; The MediaKeySession interface does not exactly match what Chrome uses, and that is being tracked in issue 757622 . However, changing the definition does not affect whether this works or not. As this works on Firefox, I'm opening a Chrome bug. It is possible that the test and/or Firefox do the wrong thing, but I can't tell. Failure happens in idlharness.js (Chromium copy here: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/resources/idlharness.js?l=1675).
,
Aug 23 2017
Tested the issue on windows 10 , ubuntu 14.04 and mac os 10.12.6 using chrome M60 #60.0.3112.101 and M62 #62.0.3193.0 and issue is reproduced. This is a non-regression issue , the tests are failed in earlier chrome versions. From M57 to M59 , 3 tests are failed. In M56 , 4 test are failed. From M55 to older versions of chrome , 16 tests are failed. Hence this is a Non-regression issue. Thanks!
,
Aug 24 2017
Changing the components, since this is either a problem with our bindings, or a bug in idlharness.js, in which case ecosystem infra is the closest thing to being responsible.
,
Aug 24 2017
What this part of of idlharness.js is testing what happens when accessing MediaKeySession.prototype.closed (or PromiseRejectionEvent.prototype.promise). Firefox returns a rejected promise, we throw an exception. So this is a real interop problem, albeit a very small one. https://heycam.github.io/webidl/#dfn-attribute-getter defines what should happen, and the TypeError thrown in step 1.1.2.4.2 is converted into a rejected promise later. So, this looks like a bindings bug, but leaving as untriaged so that it can be confirmed.
,
Aug 25 2017
The bindings automatically converts an exception into a reject promise if it's IDL operations, but not for IDL attributes. This is something that we're simply missing and we should support.
,
Aug 25 2017
,
Sep 1 2017
That might work if the attribute is declared with [RaisesException], but how would you handle the case reported here (accessing the attribute via the interface's prototype)? As far as I can see, V8 throws the "Illegal invocation" before even reaching Blink.
,
Sep 1 2017
We can disable the type check in V8. If we set the empty v8::Signature when registering a v8::FunctionTemplate, then V8 doesn't perform a type check. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp?rcl=3c9342918b69733cb42dd61d24e328448c14ef0c&l=403 Here, we're passing the empty v8::Signature when it's a promise-returning IDL operation. Basically, we can do the same thing for IDL attributes, too. This is the CL for IDL operations: https://codereview.chromium.org/2441593002 https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h?rcl=3c9342918b69733cb42dd61d24e328448c14ef0c&l=39 ExceptionToRejectPromiseScope is a helper class to support promise-returning IDL operations, and I think we can use this helper class for IDL attributes, too. See how this class is used. This is the CL introduced the class: https://codereview.chromium.org/2301993002 If you're interested in this issue, I'm more than happy to hand over this issue to you. :)
,
Sep 1 2017
I think this is the first actual reason I've been for v8::Signature :-) I can take a look at this one, it really looks quite simple (famous last words).
,
Sep 5 2017
,
Sep 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bc08fef84a633ea57aa533cf0ef3d94bf15e263a commit bc08fef84a633ea57aa533cf0ef3d94bf15e263a Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Date: Tue Sep 05 13:57:23 2017 bindings: Do not throw exceptions on IDL attributes with Promise types. Instead, turn them into rejected promises, as specified in https://heycam.github.io/webidl/#dfn-attribute-getter. Most of the required mechanisms were already in place due to the work on bug 627309 (which is essentially the same thing but for IDL operations). Just like we already do for attributes with the [LenientThis] extended attribute, we also disable some V8 type checks when an attribute has a Promise type so that we can receive any exceptions and turn them into a promise rejection, otherwise attempting to e.g. access my_interface.prototype.myPromiseAttr would result in V8 throwing a TypeError due to the wrong receiver being specified and we would not be able to turn handle the exception. Bug: 758023 Change-Id: I45ad4183f6fe7f571e20ac16c3edacfbc87383d8 Reviewed-on: https://chromium-review.googlesource.com/649627 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com> Cr-Commit-Position: refs/heads/master@{#499618} [modify] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/LayoutTests/external/wpt/html/dom/interfaces-expected.txt [modify] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/LayoutTests/external/wpt/html/dom/interfaces.worker-expected.txt [modify] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animation/idlharness-expected.txt [modify] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp [modify] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp [modify] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/Source/bindings/scripts/v8_attributes.py [modify] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl [add] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/Source/bindings/tests/idls/core/TestAttributeGetters.idl [add] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/Source/bindings/tests/results/core/V8TestAttributeGetters.cpp [add] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/Source/bindings/tests/results/core/V8TestAttributeGetters.h [modify] https://crrev.com/bc08fef84a633ea57aa533cf0ef3d94bf15e263a/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
,
Sep 5 2017
Fixed for M63. I'd rather be safe than sorry, so I've also added an entry for this change to Chromestatus: https://www.chromestatus.com/feature/5654995223445504 |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by manoranj...@chromium.org
, Aug 23 2017