Bug 209338 – Ensure base cache path exists before calculating disk cache capacity

• Component: WebKit2
• Status: RESOLVED FIXED

[Lauro Moura]

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.

[Lauro Moura]

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]

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]

Created attachment 394351 [details]
Retry getting cache capacity with glib

[Lauro Moura]

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 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]

Created attachment 394406 [details]
Updated patch fixing getVolumeFreeSpace instead

[Carlos Garcia Campos]

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 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]

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]

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

Created attachment 394691 [details]
Fix sign-compare in testcase

[Carlos Garcia Campos]

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]

Created attachment 395783 [details]
Patch

[Carlos Garcia Campos]

Committed r259712: <https://trac.webkit.org/changeset/259712>

[Radar WebKit Bug Importer]

<rdar://problem/61444602>