206057 2020-01-10 03:23:48 -0800 CacheStorage::Engine::clearCachesForOriginFromDisk ASSERT is buggy 2020-01-14 01:17:52 -0800 1 1 1 Unclassified WebKit Service Workers WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 youennf youennf cdumez cgarcia commit-queue ews-watchlist webkit-bug-importer wilander oldest_to_newest 1605049 0 youennf 2020-01-10 03:23:48 -0800 CacheStorage::Engine::clearCachesForOriginFromDisk ASSERT is buggy 1605050 1 youennf 2020-01-10 03:24:05 -0800 <rdar://problem/57762994> 1605051 2 387324 youennf 2020-01-10 03:30:29 -0800 Created attachment 387324 Patch 1605123 3 wilander 2020-01-10 08:52:38 -0800 Since I was able to reproduce this issue, could a test be created? 1605127 4 youennf 2020-01-10 08:57:13 -0800 (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. 1605959 5 youennf 2020-01-13 11:14:03 -0800 Ping review. 1605963 6 387324 wilander 2020-01-13 11:23:49 -0800 Comment on attachment 387324 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. 1605975 7 youennf 2020-01-13 11:58:16 -0800 (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. 1605981 8 wilander 2020-01-13 12:12:57 -0800 (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" :) 1606264 9 387622 youennf 2020-01-13 23:07:13 -0800 Created attachment 387622 Patch for landing 1606301 10 387622 commit-queue 2020-01-14 01:17:50 -0800 Comment on attachment 387622 Patch for landing Clearing flags on attachment: 387622 Committed r254501: <https://trac.webkit.org/changeset/254501> 1606302 11 commit-queue 2020-01-14 01:17:52 -0800 All reviewed patches have been landed. Closing bug. 387324 2020-01-10 03:30:29 -0800 2020-01-13 23:07:09 -0800 Patch bug-206057-20200110123027.patch text/plain 1957 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjU0MzMwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDk5OWNlMjk4YWFlNDhlZTUw ZDE0ZTA3YjMxN2VmY2Y1M2QxN2YzNDUuLjkxNGVkNDVkOTg5ZmM0MzE0ZWFjYjRhYTZkYjFiNjUy ZTkyYTJjODggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMjAtMDEtMTAgIHlvdWVubiBm YWJsZXQgIDx5b3Vlbm5AYXBwbGUuY29tPgorCisgICAgICAgIENhY2hlU3RvcmFnZTo6RW5naW5l OjpjbGVhckNhY2hlc0Zvck9yaWdpbkZyb21EaXNrIEFTU0VSVCBpcyBidWdneQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjA2MDU3CisgICAgICAgIDxy ZGFyOi8vcHJvYmxlbS81Nzc2Mjk5ND4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBUaGUgQVNTRVJUIGlzIG9ubHkgdmFsaWQgaWYgdGhlIGVuZ2luZSBp cyBpbml0aWFsaXplZC4KKyAgICAgICAgSXQgaXMgbm90IG5lZWRlZCB0byBpbml0aWFsaXplIGl0 IGlmIHRoZSBwbGFuIGlzIHRvIHJlbW92ZSBhbGwgZGlzayBmaWxlcy4KKyAgICAgICAgSW5zdGVh ZCwgdXBkYXRlIEFTU0VSVCB0byBjaGVjayB0aGF0IGVpdGhlciBtX3NhbHQgaXMgbm90IHRoZXJl IG9yIHRoZSBzYWx0IGlzIHRoZXJlIGFuZCB0aGUgcGF0aCBpcyBhcyBleHBlY3RlZC4KKworICAg ICAgICAqIE5ldHdvcmtQcm9jZXNzL2NhY2hlL0NhY2hlU3RvcmFnZUVuZ2luZS5jcHA6CisgICAg ICAgIChXZWJLaXQ6OkNhY2hlU3RvcmFnZTo6RW5naW5lOjpjbGVhckNhY2hlc0Zvck9yaWdpbkZy b21EaXJlY3Rvcmllcyk6CisKIDIwMjAtMDEtMTAgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dh cmNpYUBpZ2FsaWEuY29tPgogCiAgICAgICAgIEF1dG9tYXRpb246IHJlc29sdmVDaGlsZEZyYW1l V2l0aE5vZGVIYW5kbGUgc2hvdWxkIHJldHVybiBOb2RlTm90Rm91bmQgd2hlbiBub2RlIGRvZXNu J3QgZXhpc3QKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2FjaGUv Q2FjaGVTdG9yYWdlRW5naW5lLmNwcCBiL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2Fj aGUvQ2FjaGVTdG9yYWdlRW5naW5lLmNwcAppbmRleCA2ZGFiYzU0MWVhNWZlZjQ5MjNhZWM4MDY5 MWY4MDQ0NmFlMTE4NTZkLi40NzAyMWQ3ODA0ZjgxMTNmMGI5YzM0OTQ1ZGYxNGQ0NzQ1Mzc2Yjk5 IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL2NhY2hlL0NhY2hlU3Rv cmFnZUVuZ2luZS5jcHAKKysrIGIvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9jYWNoZS9D YWNoZVN0b3JhZ2VFbmdpbmUuY3BwCkBAIC03NDQsNyArNzQ0LDcgQEAgdm9pZCBFbmdpbmU6OmNs ZWFyQ2FjaGVzRm9yT3JpZ2luRnJvbURpcmVjdG9yaWVzKGNvbnN0IFZlY3RvcjxTdHJpbmc+JiBm b2xkZXJQYXQKICAgICAgICAgICAgIGlmIChmb2xkZXJPcmlnaW4tPnRvcE9yaWdpbiAhPSBvcmln aW4gJiYgZm9sZGVyT3JpZ2luLT5jbGllbnRPcmlnaW4gIT0gb3JpZ2luKQogICAgICAgICAgICAg ICAgIHJldHVybjsKIAotICAgICAgICAgICAgQVNTRVJUKGZvbGRlclBhdGggPT0gY2FjaGVzUm9v dFBhdGgoKmZvbGRlck9yaWdpbikpOworICAgICAgICAgICAgQVNTRVJUKCFtX3NhbHQgfHwgZm9s ZGVyUGF0aCA9PSBjYWNoZXNSb290UGF0aCgqZm9sZGVyT3JpZ2luKSk7CiAgICAgICAgICAgICBk ZWxldGVEaXJlY3RvcnlSZWN1cnNpdmVseU9uQmFja2dyb3VuZFRocmVhZChmb2xkZXJQYXRoLCBb Y2FsbGJhY2tBZ2dyZWdhdG9yID0gV1RGTW92ZShjYWxsYmFja0FnZ3JlZ2F0b3IpXSB7IH0pOwog ICAgICAgICB9KTsKICAgICB9Cg== 387622 2020-01-13 23:07:13 -0800 2020-01-14 01:17:50 -0800 Patch for landing bug-206057-20200114080712.patch text/plain 2030 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjU0NDk2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDJjZGRhZDg1NTliMDUxOGYx NmRhM2JiMWZlNGY5ZjhiNDQ4NGU4MTMuLjY5NzRhY2ZjZGUyNTA4YTc3NmM3MzhiZDZlYjE4MzRi OTM4MzU2YjEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMjAtMDEtMTMgIHlvdWVubiBm YWJsZXQgIDx5b3Vlbm5AYXBwbGUuY29tPgorCisgICAgICAgIENhY2hlU3RvcmFnZTo6RW5naW5l OjpjbGVhckNhY2hlc0Zvck9yaWdpbkZyb21EaXNrIEFTU0VSVCBpcyBidWdneQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjA2MDU3CisgICAgICAgIDxy ZGFyOi8vcHJvYmxlbS81Nzc2Mjk5ND4KKworICAgICAgICBSZXZpZXdlZCBieSBKb2huIFdpbGFu ZGVyLgorCisgICAgICAgIFRoZSBBU1NFUlQgaXMgb25seSB2YWxpZCBpZiB0aGUgZW5naW5lIGlz IGluaXRpYWxpemVkLgorICAgICAgICBJdCBpcyBub3QgbmVlZGVkIHRvIGluaXRpYWxpemUgaXQg aWYgdGhlIHBsYW4gaXMgdG8gcmVtb3ZlIGFsbCBkaXNrIGZpbGVzLgorICAgICAgICBJbnN0ZWFk LCB1cGRhdGUgQVNTRVJUIHRvIGNoZWNrIHRoYXQgZWl0aGVyIG1fc2FsdCBpcyBub3QgdGhlcmUg b3IgdGhlIHNhbHQgaXMgdGhlcmUgYW5kIHRoZSBwYXRoIGlzIGFzIGV4cGVjdGVkLgorCisgICAg ICAgICogTmV0d29ya1Byb2Nlc3MvY2FjaGUvQ2FjaGVTdG9yYWdlRW5naW5lLmNwcDoKKyAgICAg ICAgKFdlYktpdDo6Q2FjaGVTdG9yYWdlOjpFbmdpbmU6OmNsZWFyQ2FjaGVzRm9yT3JpZ2luRnJv bURpcmVjdG9yaWVzKToKKwogMjAyMC0wMS0xMyAgRG9uIE9sbXN0ZWFkICA8ZG9uLm9sbXN0ZWFk QHNvbnkuY29tPgogCiAgICAgICAgIFtXZWJBdXRobl0gU3VwcG9ydCBDVEFQIENsaWVudCBQaW4K ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2FjaGUvQ2FjaGVTdG9y YWdlRW5naW5lLmNwcCBiL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2FjaGUvQ2FjaGVT dG9yYWdlRW5naW5lLmNwcAppbmRleCA2ZGFiYzU0MWVhNWZlZjQ5MjNhZWM4MDY5MWY4MDQ0NmFl MTE4NTZkLi5kMTkwZmNmZDA3YmVhMDFkNDIwMGQ4ODBjZTg3ZTk2OTMzOTJmODk0IDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL2NhY2hlL0NhY2hlU3RvcmFnZUVuZ2lu ZS5jcHAKKysrIGIvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9jYWNoZS9DYWNoZVN0b3Jh Z2VFbmdpbmUuY3BwCkBAIC03NDQsNyArNzQ0LDggQEAgdm9pZCBFbmdpbmU6OmNsZWFyQ2FjaGVz Rm9yT3JpZ2luRnJvbURpcmVjdG9yaWVzKGNvbnN0IFZlY3RvcjxTdHJpbmc+JiBmb2xkZXJQYXQK ICAgICAgICAgICAgIGlmIChmb2xkZXJPcmlnaW4tPnRvcE9yaWdpbiAhPSBvcmlnaW4gJiYgZm9s ZGVyT3JpZ2luLT5jbGllbnRPcmlnaW4gIT0gb3JpZ2luKQogICAgICAgICAgICAgICAgIHJldHVy bjsKIAotICAgICAgICAgICAgQVNTRVJUKGZvbGRlclBhdGggPT0gY2FjaGVzUm9vdFBhdGgoKmZv bGRlck9yaWdpbikpOworICAgICAgICAgICAgLy8gSWYgY2FjaGUgc2FsdCBpcyBpbml0aWFsaXpl ZCBhbmQgdGhlIHBhdGhzIGRvIG5vdCBtYXRjaCwgc29tZSBjYWNoZSBmaWxlcyBoYXZlIHByb2Jh Ymx5IGJlIHJlbW92ZWQgb3IgcGFydGlhbGx5IGNvcnJ1cHRlZC4KKyAgICAgICAgICAgIEFTU0VS VCghbV9zYWx0IHx8IGZvbGRlclBhdGggPT0gY2FjaGVzUm9vdFBhdGgoKmZvbGRlck9yaWdpbikp OwogICAgICAgICAgICAgZGVsZXRlRGlyZWN0b3J5UmVjdXJzaXZlbHlPbkJhY2tncm91bmRUaHJl YWQoZm9sZGVyUGF0aCwgW2NhbGxiYWNrQWdncmVnYXRvciA9IFdURk1vdmUoY2FsbGJhY2tBZ2dy ZWdhdG9yKV0geyB9KTsKICAgICAgICAgfSk7CiAgICAgfQo=