internals.runtimeFlags and v8 bindings

[Stefan Zager]

Hello blinkies,

I'm writing tests for a feature that is hidden behind a runtime flag. Rather than creating a virtual test suite, I'm attempting to use the internals.runtimeFlags API to enable the feature at the beginning of each test:

if (window.internals && internals.runtimeFlags) {                                            

  internals.runtimeFlags.intersectionObserverV2Enabled = true;                               

}

When the feature is enabled, a new attribute appears on the IntersectionObserverEntry object:

interface IntersectionObserverEntry {

    // ...

    [RuntimeEnabled=IntersectionObserverV2] readonly attribute boolean isVisible;

    // ...

};

When I run the tests locally (on linux), everything seems to work fine; but after landing the patch, the new tests are flaky on the Windows bots. As far as I can tell, the source of the flakiness is that the feature-enabled attribute "isVisible" is not always defined on IntersectionObserverEntry objects. Digging into the binding code generated for IntersectionObserverEntry, it seems that the object prototype is defined statically by a method that probably is not intended to run more than once:

void V8IntersectionObserverEntry::InstallRuntimeEnabledFeaturesOnTemplate(

  ...

So... can anyone clarify what's going on here? Does InstallRuntimeEnabledFeaturesOnTemplate run lazily, so that modifying internals.runtimeFlags at the beginning of a test is OK as long as it happens before the feature is used? Is there some unpredictability in the timing of when InstallRuntimeEnabledFeaturesOnTemplate runs, which might account for the flakiness? Is Windows special in this regard?

Thanks for any guidance!

[Dave Tapuska]

My guess is that the bindings aren't being rebound when you update a runtime enabled property. The layout test engine tries to reuse the same page I believe for optimization. So locally it probably works if you run the ones that it needs directly. But if you run one that doesn't have the option then your test and it reuses a renderer then the attribute isn't bound.

I don't believe setting runtime settings via javascript is a desirable approach.. See https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/testing/internal_settings.idl?sq=package:chromium&g=0&l=59

dave.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAHOQ7J_wURASwQV0k3frq_CoJOU6T1tZCbbETjP0FxkgyoGYww%40mail.gmail.com.

[Kentaro Hara]

+Yuki Shiino +Hitoshi Yoshida 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAHgVhZWn%2BqKQeSx6N8wYM0YViwgp4kJQgL8mJbvg-94QoQ0%2Baw%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

[Ian Clelland]

On Wed, Sep 12, 2018 at 12:29 PM Stefan Zager <sza...@chromium.org> wrote:

Hello blinkies,

I'm writing tests for a feature that is hidden behind a runtime flag. Rather than creating a virtual test suite, I'm attempting to use the internals.runtimeFlags API to enable the feature at the beginning of each test:

if (window.internals && internals.runtimeFlags) {                                            

  internals.runtimeFlags.intersectionObserverV2Enabled = true;                               

}

When the feature is enabled, a new attribute appears on the IntersectionObserverEntry object:

interface IntersectionObserverEntry {

    // ...

    [RuntimeEnabled=IntersectionObserverV2] readonly attribute boolean isVisible;

    // ...

};

When I run the tests locally (on linux), everything seems to work fine; but after landing the patch, the new tests are flaky on the Windows bots. As far as I can tell, the source of the flakiness is that the feature-enabled attribute "isVisible" is not always defined on IntersectionObserverEntry objects. Digging into the binding code generated for IntersectionObserverEntry, it seems that the object prototype is defined statically by a method that probably is not intended to run more than once:

void V8IntersectionObserverEntry::InstallRuntimeEnabledFeaturesOnTemplate(

  ...

So... can anyone clarify what's going on here? Does InstallRuntimeEnabledFeaturesOnTemplate run lazily, so that modifying internals.runtimeFlags at the beginning of a test is OK as long as it happens before the feature is used?

Yes, I believe it's installed lazily, the first time the constructor for the object is looked up. As long as you have set the flag before that point, the prototype should be set up correctly. Otherwise, the version without the feature will be cached and used from that point on.

 

Is there some unpredictability in the timing of when InstallRuntimeEnabledFeaturesOnTemplate runs, which might account for the flakiness?

I don't think so, unless there is unpredictablility in when the IntersectionObserverEntry object is initially needed.

 

Is Windows special in this regard?

Probably not.

Origin trials use a slighly different mechanism for adding features which be more resilient to switching the flags on an already running v8 context; it might be worth looking in to whether that mechanism should be hooked up to the InternalRuntmeSettings object as well, when possible.

[Stefan Zager]

On Wed, Sep 12, 2018 at 9:42 AM Dave Tapuska <dtap...@chromium.org> wrote:

My guess is that the bindings aren't being rebound when you update a runtime enabled property. The layout test engine tries to reuse the same page I believe for optimization. So locally it probably works if you run the ones that it needs directly. But if you run one that doesn't have the option then your test and it reuses a renderer then the attribute isn't bound.

I don't believe setting runtime settings via javascript is a desirable approach.. See https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/testing/internal_settings.idl?sq=package:chromium&g=0&l=59

I think that comment is discouraging us from using internals.settings. It actually recommends using internals.runtimeFlags instead, which is what I've done in my tests.

Do you know if it's OK to re-bind the prototype for IntersectionObserverEntry in response to modifying the flag?

[Yuki Shiino]

+1 to Dave's comment.

Runtime-enabled-features/flags are expected to be statically set and to not change dynamically.  If the values change, then the process should be restarted (that's what we're asking for end users).  I understand the demand and its convenience, but the bindings are optimized based on the fact that runtime-enabled-features are static.

I feel sorry, but would you avoid dynamically changing runtime-enabled-features?

Cheers,

Yuki Shiino

2018年9月13日(木) 1:45 Kentaro Hara <har...@chromium.org>:

[Kentaro Hara]

Yeah, agreed. Historically we had supported [ContextEnabled] (I forgot the exact name) to let users change the flag dynamically with a per-document granularity. However, we removed the feature for its complexity.

[Hitoshi Yoshida]

 

So... can anyone clarify what's going on here? Does InstallRuntimeEnabledFeaturesOnTemplate run lazily, so that modifying internals.runtimeFlags at the beginning of a test is OK as long as it happens before the feature is used? Is there some unpredictability in the timing of when InstallRuntimeEnabledFeaturesOnTemplate runs, which might account for the flakiness? Is Windows special in this regard?

Yes, you have to update flags before the first instantiation of the interface object.

Is Window special?  Yes, interface templates for Window and few other interfaces are generated before

running any JS script, so they are not effected by such dynamic updates.

In general, however, as other bindings mates say, we do not recommend to update runtime enabled flags dynamically.

--

Hitoshi Yoshida (Peria)

email: pe...@chromium.org

[Stefan Zager]

This has some overlap with the other blink-dev thread about experimental features and virtual test suites.

Are you saying that internals.runtimeFlags ought to be deprecated? There are a bunch of layout tests that use it. Compared to creating a virtual test suite, it's much simpler and more attractive to set the flag from script in a test. I assume that if the feature author adds settable_from_internals:true to the runtime_enabled_features.json5 entry, then they take responsibility for ensuring that changing the feature dynamically works as exepcted.

[Yuki Shiino]

> Are you saying that internals.runtimeFlags ought to be deprecated?

If you just expect the internal implementation to change its behavior according to the flag, internals.runtimeFlags may work (depending on the implementation).  However, the bindings doesn't support the exposure change of the Web APIs, and it's hard to support it.

http://dev.chromium.org/blink/runtime-enabled-features

I've not yet understood the situation very well, but can you live with

    status: "test",

for example?

Cheers,

Yuki Shiino

2018年9月14日(金) 13:56 Stefan Zager <sza...@chromium.org>:

[Hayato Ito]

The page, http://dev.chromium.org/blink/runtime-enabled-features, says:

> (internals.runtimeFlags') attribute is read only and cannot be changed.

However, it looks there are many layout tests which change internals.runtimeFlags.* attributes.

It looks a contradiction to me, which should be fixed somehow.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAN0uC_R-CtJZRZvtDtJ7voY%2BF5QBRRuY3CrfQtQExu3BwOn53Q%40mail.gmail.com.

--

Hayato