We should create a mock full_browsing_session_resourceLog.plist when running layout tests to prevent ITP from grandfathering data every time it runs.
<rdar://problem/33457779>
Created attachment 316115 [details] Patch
Comment on attachment 316115 [details] Patch I would like to wait till John gets back to land this.
Comment on attachment 316115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316115&action=review > Source/WebCore/testing/js/WebCoreTestSupport.h:45 > +bool makeAllDirectories(const WTF::String& path); This is not right. We don’t just re-declare functions in a different header so we can use them in tests.
Created attachment 316306 [details] Patch
Comment on attachment 316306 [details] Patch Attachment 316306 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4180486 New failing tests: media/controls-should-not-trigger-isolates-blending.html compositing/overflow/paint-neg-z-order-descendants-into-scrolling-contents-layer.html
Created attachment 316317 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 317187 [details] Patch
After talking with John, we should be able to get away with just a version number to prevent grandfathering. We also need to revisit how we handle plugins for testing.
I believe this should do the trick. As Jonathan and I discussed, it would be neat to have a test case that failed if this file wasn't loaded so we can detect that. But there were challenges in that the test runner resets ITP state between runs.
Comment on attachment 317187 [details] Patch Attachment 317187 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4250435 New failing tests: media/controls-strict.html
Created attachment 317210 [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.12.5
(In reply to Build Bot from comment #12) > Created attachment 317210 [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.12.5 Just a note: I've spoke with Ryan, yesterday evening there was a string of undiagnosed flakiness on our iOS Sim EWS bots, this is one of the runs exhibiting that flakiness.
Comment on attachment 317187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317187&action=review > Tools/WebKitTestRunner/TestController.cpp:424 > +static void writeMockResourceLoadStatistics(const String& resourceLoadStatisticsFolder) > +{ > +#if PLATFORM(WIN) > + SHCreateDirectoryEx(0, (char*) resourceLoadStatisticsFolder.characters8(), 0); > +#else > + mkdir((char*) resourceLoadStatisticsFolder.characters8(), 0755); > +#endif > + > + String fullBrowsingSessionResourceLog = resourceLoadStatisticsFolder + '/' + "full_browsing_session_resourceLog.plist"; > + FILE* tempFile = fopen((char*) fullBrowsingSessionResourceLog.characters8(), "w"); > + if (!tempFile) > + WTFCrash(); > + fprintf(tempFile, "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"); > + fprintf(tempFile, "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"); > + fprintf(tempFile, "<plist version=\"1.0\">\n"); > + fprintf(tempFile, "<dict>\n"); > > + fprintf(tempFile, "<key>version</key>\n"); > + fprintf(tempFile, "<integer>1</integer>\n"); > + > + fprintf(tempFile, "</dict>\n"); > + fprintf(tempFile, "</plist>\n"); > + fclose(tempFile); > +} This file is a port-independent file. This is not the appropriate place for such port-specific logic. It should be in a port-specific file. > Tools/WebKitTestRunner/TestController.cpp:447 > + writeMockResourceLoadStatistics(resourceLoadStatisticsFolder); Is it necessary to do this every time we create a new web view? Could the port-specific class(es) for run-webkit-tests create this file as part of setting up the test tool environment?
I'm unclear what problem this bug is trying to solve. Why do we want to "prevent ITP from grandfathering data every time" we run layout tests? Neither the ChangeLog nor the description of this bug (comment #0) describe the problem we are trying to solve. They describe a solution.
Comment on attachment 317187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317187&action=review >> Tools/WebKitTestRunner/TestController.cpp:424 >> +} > > This file is a port-independent file. This is not the appropriate place for such port-specific logic. It should be in a port-specific file. Maybe this code should be in "TestControllerCocoa.mm" >> Tools/WebKitTestRunner/TestController.cpp:447 >> + writeMockResourceLoadStatistics(resourceLoadStatisticsFolder); > > Is it necessary to do this every time we create a new web view? Could the port-specific class(es) for run-webkit-tests create this file as part of setting up the test tool environment? I agree --maybe add a call right at the end here: "platformGenerateContextConfiguration()", and have the implementation in TestControllerCocoa, with stubs for other platforms?
Comment on attachment 317187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317187&action=review >>> Tools/WebKitTestRunner/TestController.cpp:424 >>> +} >> >> This file is a port-independent file. This is not the appropriate place for such port-specific logic. It should be in a port-specific file. > > Maybe this code should be in "TestControllerCocoa.mm" I know we don't have cookie partitioning on other ports, but I don't see anything in ResourceLoadStatisticsPersistentStorage.cpp that would make this file port specific.
I just spoke with Brady and Dan today (8/4) and we addressed some of Dan's questions, I'm going to include a brief summary here. - The feature is not platform specific, although, Mac and iOS are the only ones which implement it, so the plist code should be platform specific - It is technically possible to do this in webkitpy, but since it needs to occur at the start of EVERY test-runner (instead of at the start of ALL test runners) and since it can effect those running WebKitTestRunner directly, this belongs in the test runner. - The problem which started this investigation is that grandfathering starts the plugin process. A few minutes later, the plugin process will unload any loaded plugin. These loaded plugins may print some output when they unload, which can cause later tests running in the same instance of the test runner to fail. - We could add a switch to specifically disable grandfathering, but this seems like an unnecessary addition since there is a well-defined state which will achieve the same end.
(In reply to Jonathan Bedard from comment #18) > I just spoke with Brady and Dan today (8/4) and we addressed some of Dan's > questions, I'm going to include a brief summary here. > > - The feature is not platform specific, although, Mac and iOS are the only > ones which implement it, so the plist code should be platform specific > - It is technically possible to do this in webkitpy, but since it needs to > occur at the start of EVERY test-runner (instead of at the start of ALL test > runners) This is not a problem for webkitpy. The driver always sets up the environment before invoking the test tool, including creating the temporary directory and setting the environment variable DUMPRENDERTREE_TEMP that the test tool depends on (and depended on in attachment #317187 [details]) > and since it can effect those running WebKitTestRunner directly, > this belongs in the test runner. This is the reason we cannot create the plist file in webkitpy :(
Created attachment 317290 [details] Patch
Comment on attachment 317290 [details] Patch Lgtm. r=me.
Comment on attachment 317290 [details] Patch Clearing flags on attachment: 317290 Committed r220604: <http://trac.webkit.org/changeset/220604>
All reviewed patches have been landed. Closing bug.
Comment on attachment 317290 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317290&action=review > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:105 > + NSDictionary *resourceLogPlist = [[NSDictionary alloc] initWithObjectsAndKeys: [NSNumber numberWithInt:1], @"version", nil]; You're leaking resourceLogPlist
Reopening to attach new patch.
Created attachment 317940 [details] Patch
(In reply to Simon Fraser (smfr) from comment #24) > Comment on attachment 317290 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=317290&action=review > > > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:105 > > + NSDictionary *resourceLogPlist = [[NSDictionary alloc] initWithObjectsAndKeys: [NSNumber numberWithInt:1], @"version", nil]; > > You're leaking resourceLogPlist Good catch. Fixing.
Comment on attachment 317940 [details] Patch Clearing flags on attachment: 317940 Committed r220605: <http://trac.webkit.org/changeset/220605>