Bug 158902 – js/dom/global-constructors-attributes.html is flaky: ResourceTiming runtime feature leaks between tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

In my local test run, js/dom/global-constructors-attributes.html failed because PerformanceEntry and PerformanceResourceTiming were unexpectedly enabled.

Looks like nothing resets runtime enabled properties after they are enabled via Internals. This should be done in Internals::resetToConsistentState().

[Yoav Weiss]

Thanks for reporting. I'll work on a patch and hope to upload it shortly

[Yoav Weiss]

Created attachment 281609 [details]
Patch

[WebKit Commit Bot]

Attachment 281609 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yoav Weiss]

Created attachment 281611 [details]
Patch

[Alex Christensen]

Do we not have this problem with other runtime enabled features?

[Alexey Proskuryakov]

Comment on attachment 281611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281611&action=review

> Source/WebCore/ChangeLog:8
> +        Make sure that the ResourceTiming runtime flag is turned off between tests.

As Alex noted, this presumably happens with other runtime enabled features too.

> Source/WebCore/testing/Internals.cpp:418
> +    RuntimeEnabledFeatures::sharedFeatures().setResourceTimingEnabled(false);

My thinking was that we need to record original values before changing them. A hardcoded false could get out of sync with a changed default.

[Alexey Proskuryakov]

Yoav, do you expect to have a fix soon?

[Yoav Weiss]

Apologies. I'm currently at a conference, so it'll probably be a few days before I can work on this.

To make sure that I understand the proposed solution, do you think I should add a method to RuntimeEnabledFeatures that resets all the flags to their initial state?

[Alexey Proskuryakov]

Yes, and one thing that is important is to actually use initial state without hardcoding it again - we shouldn't have a second copy of initial state values for every feature here.

[Yoav Weiss]

Created attachment 282437 [details]
Patch

[Yoav Weiss]

Added a reset() func as discussed and called it from internals.

PTAL?

[Build Bot]

Comment on attachment 282437 [details]
Patch

Attachment 282437 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1600630

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 282440 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282437 [details]
Patch

Attachment 282437 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1600727

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 282443 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Alex Christensen]

Comment on attachment 282437 [details]
Patch

Not this initial state.  This makes all the tests fail.  The initial state that DumpRenderTree and WebKitTestRunner set.

[Yoav Weiss]

(In reply to comment #16)
> Comment on attachment 282437 [details]
> Patch
> 
> Not this initial state.  This makes all the tests fail.  The initial state
> that DumpRenderTree and WebKitTestRunner set.

I'm not familiar with that state and where it is set. Could you point me in the right direction?

[Alex Christensen]

resetWebPreferencesToConsistentValues in Mac and Win DumpRenderTree
InjectedBundle::beginTesting in WebKitTestRunner

[Yoav Weiss]

Created attachment 282891 [details]
Patch

[Build Bot]

Comment on attachment 282891 [details]
Patch

Attachment 282891 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1635972

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 282897 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Yoav Weiss]

Created attachment 282905 [details]
Patch

[Yoav Weiss]

Current patch changes the defaults for some of the runtime enabled features, as that seems to be the cause for the test failures, but that's obviously not the right solution. I'm just testing it to see if this is the only reason for the bot failures.

[Build Bot]

Comment on attachment 282905 [details]
Patch

Attachment 282905 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1636559

New failing tests:
platform/mac/fast/text/trailing-word-parse.html
fast/text/trailing-word.html
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 282907 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282905 [details]
Patch

Attachment 282905 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1636565

New failing tests:
platform/mac/fast/text/trailing-word-parse.html
fast/dom/navigator-detached-no-crash.html
fast/text/trailing-word.html
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 282908 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282905 [details]
Patch

Attachment 282905 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1636584

New failing tests:
platform/mac/fast/text/trailing-word-parse.html
fast/text/trailing-word.html
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 282910 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Yoav Weiss]

Created attachment 282918 [details]
Patch

[Yoav Weiss]

Seems like IndexedDB gets enabled to true in the mac port (as well as the WebView), so I had to turn it to true in order for the related tests to pass.

I wonder what's the best course of action here. Reseting it to false breaks the tests, and at the same time, AFAICT setting it to true will change its state in other ports (including iossim)

