New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 758023 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
OoO until Feb 4th
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

W3C tests fail verifying IDL attributes that are promises

Project Member Reported by jrumm...@chromium.org, Aug 22 2017

Issue description

Chrome 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).
 
Labels: Needs-Triage-M60 Needs-Bisect

Comment 2 by hdodda@chromium.org, Aug 23 2017

Cc: hdodda@chromium.org
Labels: -Pri-3 -Needs-Bisect M-62 Pri-2
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!

Comment 3 by foolip@chromium.org, Aug 24 2017

Components: -Blink>JavaScript Blink>Bindings Blink>Infra>Ecosystem
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.

Comment 4 by foolip@chromium.org, Aug 24 2017

Components: -Blink>Infra>Ecosystem
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.
Owner: yukishiino@chromium.org
Status: Assigned (was: Untriaged)
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.
Cc: bashi@chromium.org peria@chromium.org
Cc: raphael....@intel.com
Labels: -Needs-Triage-M60
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.
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.  :)

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).
Cc: yukishiino@chromium.org
Owner: raphael....@intel.com
Status: Started (was: Assigned)
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Labels: -M-62 M-63
Status: Fixed (was: Started)
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