Bug 209338

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.

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.

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

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

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.

Created attachment 394406 [details]
Updated patch fixing getVolumeFreeSpace instead

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.

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

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

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

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

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.

Created attachment 395783 [details]
Patch

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

<rdar://problem/61444602>