Bug 163013

Summary: [Win] Parallel DRTs are sharing preferences and cache.
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: Tools / TestsAssignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, glenn, lforschler, ryanhaddad, sabouhallawa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Per Arne Vollan 2016-10-06 08:00:54 PDT 
This is causing flakiness in http layout tests.
Comment 1 Per Arne Vollan 2016-10-06 08:22:24 PDT 
Created attachment 290824 [details]
Patch
Comment 2 Alex Christensen 2016-10-06 08:47:50 PDT 
Comment on attachment 290824 [details]
Patch

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

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:867
> +    _declspec(dllimport) CFStringRef applicationId;

I like the idea, but instead of all this extern and dllimport we should make a function for setting the applicationID.
Comment 3 Per Arne Vollan 2016-10-06 09:55:21 PDT 
(In reply to comment #2)
> Comment on attachment 290824 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290824&action=review
> 
> > Tools/DumpRenderTree/win/DumpRenderTree.cpp:867
> > +    _declspec(dllimport) CFStringRef applicationId;
> 
> I like the idea, but instead of all this extern and dllimport we should make
> a function for setting the applicationID.

Thanks, good point! I'll update the patch :)
Comment 4 Per Arne Vollan 2016-10-10 04:17:23 PDT 
Created attachment 291086 [details]
Patch
Comment 5 WebKit Commit Bot 2016-10-10 04:18:30 PDT 
Attachment 291086 [details] did not pass style-queue:


ERROR: Source/WebKit/win/WebPreferences.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Per Arne Vollan 2016-10-10 04:58:37 PDT 
Created attachment 291088 [details]
Patch
Comment 7 Per Arne Vollan 2016-10-10 05:02:53 PDT 
Created attachment 291089 [details]
Patch
Comment 8 Brent Fulgham 2016-10-10 10:03:09 PDT 
Comment on attachment 291089 [details]
Patch

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

Very, very nice!

> Tools/Scripts/webkitpy/port/win.py:356
> +        preferences_files = self._filesystem.join(os.environ['APPDATA'], "Apple Computer/Preferences", "com.apple.DumpRenderTree*")

We should not use "Apple Computer" in our naming anymore. We should use "Apple Inc."

Frankly, for WebKit-specific stuff like a test harness, I think it would be better to use "WebKit" for storing things in the registry.

However, since we still have "Apple Computer" listed in "WEBCORE_NAVIGATOR_VENDOR" in WebCore, we should stay consistent. But I think we should propose another patch to make a blanket change from "Apple Computer" -> "Apple Inc." in the project.
Comment 9 Per Arne Vollan 2016-10-11 01:30:47 PDT 
(In reply to comment #8)
> Comment on attachment 291089 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291089&action=review
> 
> Very, very nice!
> 
> > Tools/Scripts/webkitpy/port/win.py:356
> > +        preferences_files = self._filesystem.join(os.environ['APPDATA'], "Apple Computer/Preferences", "com.apple.DumpRenderTree*")
> 
> We should not use "Apple Computer" in our naming anymore. We should use
> "Apple Inc."
> 
> Frankly, for WebKit-specific stuff like a test harness, I think it would be
> better to use "WebKit" for storing things in the registry.
> 
> However, since we still have "Apple Computer" listed in
> "WEBCORE_NAVIGATOR_VENDOR" in WebCore, we should stay consistent. But I
> think we should propose another patch to make a blanket change from "Apple
> Computer" -> "Apple Inc." in the project.

Yes, I believe this should also be changed in the various libraries we are using.

Thanks for reviewing!
Comment 10 WebKit Commit Bot 2016-10-11 01:38:21 PDT 
Comment on attachment 291089 [details]
Patch

Clearing flags on attachment: 291089

Committed r207067: <http://trac.webkit.org/changeset/207067>
Comment 11 WebKit Commit Bot 2016-10-11 01:38:25 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 12 Ryan Haddad 2016-10-11 08:14:01 PDT 
This change appears to have caused two webkitpy tests to fail:

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/510/steps/webkitpy-test/logs/stdio

[1004/1504] webkitpy.port.win_unittest.WinPortTest.test_diff_image erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/port/port_testcase.py", line 260, in test_diff_image
      port.setup_test_run()
    File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/port/win.py", line 356, in setup_test_run
      preferences_files = self._filesystem.join(os.environ['APPDATA'], "Apple Computer/Preferences", "com.apple.DumpRenderTree*")
  KeyError: 'APPDATA'

[1040/1504] webkitpy.port.win_unittest.WinPortTest.test_diff_image_crashed erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/port/port_testcase.py", line 287, in test_diff_image_crashed
      port.setup_test_run()
    File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/port/win.py", line 356, in setup_test_run
      preferences_files = self._filesystem.join(os.environ['APPDATA'], "Apple Computer/Preferences", "com.apple.DumpRenderTree*")
    File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/UserDict.py", line 23, in __getitem__
      raise KeyError(key)
  KeyError: 'APPDATA'
Comment 13 Ryan Haddad 2016-10-11 09:36:08 PDT 
Reverted r207067 for reason:

This change caused webkitpy test failures.

Committed r207144: <http://trac.webkit.org/changeset/207144>
Comment 14 Per Arne Vollan 2016-10-12 01:08:17 PDT 
Created attachment 291344 [details]
Patch
Comment 15 Brent Fulgham 2016-10-12 09:15:53 PDT 
Comment on attachment 291344 [details]
Patch

Looks good. Let's try it again!
Comment 16 WebKit Commit Bot 2016-10-12 09:38:48 PDT 
Comment on attachment 291344 [details]
Patch

Clearing flags on attachment: 291344

Committed r207218: <http://trac.webkit.org/changeset/207218>
Comment 17 WebKit Commit Bot 2016-10-12 09:38:53 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 18 Said Abou-Hallawa 2016-10-12 10:19:17 PDT 
This (In reply to comment #16)
> Comment on attachment 291344 [details]
> Patch
> 
> Clearing flags on attachment: 291344
> 
> Committed r207218: <http://trac.webkit.org/changeset/207218>

It looks like this patch broke the windows build:

c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webcache.cpp(256): error C4716: 'WebCache::cacheFolder': must return a value [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webcache.cpp(272): error C4716: 'WebCache::setCacheFolder': must return a value [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
Comment 19 Said Abou-Hallawa 2016-10-12 10:22:26 PDT 
Comment on attachment 291344 [details]
Patch

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

> Source/WebKit/win/WebCache.cpp:254
> +    return S_OK;

I think we need to keep:

#else
    return E_NOTIMPL;

before #endif

> Source/WebKit/win/WebCache.cpp:270
> +    return S_OK;

Ditto.
Comment 20 Brent Fulgham 2016-10-12 10:23:30 PDT 
(In reply to comment #19)
> Comment on attachment 291344 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291344&action=review
> 
> > Source/WebKit/win/WebCache.cpp:254
> > +    return S_OK;
> 
> I think we need to keep:
> 
> #else
>     return E_NOTIMPL;

That's a fine change, but the real problem is that the patch predates the change from USE(CFNETWORK) -> USE(CFURLCONNECTION).

I'll check in a fix.
Comment 21 Brent Fulgham 2016-10-12 10:26:00 PDT 
Build fix landed in r207223.

Committed r207223: <http://trac.webkit.org/changeset/207223>
Comment 22 Per Arne Vollan 2016-10-12 10:37:58 PDT 
(In reply to comment #21)
> Build fix landed in r207223.
> 
> Committed r207223: <http://trac.webkit.org/changeset/207223>

Thanks!
Comment 23 Said Abou-Hallawa 2016-10-12 11:46:37 PDT 
The Windows build is still failing.

C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(252): error C3861: 'wkCopyFoundationCacheDirectory': identifier not found [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(267): error C2065: 'CFURLCacheRef': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(267): error C2923: 'WTF::RetainPtr': 'CFURLCacheRef' is not a valid template type argument for parameter 'T' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
  WebDatabaseManager.cpp
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(267): error C3861: 'CFURLCacheCopySharedURLCache': identifier not found [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(267): error C2512: 'WTF::RetainPtr': no appropriate default constructor available [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/RetainPtr.h(58): note: see declaration of 'WTF::RetainPtr' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp)
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(268): error C2662: 'std::remove_pointer<_Ty>::type *WTF::RetainPtr<T>::get(void) const': cannot convert 'this' pointer from 'WTF::RetainPtr' to 'const WTF::RetainPtr<T> &' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(268): note: Reason: cannot convert from 'WTF::RetainPtr' to 'const WTF::RetainPtr<T>'
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(268): note: Conversion requires a second user-defined-conversion operator or constructor
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(268): error C3861: 'CFURLCacheMemoryCapacity': identifier not found [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(269): error C2662: 'std::remove_pointer<_Ty>::type *WTF::RetainPtr<T>::get(void) const': cannot convert 'this' pointer from 'WTF::RetainPtr' to 'const WTF::RetainPtr<T> &' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(269): note: Reason: cannot convert from 'WTF::RetainPtr' to 'const WTF::RetainPtr<T>'
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(269): note: Conversion requires a second user-defined-conversion operator or constructor
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(269): error C3861: 'CFURLCacheDiskCapacity': identifier not found [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(270): error C2065: 'CFURLCacheRef': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(270): error C2923: 'WTF::RetainPtr': 'CFURLCacheRef' is not a valid template type argument for parameter 'T' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(270): error C3861: 'CFURLCacheCreate': identifier not found [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(270): error C2512: 'WTF::RetainPtr': no appropriate default constructor available [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/RetainPtr.h(58): note: see declaration of 'WTF::RetainPtr' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp)
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(271): error C2662: 'std::remove_pointer<_Ty>::type *WTF::RetainPtr<T>::get(void) const': cannot convert 'this' pointer from 'WTF::RetainPtr' to 'const WTF::RetainPtr<T> &' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(271): note: Reason: cannot convert from 'WTF::RetainPtr' to 'const WTF::RetainPtr<T>'
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(271): note: Conversion requires a second user-defined-conversion operator or constructor
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebCache.cpp(271): error C3861: 'CFURLCacheSetSharedURLCache': identifier not found [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
Comment 24 Brent Fulgham 2016-10-12 12:06:57 PDT 
I missed one of the CFNETWORK -> CFURLSESSION changes:

Committed r207231: <http://trac.webkit.org/changeset/207231>