Bug 206057

<rdar://problem/57762994>

Created attachment 387324 [details] Patch

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

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

Ping review.

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.

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

(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" :)

Created attachment 387622 [details] Patch for landing

Comment on attachment 387622 [details] Patch for landing Clearing flags on attachment: 387622 Committed r254501: <https://trac.webkit.org/changeset/254501>

All reviewed patches have been landed. Closing bug.