Bug 206057 - CacheStorage::Engine::clearCachesForOriginFromDisk ASSERT is buggy
Summary: CacheStorage::Engine::clearCachesForOriginFromDisk ASSERT is buggy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-10 03:23 PST by youenn fablet
Modified: 2020-01-14 01:17 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.91 KB, patch)
2020-01-10 03:30 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (1.98 KB, patch)
2020-01-13 23:07 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-01-10 03:23:48 PST
CacheStorage::Engine::clearCachesForOriginFromDisk ASSERT is buggy
Comment 1 youenn fablet 2020-01-10 03:24:05 PST
<rdar://problem/57762994>
Comment 2 youenn fablet 2020-01-10 03:30:29 PST
Created attachment 387324 [details]
Patch
Comment 3 John Wilander 2020-01-10 08:52:38 PST
Since I was able to reproduce this issue, could a test be created?
Comment 4 youenn fablet 2020-01-10 08:57:13 PST
(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.
Comment 5 youenn fablet 2020-01-13 11:14:03 PST
Ping review.
Comment 6 John Wilander 2020-01-13 11:23:49 PST
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.
Comment 7 youenn fablet 2020-01-13 11:58:16 PST
(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.
Comment 8 John Wilander 2020-01-13 12:12:57 PST
(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" :)
Comment 9 youenn fablet 2020-01-13 23:07:13 PST
Created attachment 387622 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2020-01-14 01:17:50 PST
Comment on attachment 387622 [details]
Patch for landing

Clearing flags on attachment: 387622

Committed r254501: <https://trac.webkit.org/changeset/254501>
Comment 11 WebKit Commit Bot 2020-01-14 01:17:52 PST
All reviewed patches have been landed.  Closing bug.