149635 2015-09-29 11:29:55 -0700 ParallelHelperPool::runFunctionInParallel() shouldn't allocate, and ParallelHelperPool.h shouldn't be included everywhere 2015-09-30 10:14:29 -0700 1 1 1 Unclassified WebKit JavaScriptCore Other All All RESOLVED INVALID P2 Normal --- 149671 149432 1 fpizlo fpizlo benjamin cmarcelo commit-queue oldest_to_newest 1129307 0 fpizlo 2015-09-29 11:29:55 -0700 Patch forthcoming. 1129311 1 262084 fpizlo 2015-09-29 11:32:01 -0700 Created attachment 262084 the patch 1129314 2 262084 saam 2015-09-29 11:35:25 -0700 Comment on attachment 262084 the patch r=me 1129362 3 fpizlo 2015-09-29 13:26:21 -0700 Landed in http://trac.webkit.org/changeset/190324 1129618 4 ap 2015-09-30 10:02:29 -0700 I suspect that this caused flaky crashes, rdar://problem/22916304 1129620 5 commit-queue 2015-09-30 10:09:30 -0700 Re-opened since this is blocked by bug 149671 1129622 6 fpizlo 2015-09-30 10:14:11 -0700 (In reply to comment #4) > I suspect that this caused flaky crashes, rdar://problem/22916304 Yeah, this needs to be rolled out, and I believe that I understand now why the patch is wrong. The observation behind this patch is that the task cannot run again after runFunctionInParallel returns. That's true. But the way that ParallelHelperPool::helperThreadBody works, it will notify runFunctionInParallel that it's no longer going to run the task *before* it derefs the task. The sequence is: 1) get a task and ref it. 2) run the task. 3) notify task complection. ---> at this point, runFunctionInParallel can return. 4) deref the task. ---> at this point, the helper thread body will crash if runFunctionInParallel returned. We could still find other ways to avoid allocating the task. But I think that it's not worth it if it will be complicated. The easiest mental model is to treat the task as a proper ref-counted object, in which case a stray RefPtr referencing the task after it cannot run anymore is fine. If we wanted to allow stack-allocation of the task, then we'd have to change our mental model to: it's illegal to have a RefPtr to the task after we have consensus that the task cannot run anymore. I don't want to have to think that hard about this code. 1129623 7 fpizlo 2015-09-30 10:14:29 -0700 See https://bugs.webkit.org/show_bug.cgi?id=149635#c6. 262084 2015-09-29 11:32:01 -0700 2015-09-29 11:35:25 -0700 the patch blah.patch text/plain 5204 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTkwMzE3KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBA CisyMDE1LTA5LTI5ICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg UGFyYWxsZWxIZWxwZXJQb29sOjpydW5GdW5jdGlvbkluUGFyYWxsZWwoKSBzaG91bGRuJ3QgYWxs b2NhdGUsIGFuZCBQYXJhbGxlbEhlbHBlclBvb2wuaCBzaG91bGRuJ3QgYmUgaW5jbHVkZWQgZXZl cnl3aGVyZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTQ5NjM1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg SXQgYnVnZ2VkIG1lIHRoYXQgdGhpcyBjaGFuZ2UgY2F1c2VkIGEgd2hvbGUtd29ybGQgcmVjb21w aWxlLiBTbywgSSBjaGFuZ2VkIHRoZSBjb2RlIHNvCisgICAgICAgIHRoYXQgUGFyYWxsZWxIZWxw ZXJQb29sLmggaXMgb25seSBpbmNsdWRlZCBieSBIZWFwLmNwcCBhbmQgbm90IGJ5IEhlYXAuaC4K KworICAgICAgICAqIGhlYXAvSGVhcC5jcHA6CisgICAgICAgIChKU0M6OkhlYXA6OkhlYXApOgor ICAgICAgICAoSlNDOjpIZWFwOjptYXJrUm9vdHMpOgorICAgICAgICAoSlNDOjpIZWFwOjpjb3B5 QmFja2luZ1N0b3Jlcyk6CisgICAgICAgICogaGVhcC9IZWFwLmg6CisKIDIwMTUtMDktMjkgIEZp bGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KIAogICAgICAgICBHQyBjb3B5IHBoYXNlIHNw YW5zIHRvbyBtYW55IGZpbGVzCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFw LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwLmNwcAko cmV2aXNpb24gMTkwMzEwKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvSGVhcC5jcHAJ KHdvcmtpbmcgY29weSkKQEAgLTUzLDYgKzUzLDcgQEAKICNpbmNsdWRlIDxhbGdvcml0aG0+CiAj aW5jbHVkZSA8d3RmL1JBTVNpemUuaD4KICNpbmNsdWRlIDx3dGYvQ3VycmVudFRpbWUuaD4KKyNp bmNsdWRlIDx3dGYvUGFyYWxsZWxIZWxwZXJQb29sLmg+CiAjaW5jbHVkZSA8d3RmL1BhcmFsbGVs VmVjdG9ySXRlcmF0b3IuaD4KICNpbmNsdWRlIDx3dGYvUHJvY2Vzc0lELmg+CiAKQEAgLTM1Nyw3 ICszNTgsNyBAQCBIZWFwOjpIZWFwKFZNKiB2bSwgSGVhcFR5cGUgaGVhcFR5cGUpCiAjaWYgVVNF KENGKQogICAgICwgbV9kZWxheWVkUmVsZWFzZVJlY3Vyc2lvbkNvdW50KDApCiAjZW5kaWYKLSAg ICAsIG1faGVscGVyQ2xpZW50KCZoZWFwSGVscGVyUG9vbCgpKQorICAgICwgbV9oZWxwZXJDbGll bnQoc3RkOjptYWtlX3VuaXF1ZTxQYXJhbGxlbEhlbHBlckNsaWVudD4oJmhlYXBIZWxwZXJQb29s KCkpKQogewogICAgIG1fc3RvcmFnZVNwYWNlLmluaXQoKTsKICAgICBpZiAoT3B0aW9uczo6dmVy aWZ5SGVhcCgpKQpAQCAtNTQ3LDcgKzU0OCw3IEBAIHZvaWQgSGVhcDo6bWFya1Jvb3RzKGRvdWJs ZSBnY1N0YXJ0VGltZSwKIAogICAgIG1fcGFyYWxsZWxNYXJrZXJzU2hvdWxkRXhpdCA9IGZhbHNl OwogCi0gICAgbV9oZWxwZXJDbGllbnQuc2V0RnVuY3Rpb24oCisgICAgbV9oZWxwZXJDbGllbnQt PnNldEZ1bmN0aW9uKAogICAgICAgICBbdGhpc10gKCkgewogICAgICAgICAgICAgU2xvdFZpc2l0 b3IqIHNsb3RWaXNpdG9yOwogICAgICAgICAgICAgewpAQCAtNjA0LDcgKzYwNSw3IEBAIHZvaWQg SGVhcDo6bWFya1Jvb3RzKGRvdWJsZSBnY1N0YXJ0VGltZSwKICAgICAgICAgbV9wYXJhbGxlbE1h cmtlcnNTaG91bGRFeGl0ID0gdHJ1ZTsKICAgICAgICAgbV9tYXJraW5nQ29uZGl0aW9uVmFyaWFi bGUubm90aWZ5QWxsKCk7CiAgICAgfQotICAgIG1faGVscGVyQ2xpZW50LmZpbmlzaCgpOworICAg IG1faGVscGVyQ2xpZW50LT5maW5pc2goKTsKICAgICB1cGRhdGVPYmplY3RDb3VudHMoZ2NTdGFy dFRpbWUpOwogICAgIHJlc2V0VmlzaXRvcnMoKTsKIH0KQEAgLTYzNyw3ICs2MzgsNyBAQCB2b2lk IEhlYXA6OmNvcHlCYWNraW5nU3RvcmVzKCkKICAgICAgICAgLy8gdGhhdCBvdGhlciB0aHJlYWRz IHJ1bi4gVGhhdCdzIGJlY2F1c2UgYWZ0ZXIgcnVuRnVuY3Rpb25JblBhcmFsbGVsKCkgcmV0dXJu cywgdGhlIHRhc2sKICAgICAgICAgLy8gd2UgaGF2ZSBjcmVhdGVkIGlzIG5vdCBnb2luZyB0byBi ZSBydW5uaW5nIGFueW1vcmUuIEhlbmNlLCBldmVyeXRoaW5nIG9uIHRoZSBzdGFjayBoZXJlCiAg ICAgICAgIC8vIG91dGxpdmVzIHRoZSB0YXNrLgotICAgICAgICBtX2hlbHBlckNsaWVudC5ydW5G dW5jdGlvbkluUGFyYWxsZWwoCisgICAgICAgIG1faGVscGVyQ2xpZW50LT5ydW5GdW5jdGlvbklu UGFyYWxsZWwoCiAgICAgICAgICAgICBbJl0gKCkgewogICAgICAgICAgICAgICAgIENvcHlWaXNp dG9yIGNvcHlWaXNpdG9yKCp0aGlzKTsKICAgICAgICAgICAgICAgICAKSW5kZXg6IFNvdXJjZS9K YXZhU2NyaXB0Q29yZS9oZWFwL0hlYXAuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlw dENvcmUvaGVhcC9IZWFwLmgJKHJldmlzaW9uIDE5MDMxMCkKKysrIFNvdXJjZS9KYXZhU2NyaXB0 Q29yZS9oZWFwL0hlYXAuaAkod29ya2luZyBjb3B5KQpAQCAtNDUsNyArNDUsMTEgQEAKICNpbmNs dWRlICJXcml0ZUJhcnJpZXJTdXBwb3J0LmgiCiAjaW5jbHVkZSA8d3RmL0hhc2hDb3VudGVkU2V0 Lmg+CiAjaW5jbHVkZSA8d3RmL0hhc2hTZXQuaD4KLSNpbmNsdWRlIDx3dGYvUGFyYWxsZWxIZWxw ZXJQb29sLmg+CisKK25hbWVzcGFjZSBXVEYgeworY2xhc3MgUGFyYWxsZWxIZWxwZXJDbGllbnQ7 Cit9Cit1c2luZyBXVEY6OlBhcmFsbGVsSGVscGVyQ2xpZW50OwogCiBuYW1lc3BhY2UgSlNDIHsK IApAQCAtNDQxLDcgKzQ0NSw3IEBAIHByaXZhdGU6CiAgICAgTGlzdGFibGVIYW5kbGVyPFdlYWtS ZWZlcmVuY2VIYXJ2ZXN0ZXI+OjpMaXN0IG1fd2Vha1JlZmVyZW5jZUhhcnZlc3RlcnM7CiAgICAg TGlzdGFibGVIYW5kbGVyPFVuY29uZGl0aW9uYWxGaW5hbGl6ZXI+OjpMaXN0IG1fdW5jb25kaXRp b25hbEZpbmFsaXplcnM7CiAKLSAgICBQYXJhbGxlbEhlbHBlckNsaWVudCBtX2hlbHBlckNsaWVu dDsKKyAgICBzdGQ6OnVuaXF1ZV9wdHI8UGFyYWxsZWxIZWxwZXJDbGllbnQ+IG1faGVscGVyQ2xp ZW50OwogfTsKIAogfSAvLyBuYW1lc3BhY2UgSlNDCkluZGV4OiBTb3VyY2UvV1RGL0NoYW5nZUxv Zwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV1RGL0NoYW5nZUxvZwkocmV2aXNpb24gMTkwMzE3KQor KysgU291cmNlL1dURi9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwxMyBAQAor MjAxNS0wOS0yOSAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgorCisgICAgICAgIFBh cmFsbGVsSGVscGVyUG9vbDo6cnVuRnVuY3Rpb25JblBhcmFsbGVsKCkgc2hvdWxkbid0IGFsbG9j YXRlLCBhbmQgUGFyYWxsZWxIZWxwZXJQb29sLmggc2hvdWxkbid0IGJlIGluY2x1ZGVkIGV2ZXJ5 d2hlcmUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0 OTYzNQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICog d3RmL1BhcmFsbGVsSGVscGVyUG9vbC5oOgorICAgICAgICAoV1RGOjpQYXJhbGxlbEhlbHBlckNs aWVudDo6cnVuRnVuY3Rpb25JblBhcmFsbGVsKTogU3RhY2stYWxsb2NhdGUgdGhlIHRhc2sgaW5z dGVhZCBvZiBoZWFwLWFsbG9jYXRpbmcgaXQuCisKIDIwMTUtMDktMjkgIEZpbGlwIFBpemxvICA8 ZnBpemxvQGFwcGxlLmNvbT4KIAogICAgICAgICBHQyBjb3B5IHBoYXNlIHNwYW5zIHRvbyBtYW55 IGZpbGVzCkluZGV4OiBTb3VyY2UvV1RGL3d0Zi9QYXJhbGxlbEhlbHBlclBvb2wuaAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvV1RGL3d0Zi9QYXJhbGxlbEhlbHBlclBvb2wuaAkocmV2aXNpb24gMTkw MzA5KQorKysgU291cmNlL1dURi93dGYvUGFyYWxsZWxIZWxwZXJQb29sLmgJKHdvcmtpbmcgY29w eSkKQEAgLTE0OCwxNCArMTQ4LDE5IEBAIHB1YmxpYzoKICAgICAvLyBjbGllbnQtPmZpbmlzaCgp OwogICAgIFdURl9FWFBPUlRfUFJJVkFURSB2b2lkIHJ1blRhc2tJblBhcmFsbGVsKFJlZlB0cjxT aGFyZWRUYXNrPik7CiAKLSAgICAvLyBFcXVpdmFsZW50IHRvOgorICAgIC8vIFNlbWFudGljYWxs eSBlcXVpdmFsZW50IHRvOgogICAgIC8vIGNsaWVudC0+c2V0RnVuY3Rpb24oZnVuY3Rvcik7CiAg ICAgLy8gY2xpZW50LT5kb1NvbWVIZWxwaW5nKCk7CiAgICAgLy8gY2xpZW50LT5maW5pc2goKTsK KyAgICAvLworICAgIC8vIEV4Y2VwdCwgdW5saWtlIHRoZSBhYm92ZSBzZXF1ZW5jZSwgdGhpcyB3 b24ndCBhbGxvY2F0ZSB0aGUgdGFzayBpbiB0aGUgaGVhcC4gVGhpcyBhbGxvY2F0ZXMKKyAgICAv LyB0aGUgdGFzayBvbiB0aGUgc3RhY2sgYmVjYXVzZSBpdCBrbm93cyB0aGF0IHRoZSB0YXNrIGlz IG5vdCByZWFjaGFibGUgYWZ0ZXIgdGhpcyBtZXRob2QgaXMKKyAgICAvLyBkb25lLgogICAgIHRl bXBsYXRlPHR5cGVuYW1lIEZ1bmN0b3I+CiAgICAgdm9pZCBydW5GdW5jdGlvbkluUGFyYWxsZWwo Y29uc3QgRnVuY3RvciYgZnVuY3RvcikKICAgICB7Ci0gICAgICAgIHJ1blRhc2tJblBhcmFsbGVs KGNyZWF0ZVNoYXJlZFRhc2soZnVuY3RvcikpOworICAgICAgICBTaGFyZWRUYXNrRnVuY3RvcjxG dW5jdG9yPiBzaGFyZWRUYXNrKGZ1bmN0b3IpOworICAgICAgICBydW5UYXNrSW5QYXJhbGxlbCgm c2hhcmVkVGFzayk7CiAgICAgfQogCiAgICAgUGFyYWxsZWxIZWxwZXJQb29sJiBwb29sKCkgeyBy ZXR1cm4gKm1fcG9vbDsgfQo=