Bug 196289 – [WinCairo] storage/indexeddb tests are timing out

• Component: WebKit2
• Status: RESOLVED FIXED

[Fujii Hironori]

[curl] storage/indexeddb tests are timing out

Even though WK1 DumpRenderTree can pass the test cases, WK2 WKTR is timing out.

> python ./Tools/Scripts/run-webkit-tests --debug  --wincairo --no-new-test-results storage/indexeddb

[Fujii Hironori]

CacheStorageEngine doesn't work because following files are unimplemented yet.

Source/WebKit/NetworkProcess/cache/NetworkCacheDataCurl.cpp
Source/WebKit/NetworkProcess/cache/NetworkCacheIOChannelCurl.cpp

[Fujii Hironori]

Created attachment 366805 [details]
WIP patch

[Fujii Hironori]

Created attachment 367027 [details]
WIP patch

[Don Olmstead]

Is it possible to use more of FileSystem and file mapping to make the network cache platform agnostic? It feels like some of your patch is going this direction so I figured I'd just ask your opinion on it.

[Fujii Hironori]

Yeah, I have the same question in my mind.

[Fujii Hironori]

Created attachment 367926 [details]
Patch

[EWS Watchlist]

Comment on attachment 367926 [details]
Patch

Attachment 367926 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11964268

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-within-sw.https.html
http/tests/cache-storage/cache-origins.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-keys-attributes-for-service-worker.https.html
http/tests/cache-storage/cache-persistency.https.html

[EWS Watchlist]

Created attachment 367983 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 367926 [details]
Patch

Attachment 367926 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11967880

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-within-sw.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-keys-attributes-for-service-worker.https.html
http/tests/cache-storage/cache-origins.https.html
http/tests/cache-storage/cache-records-persistency.https.html
http/tests/cache-storage/cache-persistency.https.html

[EWS Watchlist]

Created attachment 368006 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Fujii Hironori]

Created attachment 369239 [details]
Patch

[Fujii Hironori]

Created attachment 369244 [details]
Patch

[Fujii Hironori]

Created attachment 369248 [details]
Patch

[Fujii Hironori]

Created attachment 369250 [details]
Patch

[EWS Watchlist]

Comment on attachment 369250 [details]
Patch

Attachment 369250 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12120984

New failing tests:
http/tests/cache-storage/cache-origins.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-match.https.html
http/tests/cache-storage/cache-records-persistency.https.html
http/tests/cache-storage/cache-persistency.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-within-sw.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-keys-attributes-for-service-worker.https.html

[EWS Watchlist]

Created attachment 369256 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Fujii Hironori]

Created attachment 369260 [details]
Patch

[EWS Watchlist]

Comment on attachment 369260 [details]
Patch

Attachment 369260 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12121879

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html

[EWS Watchlist]

Created attachment 369271 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Fujii Hironori]

Created attachment 369273 [details]
Patch

[EWS Watchlist]

Comment on attachment 369273 [details]
Patch

Attachment 369273 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12122573

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html

[EWS Watchlist]

Created attachment 369276 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369273 [details]
Patch

Attachment 369273 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12122866

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star-with-AllowContentSecurityPolicySourceStarToMatchAnyProtocol-enabled.html

[EWS Watchlist]

Created attachment 369278 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Alex Christensen]

Comment on attachment 369273 [details]
Patch

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

