Bug 217880 – [Testing] Convert DumpRenderTree to use generated test header options

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Sam Weinig]

[Testing] Convert DumpRenderTree to use generated test header options

[Sam Weinig]

Created attachment 411678 [details]
Patch

[Sam Weinig]

Created attachment 411680 [details]
Patch

[Sam Weinig]

Created attachment 411688 [details]
Patch

[Sam Weinig]

Going to try making the VPATH include the erb files.

[Sam Weinig]

If anyone has any idea what I am doing wrong with these Makefile rules, I could really use a pointer.

[Sam Weinig]

Specifically, it's these errors:
    /bin/sh -c \"/Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/WebKitBuild/DumpRenderTree.build/Release-appletvsimulator/Derived\ Sources.build/Script-0F18E7011D6B9CC60027E547.sh\"
make: Circular /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGeneratedWebKitLegacyKeyMapping.cpp.erb.cpp <- /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGeneratedWebKitLegacyKeyMapping.cpp.erb dependency dropped.
make: Circular /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGeneratedKeys.h.erb.sh <- /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGene
ratedWebKitLegacyKeyMapping.cpp.erb dependency dropped.
make: Circular /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGeneratedKeys.h.erb.sh <- /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGeneratedKeys.h.erb dependency dropped.
ruby /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/WebKitBuild/Release-appletvsimulator/usr/local/include/wtf/Scripts/GeneratePreferences.rb --frontend WebKitLegacy --base /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/WebKitBuild/Release-appletvsimulator/usr/local/include/wtf/Scripts/Preferences/WebPreferences.yaml --debug /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/WebKitBuild/Release-appletvsimulator/usr/local/include/wtf/Scripts/Preferences/WebPreferencesDebug.yaml --experimental /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/WebKitBuild/Release-appletvsimulator/usr/local/include/wtf/Scripts/Preferences/WebPreferencesExperimental.yaml --internal /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/WebKitBuild/Release-appletvsimulator/usr/local/include/wtf/Scripts/Preferences/WebPreferencesInternal.yaml --template /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGeneratedWebKitLegacyKeyMapping.cpp.erb --template /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGeneratedKeys.h.erb
cat /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGeneratedKeys.h.erb.sh >/Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGeneratedKeys.h.erb 
cat: /Volumes/Data/worker/tvOS-14-Simulator-Build-EWS/build/Tools/DumpRenderTree/Scripts/PreferencesTemplates/TestOptionsGeneratedKeys.h.erb.sh: No such file or directory


I don't understand where the .sh suffixes are coming from, but I am clearly doing something quite wrong.

[Darin Adler]

I don’t see a "cat" command anywhere in DerivedSources.make.

[Darin Adler]

Comment on attachment 411688 [details]
Patch

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

Trying to figure this out but so far nothing.

> Tools/DumpRenderTree/DerivedSources.make:32
> +VPATH = \
> +    $(UISCRIPTCONTEXT_DIR) \
> +    $(PREFERENCES_TEMPLATE_DIR) \
> +#

I am not sure what we are trying to accomplish here. VPATH tells make where to *search* for prerequisites. I don’t think we need a VPATH to make a rule work when the prerequisites are full paths, so I don’t see why PREFERENCES_TEMPLATE_DIR needs to be in a VPATH.

> Tools/DumpRenderTree/DerivedSources.make:46
>  WEB_PREFERENCES_FILES = $(basename $(notdir $(WEB_PREFERENCES_TEMPLATES)))

WEB_PREFERENCES_FILES will be:

    TestOptionsGeneratedWebKitLegacyKeyMapping.cpp TestOptionsGeneratedKeys.h

> Tools/DumpRenderTree/DerivedSources.make:47
> +WEB_PREFERENCES_PATTERNS = $(subst .,%,$(WEB_PREFERENCES_FILES))

WEB_PREFERENCES_PATTERNS will be:

        TestOptionsGeneratedWebKitLegacyKeyMapping%cpp TestOptionsGeneratedKeys%h

> Tools/DumpRenderTree/DerivedSources.make:49
>  all : $(WEB_PREFERENCES_FILES)

