205086 2019-12-10 14:49:38 -0800 Worklist::deleteCancelledPlansForVM() should not assume that a cancelled plan is ready for deletion. 2019-12-10 17:45:39 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam ews-watchlist keith_miller msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1597357 0 mark.lam 2019-12-10 14:49:38 -0800 Another thread may still have a reference to the cancelled plan and hasn't released it yet. <rdar://problem/57795002> 1597423 1 385319 mark.lam 2019-12-10 16:57:34 -0800 Created attachment 385319 proposed patch. 1597430 2 385319 saam 2019-12-10 17:02:01 -0800 Comment on attachment 385319 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=385319&action=review > Source/JavaScriptCore/dfg/DFGWorklist.cpp:310 > HashSet<RefPtr<Plan>> removedPlans; this shouldn't be a HashSet, right? > Source/JavaScriptCore/dfg/DFGWorklist.cpp:318 > + removedPlans.add(WTFMove(plan)); isn't this easier to just append only when ref count is 1? like: Plan* plan = m_cancelledPlansPendingDestruction[I].get(); if (plan->refCount() == 1) removedPlans.append(...) and remove the +1 ref in the above code 1597432 3 385319 mark.lam 2019-12-10 17:05:43 -0800 Comment on attachment 385319 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=385319&action=review >> Source/JavaScriptCore/dfg/DFGWorklist.cpp:310 >> HashSet<RefPtr<Plan>> removedPlans; > > this shouldn't be a HashSet, right? This should be a HashSet because (as I explained offline previously for the original patch) there's a chance that the GC thread cancels the plan and appends it to m_cancelledPlansPendingDestruction, and then the compiler thread sees it and appends it to m_cancelledPlansPendingDestruction again before the mutator can iterate m_cancelledPlansPendingDestruction. As a result, the same plan may show up in m_cancelledPlansPendingDestruction more than once, but we only want to add it to removedPlans once. >> Source/JavaScriptCore/dfg/DFGWorklist.cpp:318 >> + removedPlans.add(WTFMove(plan)); > > isn't this easier to just append only when ref count is 1? > > like: > Plan* plan = m_cancelledPlansPendingDestruction[I].get(); > if (plan->refCount() == 1) removedPlans.append(...) > > and remove the +1 ref in the above code Good point. I will change the fix. 1597433 4 385319 mark.lam 2019-12-10 17:07:51 -0800 Comment on attachment 385319 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=385319&action=review >>> Source/JavaScriptCore/dfg/DFGWorklist.cpp:318 >>> + removedPlans.add(WTFMove(plan)); >> >> isn't this easier to just append only when ref count is 1? >> >> like: >> Plan* plan = m_cancelledPlansPendingDestruction[I].get(); >> if (plan->refCount() == 1) removedPlans.append(...) >> >> and remove the +1 ref in the above code > > Good point. I will change the fix. I take this back. Because the plan can appear in m_cancelledPlansPendingDestruction more than once, this scheme won't work. We have to filter it through the HashSet. 1597449 5 385319 saam 2019-12-10 17:34:42 -0800 Comment on attachment 385319 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=385319&action=review > Source/JavaScriptCore/dfg/DFGWorklist.cpp:323 > RELEASE_ASSERT(plan->stage() == Plan::Cancelled); should we make this a debug assert like before? 1597453 6 mark.lam 2019-12-10 17:42:37 -0800 (In reply to Saam Barati from comment #5) > Comment on attachment 385319 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385319&action=review > > > Source/JavaScriptCore/dfg/DFGWorklist.cpp:323 > > RELEASE_ASSERT(plan->stage() == Plan::Cancelled); > > should we make this a debug assert like before? I'll change this to a debug assert. I also added a comment explaining how we can have the same cancelled plan appear more than once in m_cancelledPlansPendingDestruction, and why we need to filter it through the HashSet. The fact that you asked about it shows that we'll probably forget about this detail in the future. Hence, a comment is needed to document it for posterity. 1597456 7 mark.lam 2019-12-10 17:45:39 -0800 Thanks for the review. Landed in r253358: <http://trac.webkit.org/r253358>. 385319 2019-12-10 16:57:34 -0800 2019-12-10 17:34:42 -0800 proposed patch. bug-205086.patch text/plain 4012 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjUzMzU0KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDM2IEBA CisyMDE5LTEyLTEwICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBX b3JrbGlzdDo6ZGVsZXRlQ2FuY2VsbGVkUGxhbnNGb3JWTSgpIHNob3VsZCBub3QgYXNzdW1lIHRo YXQgYSBjYW5jZWxsZWQgcGxhbiBpcyByZWFkeSBmb3IgZGVsZXRpb24uCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDUwODYKKyAgICAgICAgPHJkYXI6 Ly9wcm9ibGVtLzU3Nzk1MDAyPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIENvbnNpZGVyIHRoaXMgcmFjZSBzY2VuYXJpbzoKKyAgICAgICAgMS4gVGhl IERGRyB0aHJlYWQgZmluZHMgYSBwbGFuIGFuZCBzdGFydGVkIGNvbXBpbGluZywgYW5kIGl0J3Mg aG9sZGluZyBhIHJlZiB0bworICAgICAgICAgICB0aGUgcGxhbiB3aGlsZSBpdCdzIGNvbXBpbGlu Zy4KKyAgICAgICAgMi4gVGhlIEdDIHRocmVhZCBkaXNjb3ZlcnMgdGhhdCB3ZSBubyBsb25nZXIg bmVlZCB0aGUgcGxhbiBhbmQgY2FuY2VscyBpdC4KKyAgICAgICAgMy4gQWZ0ZXIgdGhlIHBsYW4g aXMgY2FuY2VsbGVkIGJ1dCB3aGlsZSB0aGUgREZHIHRocmVhZCBpcyBzdGlsbCBjb21waWxpbmcs IHRoZQorICAgICAgICAgICBtdXRhdG9yIHRocmVhZCBjYWxscyBXb3JrbGlzdDo6ZGVsZXRlQ2Fu Y2VsbGVkUGxhbnNGb3JWTSgpLgorCisgICAgICAgICAgIFdvcmtsaXN0OjpkZWxldGVDYW5jZWxs ZWRQbGFuc0ZvclZNKCkgd2FzIGFzc3VtaW5nIHRoYXQgYnkgdGhlIHRpbWUgaXQgaXMKKyAgICAg ICAgICAgY2FsbGVkLCBXb3JrbGlzdDo6bV9jYW5jZWxsZWRQbGFuc1BlbmRpbmdEZXN0cnVjdGlv biB3aWxsIGNvbnRhaW4gdGhlIGxhc3QgcmVmCisgICAgICAgICAgIHRvIHRoZSBjYW5jZWxsZWQg cGxhbi4gIEhvd2V2ZXIsIHRoaXMgaXMgYW4gaW5jb3JyZWN0IGFzc3VtcHRpb24sIGFuZCB0aGUK KyAgICAgICAgICAgYXNzZXJ0aW9uIHRoZXJlIHRoYXQgYXNzZXJ0cyByZWZDb3VudCA9PSAxIHdp bGwgZmFpbC4KKworICAgICAgICBUaGlzIHBhdGNoIGZpeGVzIFdvcmtsaXN0OjpkZWxldGVDYW5j ZWxsZWRQbGFuc0ZvclZNKCkgdG8gYXBwZW5kIHRoZSBjYW5jZWxsZWQKKyAgICAgICAgcGxhbiBi YWNrIGludG8gbV9jYW5jZWxsZWRQbGFuc1BlbmRpbmdEZXN0cnVjdGlvbiBpZiBpdHMgcmVmQ291 bnQgaXMgbm90IDEKKyAgICAgICAgKGltcGx5aW5nIHRoYXQgdGhlIGNvbXBpbGVyIHRocmVhZCBz dGlsbCBoYXMgYSByZWYgdG8gaXQpLCBhbmQgZGVmZXIgZGVsZXRpb24gb2YKKyAgICAgICAgdGhl IHBsYW4gdG8gYSBzdWJzZXF1ZW50IGNhbGwgdG8gZGVsZXRlQ2FuY2VsbGVkUGxhbnNGb3JWTSgp LgorCisgICAgICAgIFRoaXMgcGF0Y2ggYWxzbyBhZGRzIGEgV1RGTW92ZSB0byBXb3JrbGlzdDo6 cmVtb3ZlRGVhZFBsYW5zKCkgd2hlbiB3ZSBhcHBlbmQgdGhlCisgICAgICAgIGNhbmNlbGxlZCBw bGFuIHRvIG1fY2FuY2VsbGVkUGxhbnNQZW5kaW5nRGVzdHJ1Y3Rpb24gdGhlcmUuICBUaGlzIHNh dmVzIHVzIG9uZQorICAgICAgICB1bm5lY2Vzc2FyeSByZWYgYW5kIGRlcmVmIG9mIHRoZSBwbGFu LgorCisgICAgICAgICogZGZnL0RGR1dvcmtsaXN0LmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpX b3JrbGlzdDo6ZGVsZXRlQ2FuY2VsbGVkUGxhbnNGb3JWTSk6CisgICAgICAgIChKU0M6OkRGRzo6 V29ya2xpc3Q6OnJlbW92ZURlYWRQbGFucyk6CisKIDIwMTktMTItMTAgIFNhYW0gQmFyYXRpICA8 c2JhcmF0aUBhcHBsZS5jb20+CiAKICAgICAgICAgbWV0aG9kT2ZHZXR0aW5nQVZhbHVlUHJvZmls ZUZvciBzaG91bGQgcmV0dXJuIGFyZ3VtZW50IHZhbHVlIHByb2ZpbGVzIGV2ZW4gd2hlbiBub2Rl IGFuZCBvcGVyYW5kTm9kZSBhcmUgdGhlIHNhbWUgb3JpZ2luCkluZGV4OiBTb3VyY2UvSmF2YVNj cmlwdENvcmUvZGZnL0RGR1dvcmtsaXN0LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNj cmlwdENvcmUvZGZnL0RGR1dvcmtsaXN0LmNwcAkocmV2aXNpb24gMjUzMzE5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL2RmZy9ERkdXb3JrbGlzdC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTMw Nyw5ICszMDcsNyBAQCB2b2lkIFdvcmtsaXN0Ojp3YWl0VW50aWxBbGxQbGFuc0ZvclZNQXJlCiB2 b2lkIFdvcmtsaXN0OjpkZWxldGVDYW5jZWxsZWRQbGFuc0ZvclZNKExvY2tIb2xkZXImLCBWTSYg dm0pCiB7CiAgICAgUkVMRUFTRV9BU1NFUlQodm0uY3VycmVudFRocmVhZElzSG9sZGluZ0FQSUxv Y2soKSk7Ci0jaWYgIUFTU0VSVF9ESVNBQkxFRAogICAgIEhhc2hTZXQ8UmVmUHRyPFBsYW4+PiBy ZW1vdmVkUGxhbnM7Ci0jZW5kaWYKIAogICAgIGZvciAoc2l6ZV90IGkgPSAwOyBpIDwgbV9jYW5j ZWxsZWRQbGFuc1BlbmRpbmdEZXN0cnVjdGlvbi5zaXplKCk7ICsraSkgewogICAgICAgICBSZWZQ dHI8UGxhbj4gcGxhbiA9IG1fY2FuY2VsbGVkUGxhbnNQZW5kaW5nRGVzdHJ1Y3Rpb25baV07CkBA IC0zMTcsMTggKzMxNSwxNSBAQCB2b2lkIFdvcmtsaXN0OjpkZWxldGVDYW5jZWxsZWRQbGFuc0Zv clZNCiAgICAgICAgICAgICBjb250aW51ZTsKICAgICAgICAgbV9jYW5jZWxsZWRQbGFuc1BlbmRp bmdEZXN0cnVjdGlvbltpLS1dID0gbV9jYW5jZWxsZWRQbGFuc1BlbmRpbmdEZXN0cnVjdGlvbi5s YXN0KCk7CiAgICAgICAgIG1fY2FuY2VsbGVkUGxhbnNQZW5kaW5nRGVzdHJ1Y3Rpb24ucmVtb3Zl TGFzdCgpOwotI2lmICFBU1NFUlRfRElTQUJMRUQKLSAgICAgICAgcmVtb3ZlZFBsYW5zLmFkZChw bGFuKTsKLSNlbmRpZgorICAgICAgICByZW1vdmVkUGxhbnMuYWRkKFdURk1vdmUocGxhbikpOwog ICAgIH0KIAotI2lmICFBU1NFUlRfRElTQUJMRUQKICAgICB3aGlsZSAoIXJlbW92ZWRQbGFucy5p c0VtcHR5KCkpIHsKICAgICAgICAgUmVmUHRyPFBsYW4+IHBsYW4gPSByZW1vdmVkUGxhbnMudGFr ZUFueSgpOwogICAgICAgICBSRUxFQVNFX0FTU0VSVChwbGFuLT5zdGFnZSgpID09IFBsYW46OkNh bmNlbGxlZCk7Ci0gICAgICAgIFJFTEVBU0VfQVNTRVJUKHBsYW4tPnJlZkNvdW50KCkgPT0gMSk7 CisgICAgICAgIGlmIChwbGFuLT5yZWZDb3VudCgpID4gMSkKKyAgICAgICAgICAgIG1fY2FuY2Vs bGVkUGxhbnNQZW5kaW5nRGVzdHJ1Y3Rpb24uYXBwZW5kKFdURk1vdmUocGxhbikpOwogICAgIH0K LSNlbmRpZgogfQogCiB2b2lkIFdvcmtsaXN0OjpyZW1vdmVBbGxSZWFkeVBsYW5zRm9yVk0oVk0m IHZtLCBWZWN0b3I8UmVmUHRyPFBsYW4+LCA4PiYgbXlSZWFkeVBsYW5zKQpAQCAtNDU5LDcgKzQ1 NCw3IEBAIHZvaWQgV29ya2xpc3Q6OnJlbW92ZURlYWRQbGFucyhWTSYgdm0pCiAgICAgICAgICAg ICAgICAgUmVmUHRyPFBsYW4+IHBsYW4gPSBtX3BsYW5zLnRha2UoKml0ZXIpOwogICAgICAgICAg ICAgICAgIHBsYW4tPmNhbmNlbCgpOwogICAgICAgICAgICAgICAgIGlmICghaXNJbk11dGF0b3Ip Ci0gICAgICAgICAgICAgICAgICAgIG1fY2FuY2VsbGVkUGxhbnNQZW5kaW5nRGVzdHJ1Y3Rpb24u YXBwZW5kKHBsYW4pOworICAgICAgICAgICAgICAgICAgICBtX2NhbmNlbGxlZFBsYW5zUGVuZGlu Z0Rlc3RydWN0aW9uLmFwcGVuZChXVEZNb3ZlKHBsYW4pKTsKICAgICAgICAgICAgIH0KICAgICAg ICAgICAgIERlcXVlPFJlZlB0cjxQbGFuPj4gbmV3UXVldWU7CiAgICAgICAgICAgICB3aGlsZSAo IW1fcXVldWUuaXNFbXB0eSgpKSB7Cg==