Bug 163013 – [Win] Parallel DRTs are sharing preferences and cache.

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

[Per Arne Vollan]

This is causing flakiness in http layout tests.

[Per Arne Vollan]

Created attachment 290824 [details]
Patch

[Alex Christensen]

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.

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 291086 [details]
Patch

[WebKit Commit Bot]

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.

[Per Arne Vollan]

Created attachment 291088 [details]
Patch

[Per Arne Vollan]

Created attachment 291089 [details]
Patch

[Brent Fulgham]

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.

[Per Arne Vollan]

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

[WebKit Commit Bot]

Comment on attachment 291089 [details]
Patch

Clearing flags on attachment: 291089

Committed r207067: <http://trac.webkit.org/changeset/207067>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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'

[Ryan Haddad]

Reverted r207067 for reason:

This change caused webkitpy test failures.

Committed r207144: <http://trac.webkit.org/changeset/207144>

[Per Arne Vollan]

Created attachment 291344 [details]
Patch

[Brent Fulgham]

Comment on attachment 291344 [details]
Patch

Looks good. Let's try it again!

[WebKit Commit Bot]

Comment on attachment 291344 [details]
Patch

Clearing flags on attachment: 291344

Committed r207218: <http://trac.webkit.org/changeset/207218>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

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]

[Said Abou-Hallawa]

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.

[Brent Fulgham]

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

[Brent Fulgham]

Build fix landed in r207223.

Committed r207223: <http://trac.webkit.org/changeset/207223>

[Per Arne Vollan]

(In reply to comment #21)
> Build fix landed in r207223.
> 
> Committed r207223: <http://trac.webkit.org/changeset/207223>

Thanks!

[Said Abou-Hallawa]

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]

[Brent Fulgham]

I missed one of the CFNETWORK -> CFURLSESSION changes:

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