Bug 174720 – Create mock ITP data in testing to prevent grandfathering

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

We should create a mock full_browsing_session_resourceLog.plist when running layout tests to prevent ITP from grandfathering data every time it runs.

[Radar WebKit Bug Importer]

<rdar://problem/33457779>

[Jonathan Bedard]

Created attachment 316115 [details]
Patch

[Jonathan Bedard]

Comment on attachment 316115 [details]
Patch

I would like to wait till John gets back to land this.

[Darin Adler]

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.

[Jonathan Bedard]

Created attachment 316306 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Jonathan Bedard]

Created attachment 317187 [details]
Patch

[Jonathan Bedard]

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.

[John Wilander]

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.

[Build Bot]

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

[Build Bot]

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

[Jonathan Bedard]

(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.

[Daniel Bates]

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?

[Daniel Bates]

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.

[Brent Fulgham]

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?

[Jonathan Bedard]

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.

[Jonathan Bedard]

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.

[Daniel Bates]

(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 :(

[Jonathan Bedard]

Created attachment 317290 [details]
Patch

[Brent Fulgham]

Comment on attachment 317290 [details]
Patch

Lgtm. r=me.

[WebKit Commit Bot]

Comment on attachment 317290 [details]
Patch

Clearing flags on attachment: 317290

Committed r220604: <http://trac.webkit.org/changeset/220604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 317940 [details]
Patch

[Jonathan Bedard]

(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.

[WebKit Commit Bot]

Comment on attachment 317940 [details]
Patch

Clearing flags on attachment: 317940

Committed r220605: <http://trac.webkit.org/changeset/220605>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.