[Yoav Weiss]

Created attachment 282941 [details]
Patch

[Benjamin Poulain]

Comment on attachment 282941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282941&action=review

Check this before landing:

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:67
> -    , m_isIndexedDBEnabled(false)
> +    m_isIndexedDBEnabled = true;

You are changing this default. Is is intended?

[Yoav Weiss]

(In reply to comment #33)
> Comment on attachment 282941 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282941&action=review
> 
> Check this before landing:
> 
> > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:67
> > -    , m_isIndexedDBEnabled(false)
> > +    m_isIndexedDBEnabled = true;
> 
> You are changing this default. Is is intended?

It is intended, but I'm not 100% sure this is the right thing to do. See https://bugs.webkit.org/show_bug.cgi?id=158902#c31

The default gets overridden to true in the mac and win WebViews as well as the WK2 port, and there are various tests that rely on it. But, other ports don't necessarily turn it on, so changing the default would change their behavior. So, I'm not sure what's the best course of action here

[Alexey Proskuryakov]

Would it be feasible to record the original value of the setting when it's overridden using RuntimeEnabledFeatures?

[Yoav Weiss]

(In reply to comment #35)
> Would it be feasible to record the original value of the setting when it's
> overridden using RuntimeEnabledFeatures?

The current flow, unless I change the default value for IndexedDB, is:
* RuntimeEnabledFeatures initializes IndexedDB to false
* WebView/WebKit2 overrides that to true when initialized
* If we reset back to defaults between tests, we override the overrides and bring it back to false.

Do you propose that we'd create a special "set and keep it set across resets" setter for the various flags in RuntimeEnabledFeatures?

[Alexey Proskuryakov]

My idea is that the first time a RuntimeEnabledFeature setter is called, we call WebCore to get the current state of it, remember that, and always reset back to that state between tests.

There are a few essentially equivalent variations here. For example, we could get and remember WebCore's idea of feature state for all RuntimeEnabledFeatures at once the first time one of the setters is called.

[Yoav Weiss]

Created attachment 283141 [details]
Patch

[Yoav Weiss]

I've changed the approach to:
* Freeze the current feature set after it's changed in WebProcess/WebView.
* Restore the features to that frozen set between tests.

It seems to be working and doesn't contain IndexedDB specific logic. It's a pretty radical change from the previous approach, so I think a new review is required.

ap and or benjamin - PTAL?

[Benjamin Poulain]

(In reply to comment #34)
> (In reply to comment #33)
> > Comment on attachment 282941 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=282941&action=review
> > 
> > Check this before landing:
> > 
> > > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:67
> > > -    , m_isIndexedDBEnabled(false)
> > > +    m_isIndexedDBEnabled = true;
> > 
> > You are changing this default. Is is intended?
> 
> It is intended, but I'm not 100% sure this is the right thing to do. See
> https://bugs.webkit.org/show_bug.cgi?id=158902#c31
> 
> The default gets overridden to true in the mac and win WebViews as well as
> the WK2 port, and there are various tests that rely on it. But, other ports
> don't necessarily turn it on, so changing the default would change their
> behavior. So, I'm not sure what's the best course of action here

I prefer your previous approach. My previous r+ still holds.

For testing, we should have a common environment for all ports. If Mac and Win already enabled it by default, it sounds like the right thing to do is to enable it everywhere.
IndexedDB is an important feature anyway. It is beneficial for GTK and EFL to support it ASAP.

I would prefer if you land the previous patch with common features and a reset(). If that breaks the universe, we roll back and discuss what to do about GTK/EFL.

[Yoav Weiss]

Created attachment 283305 [details]
Patch

[Yoav Weiss]

Brought back previous approach. I'll land it this PST afternoon if no objections arise.

[Benjamin Poulain]

Comment on attachment 283305 [details]
Patch

Go for it. Thanks Yoav.

[WebKit Commit Bot]

Comment on attachment 283305 [details]
Patch

Clearing flags on attachment: 283305

Committed r203110: <http://trac.webkit.org/changeset/203110>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

This change makes it impossible to set features that are enabled by the WK2 web process init parameters. WebPage.cpp primes the RuntimeEnabledFeatures, but then upon creation of the Internals object they are reset to initial values.