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().
Thanks for reporting. I'll work on a patch and hope to upload it shortly
Created attachment 281609 [details] Patch
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.
Created attachment 281611 [details] Patch
Do we not have this problem with other runtime enabled features?
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.
Yoav, do you expect to have a fix soon?
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?
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.
Created attachment 282437 [details] Patch
Added a reset() func as discussed and called it from internals. PTAL?
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.
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
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.
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
Comment on attachment 282437 [details] Patch Not this initial state. This makes all the tests fail. The initial state that DumpRenderTree and WebKitTestRunner set.
(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?
resetWebPreferencesToConsistentValues in Mac and Win DumpRenderTree InjectedBundle::beginTesting in WebKitTestRunner
Created attachment 282891 [details] Patch
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.
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
Created attachment 282905 [details] Patch
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.
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
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
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
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
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
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
Created attachment 282918 [details] Patch
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)
Created attachment 282941 [details] Patch
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?
(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
Would it be feasible to record the original value of the setting when it's overridden using RuntimeEnabledFeatures?
(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?
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.
Created attachment 283141 [details] Patch
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?
(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.
Created attachment 283305 [details] Patch
Brought back previous approach. I'll land it this PST afternoon if no objections arise.
Comment on attachment 283305 [details] Patch Go for it. Thanks Yoav.
Comment on attachment 283305 [details] Patch Clearing flags on attachment: 283305 Committed r203110: <http://trac.webkit.org/changeset/203110>
All reviewed patches have been landed. Closing bug.
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.