New issue
Advanced search Search tips

Issue 878566 link

Starred by 3 users
Status: Fixed
Owner:
peria@chromium.org
Closed: Sep 6
Cc:
mek@chromium.org
lfg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 878539



Sign in to add a comment

Context dependent methods on global interface are not installed

Project Member Reported by mek@chromium.org, Aug 28 
In https://chromium-review.googlesource.com/c/chromium/src/+/1194792/1 I attempted to add a method to a partial interface, but it doesn't seem to get installed correctly (i.e. going to a secure context page and trying to access it fails). If I remove the SecureContext attribute from the new method it does seem to get installed correctly.

The difference in generated bindings seems to be that with the SecureContext attribute it gets installed from (an otherwise empty, leading me to believe that this might be a untested combination of flags) if block at the end of V8WindowPartial::InstallConditionalFeatures, while without the SecureContext attribute V8WindowPartial::InstallRuntimeEnabledFeaturesOnTemplate installs the new method.

Comment 1 by peria@chromium.org, Aug 31

Owner: peria@chromium.org
Status: Started (was: Untriaged)
Summary: Context dependent methods on global interface are not installed (was: SecureContext RuntimeEnabled method on a (modules) partial interface doesn't work)
Thank you for this report. It seems a bug of our IDL compiler.

The operation is defined in Window interface, which has [Global] annotation.
So the corresponding function object should be defined on the instance object (==global object in this case).
https://heycam.github.io/webidl/#es-operations

However, adding [SecureContext], our IDL compiler generates

if (!protoypeObject.IsEmpty() || !interfaceObject.Empty()) {
  // We reach this branch when we're creating prototype chain, and have no instance objects.
  if (isSecure) {
    // installMethod(OnInstance);
  }
}

and hence it installs nothing.
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 31 

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

commit 7e2fa0a1ff802691ddc74eca842ed214456cec9e
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Fri Aug 31 14:49:35 2018

IDL compiler: Fix IDL compiler to install conditional methods on instance

Before this CL, IDL compiler tried to install all conditional methods
on a prototype object or an interface object (=prototype chain).
However, in some cases, we have to install them on an instance object.
(e.g. Operations defined on a [Global] interface.)
https://heycam.github.io/webidl/#es-operations

This CL fixes IDL compiler behavior as expected.


Bug:  878566 
Change-Id: I7efa6df6e6f452d87da82180b21d046b994c5e67
Reviewed-on: https://chromium-review.googlesource.com/1198884
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588023}
[modify] https://crrev.com/7e2fa0a1ff802691ddc74eca842ed214456cec9e/third_party/blink/renderer/bindings/templates/interface_base.cpp.tmpl
[modify] https://crrev.com/7e2fa0a1ff802691ddc74eca842ed214456cec9e/third_party/blink/renderer/bindings/templates/methods.cpp.tmpl
[modify] https://crrev.com/7e2fa0a1ff802691ddc74eca842ed214456cec9e/third_party/blink/renderer/bindings/tests/idls/core/test_interface_check_security.idl
[modify] https://crrev.com/7e2fa0a1ff802691ddc74eca842ed214456cec9e/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface.cc
[modify] https://crrev.com/7e2fa0a1ff802691ddc74eca842ed214456cec9e/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_check_security.cc
[modify] https://crrev.com/7e2fa0a1ff802691ddc74eca842ed214456cec9e/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_check_security.h
[modify] https://crrev.com/7e2fa0a1ff802691ddc74eca842ed214456cec9e/third_party/blink/renderer/bindings/tests/results/modules/v8_test_interface_5.cc

Comment 3 by peria@chromium.org, Sep 4

Cc: mek@chromium.org
Status: Fixed (was: Started)
I guess this issue was fixed with #2.
mek@, could you check with updating your IDL?

Comment 4 by mek@chromium.org, Sep 5

Status: Assigned (was: Fixed)
I tried adding SecureContext back to that chooseFileSystemEntries method, but it still doesn't seem to be working for me... The generated bindings code is now different but I still can't access the method from a secure context.

Comment 5 by mek@chromium.org, Sep 5 

Ah, I figured out what is wrong: the call to InstallMethod passed nullptr instead of instanceObject so there wasn't any instance for it to be installed on.
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 6 

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

commit 1be3dfc39a3151e0736559930a146face2b2b8e2
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Thu Sep 06 02:25:32 2018

IDL compiler: Really fix IDL compiler to install conditional methods on instance.

The previous fix still passed nullptr as instance, so wasn't actually
installing anything on the instance. This CL fixes that.

Also adds SecureContext to the chooseFileSystemEntries method that
wanted this to work.

Bug:  878566 , 878539
Change-Id: I7d7e6a5f62561ca5696b3066b3b7807ed61ff41c
Reviewed-on: https://chromium-review.googlesource.com/1208812
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589088}
[modify] https://crrev.com/1be3dfc39a3151e0736559930a146face2b2b8e2/third_party/blink/renderer/bindings/templates/methods.cpp.tmpl
[modify] https://crrev.com/1be3dfc39a3151e0736559930a146face2b2b8e2/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface.cc
[modify] https://crrev.com/1be3dfc39a3151e0736559930a146face2b2b8e2/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_check_security.cc
[modify] https://crrev.com/1be3dfc39a3151e0736559930a146face2b2b8e2/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_conditional_secure_context.cc
[modify] https://crrev.com/1be3dfc39a3151e0736559930a146face2b2b8e2/third_party/blink/renderer/bindings/tests/results/core/v8_test_interface_secure_context.cc
[modify] https://crrev.com/1be3dfc39a3151e0736559930a146face2b2b8e2/third_party/blink/renderer/bindings/tests/results/modules/v8_test_interface_5.cc
[modify] https://crrev.com/1be3dfc39a3151e0736559930a146face2b2b8e2/third_party/blink/renderer/modules/filesystem/window_file_system.idl

Comment 7 by mek@chromium.org, Sep 6

Status: Fixed (was: Assigned)
►

Sign in to add a comment