180118 2017-11-28 16:19:18 -0800 NetworkCache::Storage should protect itself when removing operations from its maps 2017-11-29 03:01:31 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 youennf youennf cdumez cgarcia commit-queue ews-watchlist koivisto webkit-bug-importer oldest_to_newest 1375592 0 youennf 2017-11-28 16:19:18 -0800 The operations can contain ref to the Storage object and removing them from the map may destroy the Storage object. 1375593 1 327802 youennf 2017-11-28 16:22:06 -0800 Created attachment 327802 Patch 1375599 2 327802 koivisto 2017-11-28 16:48:14 -0800 Comment on attachment 327802 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327802&action=review Storage::remove(const Key& key) probably needs one too before removeFromPendingWriteOperations() call. > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:672 > + auto protectedThis = makeRef(*this); > + > ASSERT(m_activeReadOperations.contains(&readOperation)); > m_activeReadOperations.remove(&readOperation); Alternatively these could do auto toDelete = m_activeReadOperations.take(&readOperation); > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:925 > RunLoop::main().dispatch([this, &traverseOperation] { > traverseOperation.handler(nullptr, { }); > + > + auto protectedThis = makeRef(*this); > + > m_activeTraverseOperations.remove(&traverseOperation); This is not strictly needed here but I guess it is good documentation. 1375625 3 youennf 2017-11-28 17:45:46 -0800 Thanks for the review! (In reply to Antti Koivisto from comment #2) > Comment on attachment 327802 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327802&action=review > > Storage::remove(const Key& key) probably needs one too before > removeFromPendingWriteOperations() call. Will add one. > > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:672 > > + auto protectedThis = makeRef(*this); > > + > > ASSERT(m_activeReadOperations.contains(&readOperation)); > > m_activeReadOperations.remove(&readOperation); > > Alternatively these could do > > auto toDelete = m_activeReadOperations.take(&readOperation); Would be nice but we would need to add UNUSED_PARAM. protectedThis is also widely used and somehow clearer about the meaning. 1375630 4 327810 youennf 2017-11-28 17:50:21 -0800 Created attachment 327810 Patch for landing 1375639 5 commit-queue 2017-11-28 18:23:11 -0800 The commit-queue encountered the following flaky tests while processing attachment 327810: svg/animations/animateTransform-pattern-transform.html bug 180121 (authors: ap@webkit.org, arv@chromium.org, krit@webkit.org, mark.lam@apple.com, and zimmermann@kde.org) The commit-queue is continuing to process your patch. 1375640 6 327810 commit-queue 2017-11-28 18:23:41 -0800 Comment on attachment 327810 Patch for landing Clearing flags on attachment: 327810 Committed r225253: <https://trac.webkit.org/changeset/225253> 1375641 7 commit-queue 2017-11-28 18:23:43 -0800 All reviewed patches have been landed. Closing bug. 1375643 8 webkit-bug-importer 2017-11-28 18:24:22 -0800 <rdar://problem/35738687> 1375768 9 koivisto 2017-11-29 03:01:31 -0800 > > auto toDelete = m_activeReadOperations.take(&readOperation); > > Would be nice but we would need to add UNUSED_PARAM. > protectedThis is also widely used and somehow clearer about the meaning. No, it doesn't need UNUSED_PARAM. The compiler is smart enough to know about the implicit destructor call. protectedThis is fine too though. 327802 2017-11-28 16:22:06 -0800 2017-11-28 17:50:20 -0800 Patch bug-180118-20171128162205.patch text/plain 2349 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjI1MTk5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGYxY2FmMzYzM2RkZGZkZmVm NmU0YmE0Mjg1NDA0YzU0MzAwY2IzMGEuLjFhMmMyMzEwMzcyZDc4ZGJiZDk5YzJlZDQ3MDVjZjBj MjE3YWI4MGYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTctMTEtMjggIFlvdWVubiBG YWJsZXQgIDx5b3Vlbm5AYXBwbGUuY29tPgorCisgICAgICAgIE5ldHdvcmtDYWNoZTo6U3RvcmFn ZSBzaG91bGQgcHJvdGVjdCBpdHNlbGYgd2hlbiByZW1vdmluZyBvcGVyYXRpb25zIGZyb20gaXRz IG1hcHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4 MDExOAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRo ZSBvcGVyYXRpb25zIGNhbiBjb250YWluIHJlZiB0byB0aGUgU3RvcmFnZSBvYmplY3QgYW5kIHJl bW92aW5nIHRoZW0gZnJvbSB0aGUgbWFwIG1heSBkZXN0cm95IHRoZSBTdG9yYWdlIG9iamVjdAor CisgICAgICAgICogTmV0d29ya1Byb2Nlc3MvY2FjaGUvTmV0d29ya0NhY2hlU3RvcmFnZS5jcHA6 CisgICAgICAgIChXZWJLaXQ6Ok5ldHdvcmtDYWNoZTo6U3RvcmFnZTo6ZmluaXNoUmVhZE9wZXJh dGlvbik6CisgICAgICAgIChXZWJLaXQ6Ok5ldHdvcmtDYWNoZTo6U3RvcmFnZTo6ZmluaXNoV3Jp dGVPcGVyYXRpb24pOgorICAgICAgICAoV2ViS2l0OjpOZXR3b3JrQ2FjaGU6OlN0b3JhZ2U6OnRy YXZlcnNlKToKKwogMjAxNy0xMS0yNyAgWW91ZW5uIEZhYmxldCAgPHlvdWVubkBhcHBsZS5jb20+ CiAKICAgICAgICAgUmVnaXN0ZXIgRG9jdW1lbnRzIGFzIFNlcnZpY2VXb3JrZXIgY2xpZW50cyB0 byB0aGUgU3RvcmFnZVByb2Nlc3MKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1By b2Nlc3MvY2FjaGUvTmV0d29ya0NhY2hlU3RvcmFnZS5jcHAgYi9Tb3VyY2UvV2ViS2l0L05ldHdv cmtQcm9jZXNzL2NhY2hlL05ldHdvcmtDYWNoZVN0b3JhZ2UuY3BwCmluZGV4IDAzZTMwOTI5MDcx MmRmNDQ3NGRkYTdmMTZiYjRjZmIyNmJlNjNlZWQuLmMyMWJkYTRlNTFlZTllYWUxZDgzYWJjNjFi YjQxMzFiY2ZjMDA1ZTQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3Mv Y2FjaGUvTmV0d29ya0NhY2hlU3RvcmFnZS5jcHAKKysrIGIvU291cmNlL1dlYktpdC9OZXR3b3Jr UHJvY2Vzcy9jYWNoZS9OZXR3b3JrQ2FjaGVTdG9yYWdlLmNwcApAQCAtNjY2LDYgKzY2Niw4IEBA IHZvaWQgU3RvcmFnZTo6ZmluaXNoUmVhZE9wZXJhdGlvbihSZWFkT3BlcmF0aW9uJiByZWFkT3Bl cmF0aW9uKQogICAgICAgICBlbHNlIGlmICghcmVhZE9wZXJhdGlvbi5pc0NhbmNlbGVkKQogICAg ICAgICAgICAgcmVtb3ZlKHJlYWRPcGVyYXRpb24ua2V5KTsKIAorICAgICAgICBhdXRvIHByb3Rl Y3RlZFRoaXMgPSBtYWtlUmVmKCp0aGlzKTsKKwogICAgICAgICBBU1NFUlQobV9hY3RpdmVSZWFk T3BlcmF0aW9ucy5jb250YWlucygmcmVhZE9wZXJhdGlvbikpOwogICAgICAgICBtX2FjdGl2ZVJl YWRPcGVyYXRpb25zLnJlbW92ZSgmcmVhZE9wZXJhdGlvbik7CiAKQEAgLTc5NCw2ICs3OTYsOCBA QCB2b2lkIFN0b3JhZ2U6OmZpbmlzaFdyaXRlT3BlcmF0aW9uKFdyaXRlT3BlcmF0aW9uJiB3cml0 ZU9wZXJhdGlvbikKICAgICBpZiAoLS13cml0ZU9wZXJhdGlvbi5hY3RpdmVDb3VudCkKICAgICAg ICAgcmV0dXJuOwogCisgICAgYXV0byBwcm90ZWN0ZWRUaGlzID0gbWFrZVJlZigqdGhpcyk7CisK ICAgICBtX2FjdGl2ZVdyaXRlT3BlcmF0aW9ucy5yZW1vdmUoJndyaXRlT3BlcmF0aW9uKTsKICAg ICBkaXNwYXRjaFBlbmRpbmdXcml0ZU9wZXJhdGlvbnMoKTsKIApAQCAtOTE1LDYgKzkxOSw5IEBA IHZvaWQgU3RvcmFnZTo6dHJhdmVyc2UoY29uc3QgU3RyaW5nJiB0eXBlLCBUcmF2ZXJzZUZsYWdz IGZsYWdzLCBUcmF2ZXJzZUhhbmRsZXImCiAgICAgICAgIH0KICAgICAgICAgUnVuTG9vcDo6bWFp bigpLmRpc3BhdGNoKFt0aGlzLCAmdHJhdmVyc2VPcGVyYXRpb25dIHsKICAgICAgICAgICAgIHRy YXZlcnNlT3BlcmF0aW9uLmhhbmRsZXIobnVsbHB0ciwgeyB9KTsKKworICAgICAgICAgICAgYXV0 byBwcm90ZWN0ZWRUaGlzID0gbWFrZVJlZigqdGhpcyk7CisKICAgICAgICAgICAgIG1fYWN0aXZl VHJhdmVyc2VPcGVyYXRpb25zLnJlbW92ZSgmdHJhdmVyc2VPcGVyYXRpb24pOwogICAgICAgICB9 KTsKICAgICB9KTsK 327810 2017-11-28 17:50:21 -0800 2017-11-28 18:23:41 -0800 Patch for landing bug-180118-20171128175020.patch text/plain 3207 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjI1MjQ4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGZmNzc4MTlkNjkwZWRhMTIz NmFmZGQ5MmU2MGY5YWY0NzdmNzZiNjguLmRiMGE4NTA4YjE2ZmY3NmRhNmVhNWQ2YTY4MjY4NDQ2 MTk5MmQwMGUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjAgQEAKIDIwMTctMTEtMjggIFlvdWVubiBG YWJsZXQgIDx5b3Vlbm5AYXBwbGUuY29tPgogCisgICAgICAgIE5ldHdvcmtDYWNoZTo6U3RvcmFn ZSBzaG91bGQgcHJvdGVjdCBpdHNlbGYgd2hlbiByZW1vdmluZyBvcGVyYXRpb25zIGZyb20gaXRz IG1hcHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4 MDExOAorCisgICAgICAgIFJldmlld2VkIGJ5IEFudHRpIEtvaXZpc3RvLgorCisgICAgICAgIFRo ZSBvcGVyYXRpb25zIGNhbiBjb250YWluIHJlZiB0byB0aGUgU3RvcmFnZSBvYmplY3QgYW5kIHJl bW92aW5nIHRoZW0gZnJvbSB0aGUgbWFwIG1heSBkZXN0cm95IHRoZSBTdG9yYWdlIG9iamVjdAor CisgICAgICAgICogTmV0d29ya1Byb2Nlc3MvY2FjaGUvTmV0d29ya0NhY2hlU3RvcmFnZS5jcHA6 CisgICAgICAgIChXZWJLaXQ6Ok5ldHdvcmtDYWNoZTo6U3RvcmFnZTo6cmVtb3ZlKToKKyAgICAg ICAgKFdlYktpdDo6TmV0d29ya0NhY2hlOjpTdG9yYWdlOjpmaW5pc2hSZWFkT3BlcmF0aW9uKToK KyAgICAgICAgKFdlYktpdDo6TmV0d29ya0NhY2hlOjpTdG9yYWdlOjpmaW5pc2hXcml0ZU9wZXJh dGlvbik6CisgICAgICAgIChXZWJLaXQ6Ok5ldHdvcmtDYWNoZTo6U3RvcmFnZTo6dHJhdmVyc2Up OgorCisyMDE3LTExLTI4ICBZb3Vlbm4gRmFibGV0ICA8eW91ZW5uQGFwcGxlLmNvbT4KKwogICAg ICAgICBSZWdpc3RlciBEb2N1bWVudHMgYXMgU2VydmljZVdvcmtlciBjbGllbnRzIHRvIHRoZSBT dG9yYWdlUHJvY2VzcwogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTgwMDQ3CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3Mv Y2FjaGUvTmV0d29ya0NhY2hlU3RvcmFnZS5jcHAgYi9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9j ZXNzL2NhY2hlL05ldHdvcmtDYWNoZVN0b3JhZ2UuY3BwCmluZGV4IDAzZTMwOTI5MDcxMmRmNDQ3 NGRkYTdmMTZiYjRjZmIyNmJlNjNlZWQuLjg3OWYxZGNjZjM2MzAwMTA5OTk1NjM3MzQxOTczNWY1 MDc3Y2I5NjkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2FjaGUv TmV0d29ya0NhY2hlU3RvcmFnZS5jcHAKKysrIGIvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vz cy9jYWNoZS9OZXR3b3JrQ2FjaGVTdG9yYWdlLmNwcApAQCAtNTYxLDEzICs1NjEsMTUgQEAgdm9p ZCBTdG9yYWdlOjpyZW1vdmUoY29uc3QgS2V5JiBrZXkpCiAgICAgaWYgKCFtYXlDb250YWluKGtl eSkpCiAgICAgICAgIHJldHVybjsKIAorICAgIGF1dG8gcHJvdGVjdGVkVGhpcyA9IG1ha2VSZWYo KnRoaXMpOworCiAgICAgLy8gV2UgY2FuJ3QgcmVtb3ZlIHRoZSBrZXkgZnJvbSB0aGUgQmxvb20g ZmlsdGVyIChidXQgc29tZSBmYWxzZSBwb3NpdGl2ZXMgYXJlIGV4cGVjdGVkIGFueXdheSkuCiAg ICAgLy8gRm9yIHNpbXBsaWNpdHkgd2UgYWxzbyBkb24ndCByZWR1Y2UgbV9hcHByb3hpbWF0ZVNp emUgb24gcmVtb3ZhbHMuCiAgICAgLy8gVGhlIG5leHQgc3luY2hyb25pemF0aW9uIHdpbGwgdXBk YXRlIGV2ZXJ5dGhpbmcuCiAKICAgICByZW1vdmVGcm9tUGVuZGluZ1dyaXRlT3BlcmF0aW9ucyhr ZXkpOwogCi0gICAgc2VyaWFsQmFja2dyb3VuZElPUXVldWUoKS5kaXNwYXRjaChbdGhpcywgcHJv dGVjdGVkVGhpcyA9IG1ha2VSZWYoKnRoaXMpLCBrZXldICgpIG11dGFibGUgeworICAgIHNlcmlh bEJhY2tncm91bmRJT1F1ZXVlKCkuZGlzcGF0Y2goW3RoaXMsIHByb3RlY3RlZFRoaXMgPSBXVEZN b3ZlKHByb3RlY3RlZFRoaXMpLCBrZXldICgpIG11dGFibGUgewogICAgICAgICBkZWxldGVGaWxl cyhrZXkpOwogICAgICAgICBSdW5Mb29wOjptYWluKCkuZGlzcGF0Y2goW3Byb3RlY3RlZFRoaXMg PSBXVEZNb3ZlKHByb3RlY3RlZFRoaXMpXSB7IH0pOwogICAgIH0pOwpAQCAtNjY2LDYgKzY2OCw4 IEBAIHZvaWQgU3RvcmFnZTo6ZmluaXNoUmVhZE9wZXJhdGlvbihSZWFkT3BlcmF0aW9uJiByZWFk T3BlcmF0aW9uKQogICAgICAgICBlbHNlIGlmICghcmVhZE9wZXJhdGlvbi5pc0NhbmNlbGVkKQog ICAgICAgICAgICAgcmVtb3ZlKHJlYWRPcGVyYXRpb24ua2V5KTsKIAorICAgICAgICBhdXRvIHBy b3RlY3RlZFRoaXMgPSBtYWtlUmVmKCp0aGlzKTsKKwogICAgICAgICBBU1NFUlQobV9hY3RpdmVS ZWFkT3BlcmF0aW9ucy5jb250YWlucygmcmVhZE9wZXJhdGlvbikpOwogICAgICAgICBtX2FjdGl2 ZVJlYWRPcGVyYXRpb25zLnJlbW92ZSgmcmVhZE9wZXJhdGlvbik7CiAKQEAgLTc5NCw2ICs3OTgs OCBAQCB2b2lkIFN0b3JhZ2U6OmZpbmlzaFdyaXRlT3BlcmF0aW9uKFdyaXRlT3BlcmF0aW9uJiB3 cml0ZU9wZXJhdGlvbikKICAgICBpZiAoLS13cml0ZU9wZXJhdGlvbi5hY3RpdmVDb3VudCkKICAg ICAgICAgcmV0dXJuOwogCisgICAgYXV0byBwcm90ZWN0ZWRUaGlzID0gbWFrZVJlZigqdGhpcyk7 CisKICAgICBtX2FjdGl2ZVdyaXRlT3BlcmF0aW9ucy5yZW1vdmUoJndyaXRlT3BlcmF0aW9uKTsK ICAgICBkaXNwYXRjaFBlbmRpbmdXcml0ZU9wZXJhdGlvbnMoKTsKIApAQCAtOTE1LDYgKzkyMSw5 IEBAIHZvaWQgU3RvcmFnZTo6dHJhdmVyc2UoY29uc3QgU3RyaW5nJiB0eXBlLCBUcmF2ZXJzZUZs YWdzIGZsYWdzLCBUcmF2ZXJzZUhhbmRsZXImCiAgICAgICAgIH0KICAgICAgICAgUnVuTG9vcDo6 bWFpbigpLmRpc3BhdGNoKFt0aGlzLCAmdHJhdmVyc2VPcGVyYXRpb25dIHsKICAgICAgICAgICAg IHRyYXZlcnNlT3BlcmF0aW9uLmhhbmRsZXIobnVsbHB0ciwgeyB9KTsKKworICAgICAgICAgICAg YXV0byBwcm90ZWN0ZWRUaGlzID0gbWFrZVJlZigqdGhpcyk7CisKICAgICAgICAgICAgIG1fYWN0 aXZlVHJhdmVyc2VPcGVyYXRpb25zLnJlbW92ZSgmdHJhdmVyc2VPcGVyYXRpb24pOwogICAgICAg ICB9KTsKICAgICB9KTsK