> Source/WTF/wtf/win/FileSystemWin.cpp:506
> +bool hardLink(const String& source, const String& destination)
> +{
> +    return CreateHardLink(destination.wideCharacters().data(), source.wideCharacters().data(), nullptr);
> +}
> +
>  bool hardLinkOrCopyFile(const String& source, const String& destination)
>  {
>      return !!::CopyFile(source.wideCharacters().data(), destination.wideCharacters().data(), TRUE);

Why not just implement hardLinkOrCopyFile like other platforms?

[EWS Watchlist]

Comment on attachment 369273 [details]
Patch

Attachment 369273 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12123492

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html

[EWS Watchlist]

Created attachment 369288 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4

[Don Olmstead]

There's some similar code to what's happening in NetworkCacheData.cpp in MemoryMappedFile within FileSystem. There isn't a Windows implementation at all but would it make sense to maybe expand on the MemoryMappedFile code and then just use that in data? This is definitely better but there's still a lot of Windows specific code happening.

[Fujii Hironori]

Comment on attachment 369273 [details]
Patch

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

>> Source/WTF/wtf/win/FileSystemWin.cpp:506
>>      return !!::CopyFile(source.wideCharacters().data(), destination.wideCharacters().data(), TRUE);
> 
> Why not just implement hardLinkOrCopyFile like other platforms?

They are used for different usages.

hardLinkOrCopyFile is used only in
SQLiteIDBTransaction::moveBlobFilesIfNecessary. (Bug 156321)

https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.cpp#L94

It is no problem for moveBlobFilesIfNecessary to use copying
becuase it will remove the source file soon.

On the other hand, BlobStorage::add should use hard linking
because BlobStorage unifies files which has the identical
content.

I don't know why moveBlobFilesIfNecessary needs the fallback of
using copy if hard linking fails. Is it possible to use only hard
linking even for moveBlobFilesIfNecessary case?

[Fujii Hironori]

(In reply to Don Olmstead from comment #28)

Sounds a good idea. Filed in Bug 197684.

[Fujii Hironori]

Created attachment 369358 [details]
Patch

[Alex Christensen]

(In reply to Fujii Hironori from comment #29)
> I don't know why moveBlobFilesIfNecessary needs the fallback of
> using copy if hard linking fails. Is it possible to use only hard
> linking even for moveBlobFilesIfNecessary case?
hardLinkOrCopyFile uses hard linking because it's faster then falls back to copying if hard linking fails, such as if the source and destination are on different drives.  It is a performance optimization to hard link.

[Alex Christensen]

Comment on attachment 369358 [details]
Patch

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

> Source/WTF/wtf/glib/FileSystemGlib.cpp:432
> +bool hardLink(const String& source, const String& destination)

hardLinkOrCopyFile could call this to reduce duplicate code.

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:447
> +bool hardLink(const String& source, const String& destination)

ditto

> Source/WebKit/NetworkProcess/cache/NetworkCacheData.h:78
> +#if !OS(WINDOWS)

I think we should not have so many macros in this header.  Having non-inline functions in the header would move these macros to the implementation files and make this file much tidier

[Fujii Hironori]

Created attachment 369457 [details]
Patch

* Addressed review feedbacks.

[Alex Christensen]

Comment on attachment 369457 [details]
Patch

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

> Source/WTF/wtf/glib/FileSystemGlib.cpp:456
> +    return !!::CopyFile(source.charactersWithNullTermination().data(), destination.charactersWithNullTermination().data(), TRUE);

Why do we use charactersWithNullTermination here...

> Source/WTF/wtf/win/FileSystemWin.cpp:510
>      return !!::CopyFile(source.wideCharacters().data(), destination.wideCharacters().data(), TRUE);

...but wideCharacters here?

[Fujii Hironori]

Comment on attachment 369457 [details]
Patch

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

>> Source/WTF/wtf/glib/FileSystemGlib.cpp:456
>> +    return !!::CopyFile(source.charactersWithNullTermination().data(), destination.charactersWithNullTermination().data(), TRUE);
> 
> Why do we use charactersWithNullTermination here...

Good catch. It should be wideCharacters. We should use wideCharacters for Win32 API after ICU 59.

[Fujii Hironori]

Created attachment 369539 [details]
Patch

* Addressed review feedbacks.

[WebKit Commit Bot]

Comment on attachment 369539 [details]
Patch

Clearing flags on attachment: 369539

Committed r245186: <https://trac.webkit.org/changeset/245186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50671610>