New issue
Advanced search Search tips

Issue 627309 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug


Sign in to add a comment

IDL bindings: Calling a promise-returning method with this=null should reject, not throw

Project Member Reported by mgiuca@chromium.org, Jul 12 2016

Issue description

Version: 54
OS: All

What steps will reproduce the problem?
1. Open console.
2. navigator.permissions.query.apply().catch(x => console.log('rejected:', x))

What is the expected output?
rejected: TypeError: Illegal invocation

What do you see instead?
VM134:1 Uncaught TypeError: Illegal invocation(…)

If the method is called with a null this, it is supposed to reject the promise with a TypeError, not directly throw an exception. Firefox behaves correctly.

From the spec*
https://heycam.github.io/webidl/#es-operations

> 2. Let O be a value determined as follows:
>  - If the operation is a static operation, then O is null.
>  - Otherwise, if the interface on which the operation appears has an [ImplicitThis]
>    extended attribute, and the this value is null or undefined, then O is the ECMAScript
>    global object associated with the Function object.
>  - Otherwise, if the this value is not null, then O is the this value.
>  - Otherwise, throw a TypeError.
> ...
> And then, if an exception was thrown:
> 1. If the operation has a return type that is a promise type, then:
>  - Let reject be the initial value of %Promise%.reject.
>  - Return the result of calling reject with %Promise% as the this object and the
>    exception as the single argument value.

So it is supposed to convert the exception into a rejection even in the case where the this value is null.

The idlharness test framework (which we are using for new APIs on Chrome) explicitly tests for this behaviour in do_member_operation_asserts:
https://github.com/w3c/testharness.js/blob/master/idlharness.js

