Bug 209338

Summary: Ensure base cache path exists before calculating disk cache capacity
Product: WebKit Reporter: Lauro Moura <lmoura>
Component: WebKit2Assignee: Lauro Moura <lmoura>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, benjamin, bugs-noreply, cdumez, cgarcia, cmarcelo, dpino, ews-watchlist, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Retry getting cache capacity with glib
none
Updated patch fixing getVolumeFreeSpace instead
none
Fix sign-compare in testcase
cgarcia: review-, cgarcia: commit-queue-
Patch aperez: review+

Lauro Moura
Reported 2020-03-20 08:06:43 PDT
The EWS bot is using `--skip-failing-tests` to give faster feedback about the patche being tested, but this flag is causing some disk-cache tests to fail: Regressions: Unexpected text-only failures (15) http/tests/cache/disk-cache/disk-cache-media.html [ Failure ] http/tests/cache/disk-cache/disk-cache-range.html [ Failure ] http/tests/cache/disk-cache/disk-cache-redirect.html [ Failure ] http/tests/cache/disk-cache/disk-cache-request-headers.html [ Failure ] http/tests/cache/disk-cache/disk-cache-request-max-stale.html [ Failure ] http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header.html [ Failure ] http/tests/cache/disk-cache/disk-cache-validation-attachment.html [ Failure ] http/tests/cache/disk-cache/disk-cache-validation-back-navigation-policy.html [ Failure ] http/tests/cache/disk-cache/disk-cache-validation-no-body.html [ Failure ] http/tests/cache/disk-cache/disk-cache-validation.html [ Failure ] http/tests/cache/disk-cache/disk-cache-vary-no-body.html [ Failure ] http/tests/cache/disk-cache/disk-cache-vary.html [ Failure ] http/tests/cache/disk-cache/memory-cache-revalidation-updates-disk-cache.html [ Failure ] http/tests/cache/disk-cache/redirect-chain-limits.html [ Failure ] http/tests/cache/disk-cache/resource-becomes-uncacheable.html [ Failure ] Sample failure from disk-cache-media: +++ /home/lauro/dev/WebKit/layout-test-results-1/http/tests/cache/disk-cache/disk-cache-media-actual.txt @@ -10,7 +10,7 @@ response source: Network response headers: {"Cache-control":"max-age=100","Content-Type":"text/plain"} -response source: Disk cache +response source: Network response headers: {"Cache-control":"max-age=0","Content-Type":"video/mp4"} response source: Network Maybe there is some dependency among the tests, and not running the ones marked as failure is causing the others to fail.
Attachments
Retry getting cache capacity with glib (2.31 KB, patch)
2020-03-23 21:23 PDT, Lauro Moura 		no flags
DetailsFormatted DiffDiff
Updated patch fixing getVolumeFreeSpace instead (3.18 KB, patch)
2020-03-24 13:50 PDT, Lauro Moura 		no flags
DetailsFormatted DiffDiff
Fix sign-compare in testcase (3.23 KB, patch)
2020-03-26 18:46 PDT, Lauro Moura 		cgarcia: review-
cgarcia: commit-queue-
DetailsFormatted DiffDiff
Patch (1.71 KB, patch)
2020-04-08 02:46 PDT, Carlos Garcia Campos 		aperez: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Lauro Moura
Comment 1 2020-03-20 13:31:34 PDT
These failures are triggered when http/tests/cache/disk-cache/disk-cache-disable.html is not run. For example, running http/tests/cache/disk-cache/disk-cache-media.html after the disk-cache-disable.html causes it to work. But running disk-cache-media.html alone (or before disk-cache-disable) fails. To make it work alone, I have to call `testRunner.SetCacheModel(0); testRunner.SetCacheModel(1);` before `runTests()` in the media tests. Calling it once with `1` is not enough.
Diego Pino
Comment 2 2020-03-23 10:16:50 PDT
The test disk-cache-disable.html leaves the cache as DocumentBrowser. ``` debug("Default (cache enabled)"); runTests(tests, function () { debug("Disabling cache"); testRunner.setCacheModel(0); // DocumentViewer runTests(tests, function () { debug("Re-enabling cache"); testRunner.setCacheModel(1); // DocumentBrowser runTests(tests); }); }); ``` If I run the test http/tests/cache/disk-cache/redirect-chain-limits.html alone, it fails. However, if I run disk-cache-disable.html before this test, it passes. If I edit http/tests/cache/disk-cache/redirect-chain-limits.html, and do the same steps as disk-cache-disable.html before running the test, the test passes too: ``` testRunner.setCacheModel(0); testRunner.setCacheModel(1); testRedirectChain(1, () => { testRedirectChain(5, () => { testRedirectChain(6, () => { testRedirectChain(20, () => { testRedirectChain(40, () => { finishJSTest(); }); }); }); }); }); ``` If I simply do `testRunner.setCacheModel(1)` before running the test, it doesn't work. It seems is necessary to do `testRunner.setCacheModel(0)`. It seems that whether disk-cache-disable.html is run before or after the other tests have an effect on the result of the tests. According to Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:webkit_web_context_set_cache_model the default cache model is DocumentBrowser, so the additional steps in `redirect-chain-limits.html` shouldn't be necessary. Regarding the connection with --skip-failing-tests, this flag doesn't run the tests marked as failure in TestExpectations. Since disk-cache-disable.html is in gtk/TestExpectactions, the test is run and the cache is never set to DocumentBrowser, and all the other disk-cache tests fail.
Lauro Moura
Comment 3 2020-03-23 21:23:09 PDT
Created attachment 394351 [details] Retry getting cache capacity with glib
Lauro Moura
Comment 4 2020-03-23 21:26:59 PDT
The cache creation code tries to get the cache capacity from the cache file location. The problem is that glib's FileSystem::getVolumeFreeSpace does not work for non-existing files (as is the case when initializing the NetworkProcess). I've added a glib-specific patch that tries to get the capacity again after opening the cache if the first try resulted in zero-capacity. With it the currently passing disk-cache files work normally without this unset/set cache model (for example, with skip-failing-tests).
Zan Dobersek
Comment 5 2020-03-24 06:57:59 PDT
Comment on attachment 394351 [details] Retry getting cache capacity with glib View in context: https://bugs.webkit.org/attachment.cgi?id=394351&action=review > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:97 > +#if PLATFORM(GTK) || PLATFORM(WPE) > + // GLIB's getVolumeFreeSpace requires the file to exist to work correctly. > + // As this may not be the case when initializing the NetworkProcess, the first call > + // to computeCapacity may fail and we must try again after the file is created. > + if (!capacity) { > + capacity = computeCapacity(networkProcess.cacheModel(), cachePath); > + storage->setCapacity(capacity); > + } > +#endif It would be better to amend FileSystem::getVolumeFreeSpace() to walk up the cache path to find the first existing file, and query for the free-space information through that.
Lauro Moura
Comment 6 2020-03-24 13:50:12 PDT
Created attachment 394406 [details] Updated patch fixing getVolumeFreeSpace instead
Carlos Garcia Campos
Comment 7 2020-03-25 02:23:43 PDT
Comment on attachment 394406 [details] Updated patch fixing getVolumeFreeSpace instead View in context: https://bugs.webkit.org/attachment.cgi?id=394406&action=review > Source/WTF/ChangeLog:10 > + In some scenarios getVolumeFreeSpace can be called with a not yet > + existing path, which would make the file stat query to fail. For > + example, the NetworkProcess initializing a new cache file. I'm not sure about this. FileSystem::getVolumeFreeSpace() is only called by network cache. I think other implementations in WTF also fail if the file doesn't exist, so I think the problem is not the implementation of getVolumeFreeSpace(), but the caller expecting the file exists. I suspect that for some reason the cache dir is created already at the point computeCapacity() si called for other ports.
Carlos Garcia Campos
Comment 8 2020-03-25 02:25:02 PDT
Comment on attachment 394406 [details] Updated patch fixing getVolumeFreeSpace instead View in context: https://bugs.webkit.org/attachment.cgi?id=394406&action=review > Tools/TestWebKitAPI/Tests/WTF/FileSystem.cpp:160 > +TEST_F(FileSystemTest, getVolumeFreeSpace) > +{ > + String home = FileSystem::homeDirectoryPath(); > + String path = FileSystem::pathByAppendingComponent(home, "ThisFileShouldNotExist"); > + uint64_t freeSpace = 0; > + EXPECT_TRUE(FileSystem::getVolumeFreeSpace(path, freeSpace)); > + EXPECT_GT(freeSpace, 0); Build failed in other ports in EWS, it would be interesting to see if this test passes...
Carlos Garcia Campos
Comment 9 2020-03-25 02:31:04 PDT
So, this is what I would try: - Change Storage::open() to receive the CacheModel instead of the capacity. - Move computeCapacity() from NetworkCache.cpp to NetworkCacheStorage.cpp - In Storage::open() call computeCapacity() right before creating the instance, after readOrMakeSalt().
Zan Dobersek
Comment 10 2020-03-26 02:53:44 PDT
(In reply to Carlos Garcia Campos from comment #7) > Comment on attachment 394406 [details] > Updated patch fixing getVolumeFreeSpace instead > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394406&action=review > > > Source/WTF/ChangeLog:10 > > + In some scenarios getVolumeFreeSpace can be called with a not yet > > + existing path, which would make the file stat query to fail. For > > + example, the NetworkProcess initializing a new cache file. > > I'm not sure about this. FileSystem::getVolumeFreeSpace() is only called by > network cache. I think other implementations in WTF also fail if the file > doesn't exist, so I think the problem is not the implementation of > getVolumeFreeSpace(), but the caller expecting the file exists. I think we should then find a place where we guarantee that the directory specified to NetworkCache::Cache::open() exists. After all the directory specified here is only the parent directory to the directory that Storage::open() spawns.
Lauro Moura
Comment 11 2020-03-26 18:46:48 PDT
Created attachment 394691 [details] Fix sign-compare in testcase
Carlos Garcia Campos
Comment 12 2020-03-27 02:42:13 PDT
And the new api test is failing in other ports as expected, which confirms this is not he way to go. Let's keep the expected behavior of getVolumeFreeSpace() and fix the caller to ensure the directory exists.
Carlos Garcia Campos
Comment 13 2020-04-08 02:46:00 PDT
Created attachment 395783 [details] Patch
Carlos Garcia Campos
Comment 14 2020-04-08 03:29:57 PDT
Committed r259712: <https://trac.webkit.org/changeset/259712>
Radar WebKit Bug Importer
Comment 15 2020-04-08 03:30:17 PDT
<rdar://problem/61444602>
Note You need to log in before you can comment on or make changes to this bug.