CacheStorage::Engine::clearCachesForOriginFromDisk ASSERT is buggy

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

CacheStorage::Engine::clearCachesForOriginFromDisk ASSERT is buggy

[youenn fablet]

<rdar://problem/57762994>

[youenn fablet]

Created attachment 387324 [details]
Patch

[John Wilander]

Since I was able to reproduce this issue, could a test be created?

[youenn fablet]

(In reply to John Wilander from comment #3)
> Since I was able to reproduce this issue, could a test be created?

I guess we could write an API test that would:
- Trigger some persistent cache storage.
- Tear down the network process
- Create a new network process and clear the cache storage.

I am not sure this is worth it though.

[youenn fablet]

Ping review.

[John Wilander]

Comment on attachment 387324 [details]
Patch

cachesRootPath() returns the empty string if the salt is not set, like so:

String Engine::cachesRootPath(const WebCore::ClientOrigin& origin)
{
    if (!shouldPersist() || !m_salt)
        return { };
  …
}

With your change, we're saying folderPath does not have to be equal to the return value of cachesRootPath() if we don't have a salt. Then we go ahead and call deleteDirectoryRecursivelyOnBackgroundThread() with the folderPath that does not match the return value of cachesRootPath(). Is that correct?

My assumption would be to return early if the path is empty.

We need a comment to explain this logic in my opinion.

[youenn fablet]

(In reply to John Wilander from comment #6)
> Comment on attachment 387324 [details]
> Patch
> 
> cachesRootPath() returns the empty string if the salt is not set, like so:
> 
> String Engine::cachesRootPath(const WebCore::ClientOrigin& origin)
> {
>     if (!shouldPersist() || !m_salt)
>         return { };
>   …
> }
> 
> With your change, we're saying folderPath does not have to be equal to the
> return value of cachesRootPath() if we don't have a salt. Then we go ahead
> and call deleteDirectoryRecursivelyOnBackgroundThread() with the folderPath
> that does not match the return value of cachesRootPath(). Is that correct?
> 
> My assumption would be to return early if the path is empty.

If the path is empty, cache salt has not be read/was not readable.

We are always deleting data from the origin given to the method.
The reason is that we read the folder origin directly from the "origin" file inside that folder.
I guess we could add a comment for that.

> We need a comment to explain this logic in my opinion.

It seems the issue you have is with the purpose of the ASSERT, right?
I can add something like: // If cache salt is initialised and the paths do not match, some cache files are probably partially corrupted.

[John Wilander]

(In reply to youenn fablet from comment #7)
> (In reply to John Wilander from comment #6)
> > Comment on attachment 387324 [details]
> > Patch
> > 
> > cachesRootPath() returns the empty string if the salt is not set, like so:
> > 
> > String Engine::cachesRootPath(const WebCore::ClientOrigin& origin)
> > {
> >     if (!shouldPersist() || !m_salt)
> >         return { };
> >   …
> > }
> > 
> > With your change, we're saying folderPath does not have to be equal to the
> > return value of cachesRootPath() if we don't have a salt. Then we go ahead
> > and call deleteDirectoryRecursivelyOnBackgroundThread() with the folderPath
> > that does not match the return value of cachesRootPath(). Is that correct?
> > 
> > My assumption would be to return early if the path is empty.
> 
> If the path is empty, cache salt has not be read/was not readable.
> 
> We are always deleting data from the origin given to the method.
> The reason is that we read the folder origin directly from the "origin" file
> inside that folder.
> I guess we could add a comment for that.
> 
> > We need a comment to explain this logic in my opinion.
> 
> It seems the issue you have is with the purpose of the ASSERT, right?
> I can add something like: // If cache salt is initialised and the paths do
> not match, some cache files are probably partially corrupted.

r+ with the comment and US English spelling "initialised" –> "initialized" :)

[youenn fablet]

Created attachment 387622 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 387622 [details]
Patch for landing

Clearing flags on attachment: 387622

Committed r254501: <https://trac.webkit.org/changeset/254501>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.