And currently fails on all interfaces (e.g., on my new share interface that I'm trying to write tests for):
FAIL Navigator interface: operation share(DOMString,DOMString) assert_unreached: Throws "TypeError: Illegal invocation" instead of rejecting promise Reached unreachable code

* Wait, isn't this an unratified editor's draft? And the real spec at http://www.w3.org/TR/WebIDL/ says nothing of the sort? Technically true but from what I can tell, all the browser vendors have thrown all of that ratification business out the window and just implement drafts now. Chrome and Firefox are both mostly compliant with the current editor's draft, and our tests expect the behaviour from the editor's draft. I previously filed a bug against Chrome (https://crbug.com/602909) about this behaviour and was told the editor's draft is the correct behaviour.
 

Comment 1 Deleted

Comment 2 by mgiuca@chromium.org, Jul 12 2016

Blocking: 627310

Comment 3 by mfo...@chromium.org, Jul 12 2016

Blocking: 598113
I ran into this same issue with the WebIDL tests for the PaymentRequest API:

https://w3c-test.org/submissions/3439/payment-request/interfaces.https.html

And in https://bugs.chromium.org/p/chromium/issues/detail?id=635688 I see that Encrypted Media WebIDL tests also ran into it:

https://www.w3c-test.org/encrypted-media/idlharness.html
Blocking: 635688
There are at least four specs experiencing this issue. While this is probably low priority for users and authors, it is impacting developers and standards progress.
Cc: yukishiino@chromium.org
The "illegal invocation" error is thrown from HandleApiCallHelper in v8/src/builtins/builtins-api.cc. The bindings layer knows whether a function is expected to return a promise, but V8 doesn't. If we want to return a rejected promise rather than throw an exception, we need to teach it to V8.

Comment 8 by mfo...@chromium.org, Aug 22 2016

Can v8 throw a specific error for a |null| receiver and allow the bindings layer to propagate it (for synchronous functions) or reject the Promise (for Promise-returning functions)?

V8 checks the type of the receiver object only when an embedder specified to do  so.  Otherwise, V8 allows any value as |this|.

I think that the code generator for the binding layer can specify the flag to allow any value as |this|, and check the type of |this| value on Blink side.  Then we can do whatever we want for this=null|undefined case.
Owner: yukishiino@chromium.org
Status: Started (was: Available)
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/afb9da1a91b0d1742a765b7a35f27e1be6645596

commit afb9da1a91b0d1742a765b7a35f27e1be6645596
Author: yukishiino <yukishiino@chromium.org>
Date: Wed Sep 14 08:46:00 2016

binding: Introduces ExceptionToPromiseScope.

ExceptionState is now a scope-like object, and guarantees
that an exception is automatically thrown at destruction.
ExceptionToPromiseScope similarly guarantees to return
a reject promise instead of throwing an exception.

A new Jinja2 filter "format_remove_duplicates" helps
write a template more easily without caring duplicates.
    {% if A_enabled %}
    ScriptState* scriptState = ...;
    A(scriptState);
    {% endif %}
    {% if B_enabled %}
    ScriptState* scriptState = ...;
    B(scriptState);
    {% endif %}
produces two lines of definitions of |scriptState|,
however, format_remove_duplicates removes the duplicates.

Jinja2 macro "propagate_error_with_exception_state" is no
longer necessary, because ExceptionState and
ExceptionToPromiseScope automatically throws an exception
or returns a reject promise appropriately.

This CL alone does not fix  crbug.com/627309  , but the CL
makes it easy to fix the issue.

BUG= 627309 

Review-Url: https://codereview.chromium.org/2301993002
Cr-Commit-Position: refs/heads/master@{#418513}

[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/scripts/code_generator_v8.py
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/scripts/utilities.py
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/templates/methods.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestException.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexed.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedGlobal.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedPrimaryGlobal.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface3.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCheckSecurity.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor2.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor3.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor4.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceEventInitConstructor.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceEventTarget.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceNamedConstructor.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceNamedConstructor2.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceNode.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceOriginTrialEnabled.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceSecureContext.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperations.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/core/V8TestTypedefs.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface2Partial.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface5.cpp
[modify] https://crrev.com/afb9da1a91b0d1742a765b7a35f27e1be6645596/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterfacePartial.cpp

Blocking: 651761
>This CL alone does not fix  crbug.com/627309  , but the CL
>makes it easy to fix the issue.

yukishiino, do you plan to fix the issue? Is there a single fix for all APIs or will there be per-API changes? Thanks.
Status: Assigned (was: Started)
I have an idea to fix this issue in my mind, but the priority of this issue is low on my todo list.  If this issue is important, please let me know.  I think a single CL will fix all promise-returning DOM operations.
The main thing this is blocking is testing.

New API authors are being encouraged to use idlharness to automate test writing. Unfortunately (as I said in the original report), this can't be used right now, because it assumes this behaviour which Blink doesn't do correctly.

Everyone who adds a new API to Blink will likely encounter this (see #5) so I think this actually should have a higher priority.
I work on the Web Platform Tests (WPT) project https://github.com/w3c/web-platform-tests and am chiming in here in support of mguica’s statements in https://bugs.chromium.org/p/chromium/issues/detail?id=627309&desc=2#c15. Along with him I would like to see this issue be given higher priority, because it causes Blink/Chromium-based browsers to fail all tests in the WPT suite of interfaces with methods that return promises—while other browser engines pass those tests as expected.

But perhaps more importantly, for Web developers who are trying to write Web-application code that use those interfaces, it can potentially cause them to run into surprising problems with the behavior of their code due to the mismatch between what what the expected behavior is based on the spec, and the actual behavior in Blink/Chrome.

Admittedly, the code that most developers write would probably not cause them to ever run into the mismatch. But still, for some developers, the mismatch/non-conformance to the standard spec could cause their code to fail in surprising ways, and so it risks wasting the time of those developers that they would have to spend figuring out why they are not seeing the same behavior in Chrome or some other Blink/Chromium-base that they see in browsers based on other engines.
Status: Started (was: Assigned)
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa

commit ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa
Author: yukishiino <yukishiino@chromium.org>
Date: Thu Nov 17 07:20:50 2016

binding: Returns a reject promise when |this| is not of the type.

Returns a reject promise when the receiver object is not of type
of the interface, instead of throwing a TypeError.

This CL disables the type check on V8 side and does the type check
on Blink side, then, if the receiver object is not of the type,
converts a TypeError to a reject promise and returns it.

BUG= 627309 

Review-Url: https://codereview.chromium.org/2441593002
Cr-Commit-Position: refs/heads/master@{#432798}

[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/LayoutTests/imported/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackFunctions.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestException.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexed.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedGlobal.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedPrimaryGlobal.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface3.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCheckSecurity.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceNode.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceOriginTrialEnabled.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceSecureContext.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperations.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/core/V8TestTypedefs.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface2Partial.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface5.cpp
[modify] https://crrev.com/ef6ea8e2cae02459a206885d1eb6ab2f5b62fbfa/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterfacePartial.cpp

Status: Fixed (was: Started)

Sign in to add a comment