A little strange to have this *before* the .PHONY : all rule below.

[Sam Weinig]

Created attachment 411689 [details]
Patch

[Sam Weinig]

Try a more traditional Makefile pattern rule here:

%.cpp : %.cpp.erb $(WTF_BUILD_SCRIPTS_DIR)/GeneratePreferences.rb $(WEB_PREFERENCES_TEMPLATES) $(WEB_PREFERENCES)
	$(RUBY) $(WTF_BUILD_SCRIPTS_DIR)/GeneratePreferences.rb --frontend WebKitLegacy --base ${WTF_BUILD_SCRIPTS_DIR}/Preferences/WebPreferences.yaml --debug ${WTF_BUILD_SCRIPTS_DIR}/Preferences/WebPreferencesDebug.yaml --experimental ${WTF_BUILD_SCRIPTS_DIR}/Preferences/WebPreferencesExperimental.yaml --internal ${WTF_BUILD_SCRIPTS_DIR}/Preferences/WebPreferencesInternal.yaml --template $<

%.h : %.h.erb $(WTF_BUILD_SCRIPTS_DIR)/GeneratePreferences.rb $(WEB_PREFERENCES_TEMPLATES) $(WEB_PREFERENCES)
	$(RUBY) $(WTF_BUILD_SCRIPTS_DIR)/GeneratePreferences.rb --frontend WebKitLegacy --base ${WTF_BUILD_SCRIPTS_DIR}/Preferences/WebPreferences.yaml --debug ${WTF_BUILD_SCRIPTS_DIR}/Preferences/WebPreferencesDebug.yaml --experimental ${WTF_BUILD_SCRIPTS_DIR}/Preferences/WebPreferencesExperimental.yaml --internal ${WTF_BUILD_SCRIPTS_DIR}/Preferences/WebPreferencesInternal.yaml --template $<


I'm not sure what the right way to say the output could either end in a .h or a .cpp here

[Sam Weinig]

Created attachment 411693 [details]
Patch

[Sam Weinig]

This one has make being invoked with make -d, see if we can find anything interesting from the debug logging make has.

[Sam Weinig]

Created attachment 411695 [details]
Patch

[Sam Weinig]

Created attachment 411706 [details]
Patch

[Sam Weinig]

Created attachment 411707 [details]
Patch

[Sam Weinig]

Created attachment 411709 [details]
Patch

[Sam Weinig]

Yay! I think the static patterns work! Never used those before, but they seem like a good fit here.

[Darin Adler]

Yes, that does seem like a great solution. Less exotic than the others, too.

[Sam Weinig]

(In reply to Darin Adler from comment #18)
> Yes, that does seem like a great solution. Less exotic than the others, too.

Now just to fix the test failures.

[Sam Weinig]

Created attachment 411711 [details]
Patch

[Sam Weinig]

Created attachment 411712 [details]
Patch

[Sam Weinig]

Created attachment 411713 [details]
Patch

[Sam Weinig]

Created attachment 411714 [details]
Patch

[Sam Weinig]

Created attachment 411715 [details]
Patch

[Sam Weinig]

Created attachment 411716 [details]
Patch

[Sam Weinig]

Created attachment 411723 [details]
Patch

[Sam Weinig]

Created attachment 411724 [details]
Patch

[Darin Adler]

Looks like those tests are passing.

[Sam Weinig]

(In reply to Darin Adler from comment #28)
> Looks like those tests are passing.

The Mac-wk1 bots seem to have hit some issues extracting results, and seem to still be failing. I reached out to the bot-watchers to see what is up.

[Aakash Jain]

(In reply to Sam Weinig from comment #29)
> The Mac-wk1 bots seem to have hit some issues extracting results, and seem
> to still be failing. I reached out to the bot-watchers to see what is up.
mac-wk1 and mac-debug-wk1 seems to be consistently failing with 30+ failures. Also the layout-test runs for this patch on these queues have 500,000+ log lines which is unusually high (~10x the usual). Please have a look.

e.g.:
https://ews-build.webkit.org/#/builders/30/builds/20930
https://ews-build.webkit.org/#/builders/30/builds/20900
https://ews-build.webkit.org/#/builders/30/builds/20894
https://ews-build.webkit.org/#/builders/32/builds/20683
https://ews-build.webkit.org/#/builders/32/builds/20675

[Sam Weinig]

Created attachment 412186 [details]
Patch

[Sam Weinig]

Created attachment 412189 [details]
Patch

[Darin Adler]

Comment on attachment 412189 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:2992
>  @end
>  
> +
> +@implementation WebPreferences (WebPrivateTesting)

Maybe just one blank line here?

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3004
> +    RetainPtr<CFHTTPCookieStorageRef> cookieStorage = NetworkStorageSessionMap::defaultStorageSession().cookieStorage();

auto

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3005
> +    ASSERT(cookieStorage); // Will fail when NetworkStorageSessionMap::switchToNewTestingSession() was not called beforehand.

Maybe RELEASE_ASSERT? Certainly harmless to do that, and can’t say that about all ASSERT.

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:306
> +@interface WebPreferences (WebPrivateTesting)

I suggest either a blank line after @interface or no blank line before @end.

[Sam Weinig]

Created attachment 412190 [details]
Patch

[Sam Weinig]

Created attachment 412191 [details]
Patch

[Sam Weinig]

hm, something is just destroying the tests here. 

09:56:21.563 29570 worker/9 animations/font-variations/font-stretch.html output stderr lines:
09:56:21.563 29570   2020-10-23 09:56:20.919 DumpRenderTree[29573:69131] NetworkStorageDB:_openDBReadConnections: failed to open read connection to DB @ (null)/Cache.db.  Error=14. Cause=unable to open database file
09:56:21.563 29570   2020-10-23 09:56:20.919 DumpRenderTree[29573:69131] CacheRead: unable to open cache files in (null)
09:56:21.563 29570   2020-10-23 09:56:21.436 DumpRenderTree[29573:69131] NetworkStorageDB:_openDBReadConnections: failed to open read connection to DB @ (null)/Cache.db.  Error=14. Cause=unable to open database file
09:56:21.563 29570   2020-10-23 09:56:21.436 DumpRenderTree[29573:69131] CacheRead: unable to open cache files in (null)
09:56:21.563 29570   2020-10-23 09:56:21.462 DumpRenderTree[29573:69131] NetworkStorageDB:_openDBReadConnections: failed to open read connection to DB @ (null)/Cache.db.  Error=14. Cause=unable to open database file
09:56:21.563 29570   2020-10-23 09:56:21.462 DumpRenderTree[29573:69131] CacheRead: unable to open cache files in (null)

I don't see this output when running locally, and can't find any references to NetworkStorageDB, so not sure what is going on yet.

[Sam Weinig]

Created attachment 412194 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #36)
> hm, something is just destroying the tests here. 
> 
> 09:56:21.563 29570 worker/9 animations/font-variations/font-stretch.html
> output stderr lines:
> 09:56:21.563 29570   2020-10-23 09:56:20.919 DumpRenderTree[29573:69131]
> NetworkStorageDB:_openDBReadConnections: failed to open read connection to
> DB @ (null)/Cache.db.  Error=14. Cause=unable to open database file
> 09:56:21.563 29570   2020-10-23 09:56:20.919 DumpRenderTree[29573:69131]
> CacheRead: unable to open cache files in (null)
> 09:56:21.563 29570   2020-10-23 09:56:21.436 DumpRenderTree[29573:69131]
> NetworkStorageDB:_openDBReadConnections: failed to open read connection to
> DB @ (null)/Cache.db.  Error=14. Cause=unable to open database file
> 09:56:21.563 29570   2020-10-23 09:56:21.436 DumpRenderTree[29573:69131]
> CacheRead: unable to open cache files in (null)
> 09:56:21.563 29570   2020-10-23 09:56:21.462 DumpRenderTree[29573:69131]
> NetworkStorageDB:_openDBReadConnections: failed to open read connection to
> DB @ (null)/Cache.db.  Error=14. Cause=unable to open database file
> 09:56:21.563 29570   2020-10-23 09:56:21.462 DumpRenderTree[29573:69131]
> CacheRead: unable to open cache files in (null)
> 
> I don't see this output when running locally, and can't find any references
> to NetworkStorageDB, so not sure what is going on yet.

Finally found NetworkStorageDB in CFNetwork. I broke the cache somehow here.

[Sam Weinig]

(In reply to Sam Weinig from comment #38)
> (In reply to Sam Weinig from comment #36)
> > hm, something is just destroying the tests here. 
> > 
> > 09:56:21.563 29570 worker/9 animations/font-variations/font-stretch.html
> > output stderr lines:
> > 09:56:21.563 29570   2020-10-23 09:56:20.919 DumpRenderTree[29573:69131]
> > NetworkStorageDB:_openDBReadConnections: failed to open read connection to
> > DB @ (null)/Cache.db.  Error=14. Cause=unable to open database file
> > 09:56:21.563 29570   2020-10-23 09:56:20.919 DumpRenderTree[29573:69131]
> > CacheRead: unable to open cache files in (null)
> > 09:56:21.563 29570   2020-10-23 09:56:21.436 DumpRenderTree[29573:69131]
> > NetworkStorageDB:_openDBReadConnections: failed to open read connection to
> > DB @ (null)/Cache.db.  Error=14. Cause=unable to open database file
> > 09:56:21.563 29570   2020-10-23 09:56:21.436 DumpRenderTree[29573:69131]
> > CacheRead: unable to open cache files in (null)
> > 09:56:21.563 29570   2020-10-23 09:56:21.462 DumpRenderTree[29573:69131]
> > NetworkStorageDB:_openDBReadConnections: failed to open read connection to
> > DB @ (null)/Cache.db.  Error=14. Cause=unable to open database file
> > 09:56:21.563 29570   2020-10-23 09:56:21.462 DumpRenderTree[29573:69131]
> > CacheRead: unable to open cache files in (null)
> > 
> > I don't see this output when running locally, and can't find any references
> > to NetworkStorageDB, so not sure what is going on yet.
> 
> Finally found NetworkStorageDB in CFNetwork. I broke the cache somehow here.

Alex, Antti, Kilzer,

Do any of you know what might be causing this? Seems like the Cache.db path is getting set to null. But I can't quite figure out why (and for unrelated reasons, can't easily debug it locally.)

[Alex Christensen]

Comment on attachment 412194 [details]
Patch

WebKitLegacy networking was mostly before my time, but I would look closely at the call to switchToNewTestingSession and the call to _resetToDefaultValuesForTesting looks  like a suspicious change in behavior.

[Sam Weinig]

Created attachment 412219 [details]
Patch

[Alex Christensen]

Comment on attachment 412219 [details]
Patch

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

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:855
> +    // [preferences _resetToDefaultValuesForTesting];

This seems to have done it.  Please don't commit this, though.

[Sam Weinig]

(In reply to Alex Christensen from comment #42)
> Comment on attachment 412219 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412219&action=review
> 
> > Tools/DumpRenderTree/mac/DumpRenderTree.mm:855
> > +    // [preferences _resetToDefaultValuesForTesting];
> 
> This seems to have done it.  Please don't commit this, though.

Oh, I'm very well aware it's the reset that is breaking things (and won't commit until I figure it out and fix it), I've done a variety of different reset methods and they all cause the breakage. Ordinarily I would just debug it, but can't at the moment, so am doing triage via the bots :(.

[Sam Weinig]

Created attachment 412243 [details]
Patch

[Sam Weinig]

Created attachment 412244 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/70653177>

[Sam Weinig]

Created attachment 412252 [details]
Patch

[Sam Weinig]

Created attachment 412253 [details]
Experiment: Remove setSharedURLCache  and add _resetToDefaultValuesForTesting

[Sam Weinig]

Created attachment 412254 [details]
Experiment: Remove setSharedURLCache and new WebPreferences per test

[Sam Weinig]

Created attachment 412256 [details]
Patch

[Sam Weinig]

Created attachment 412264 [details]
Patch

[Sam Weinig]

This ended up being done in via other changes in a slightly different way.