141554 2015-02-12 22:24:44 -0800 TimerBase::m_heapInsertionOrder calculation is racy 2015-02-13 10:30:34 -0800 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 ap ap darin oldest_to_newest 1068852 0 ap 2015-02-12 22:24:44 -0800 TimerBase is used from multiple threads, and insertion order can get incorrect if the threads start overwriting the value. 1068857 1 246510 ap 2015-02-12 23:29:22 -0800 Created attachment 246510 proposed fix Fingers crossed that this won't trigger a static destructor warning anywhere (it did build fine locally for me). I'm not sure if it is legitimate to use uint64_t here, that's not one of the types listed for specializations at <http://en.cppreference.com/w/cpp/atomic/atomic>. 1068921 2 246510 andersca 2015-02-13 08:05:59 -0800 Comment on attachment 246510 proposed fix Just use uint64_t instead of uint_fast64_t. 1068946 3 ap 2015-02-13 10:30:34 -0800 Committed <http://trac.webkit.org/r180058>. Anders also noted that the static object doesn't need to be static, it can be per-thread (perhaps part of ThreadTimers). Looking at the code, it seems like it could use a bit of refactoring for which functions are in TimerBase, and which are in ThreadTimers. I noticed that the counter doesn't actually have a problem with overflow, so I kept it as unsigned. // We need to look at the difference of the insertion orders instead of comparing the two // outright in case of overflow. 246510 2015-02-12 23:29:22 -0800 2015-02-13 10:05:01 -0800 proposed fix TimerInsertionCounter.txt text/plain 2691 ap SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4MDAzMCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDE1LTAyLTEyICBBbGV4ZXkg UHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29tPgorCisgICAgICAgIFRpbWVyQmFzZTo6bV9oZWFw SW5zZXJ0aW9uT3JkZXIgY2FsY3VsYXRpb24gaXMgcmFjeQorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQxNTU0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVXNlIGFuIGF0b21pYyBpbmNyZW1lbnQuIFdoaWxl IGF0IGl0LCBtYWRlIHRoZSBjb3VudGVyIGJpZ2dlciwgc28gdGhhdCBpdCBkb2Vzbid0CisgICAg ICAgIG92ZXJmbG93LgorCisgICAgICAgICogcGxhdGZvcm0vVGltZXIuY3BwOgorICAgICAgICAo V2ViQ29yZTo6VGltZXJIZWFwTGVzc1RoYW5GdW5jdGlvbjo6b3BlcmF0b3IoKSk6CisgICAgICAg IChXZWJDb3JlOjpUaW1lckJhc2U6OnNldE5leHRGaXJlVGltZSk6CisgICAgICAgICogcGxhdGZv cm0vVGltZXIuaDoKKwogMjAxNS0wMi0xMiAgVGltb3RoeSBIb3J0b24gIDx0aW1vdGh5X2hvcnRv bkBhcHBsZS5jb20+CiAKICAgICAgICAgQ3Jhc2hlcyB1bmRlciBkZXRlY3RJdGVtQXJvdW5kSGl0 VGVzdFJlc3VsdCB3aGVuIERhdGFEZXRlY3RvcnMgaXMgbm90IGF2YWlsYWJsZQpJbmRleDogU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vVGltZXIuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL1RpbWVyLmNwcAkocmV2aXNpb24gMTc5NzgyKQorKysgU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vVGltZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xODAsOCArMTgwLDggQEAg aW5saW5lIGJvb2wgVGltZXJIZWFwTGVzc1RoYW5GdW5jdGlvbjo6bwogICAgIAogICAgIC8vIFdl IG5lZWQgdG8gbG9vayBhdCB0aGUgZGlmZmVyZW5jZSBvZiB0aGUgaW5zZXJ0aW9uIG9yZGVycyBp bnN0ZWFkIG9mIGNvbXBhcmluZyB0aGUgdHdvIAogICAgIC8vIG91dHJpZ2h0IGluIGNhc2Ugb2Yg b3ZlcmZsb3cuIAotICAgIHVuc2lnbmVkIGRpZmZlcmVuY2UgPSBhLT5tX2hlYXBJbnNlcnRpb25P cmRlciAtIGItPm1faGVhcEluc2VydGlvbk9yZGVyOwotICAgIHJldHVybiBkaWZmZXJlbmNlIDwg c3RkOjpudW1lcmljX2xpbWl0czx1bnNpZ25lZD46Om1heCgpIC8gMjsKKyAgICB1aW50X2Zhc3Q2 NF90IGRpZmZlcmVuY2UgPSBhLT5tX2hlYXBJbnNlcnRpb25PcmRlciAtIGItPm1faGVhcEluc2Vy dGlvbk9yZGVyOworICAgIHJldHVybiBkaWZmZXJlbmNlIDwgc3RkOjpudW1lcmljX2xpbWl0czx1 aW50X2Zhc3Q2NF90Pjo6bWF4KCkgLyAyOwogfQogCiAvLyAtLS0tLS0tLS0tLS0tLS0tCkBAIC0z ODQsNyArMzg0LDcgQEAgdm9pZCBUaW1lckJhc2U6OnNldE5leHRGaXJlVGltZShkb3VibGUgbgog ICAgIGRvdWJsZSBuZXdUaW1lID0gYWxpZ25lZEZpcmVUaW1lKG5ld1VuYWxpZ25lZFRpbWUpOwog ICAgIGlmIChvbGRUaW1lICE9IG5ld1RpbWUpIHsKICAgICAgICAgbV9uZXh0RmlyZVRpbWUgPSBu ZXdUaW1lOwotICAgICAgICBzdGF0aWMgdW5zaWduZWQgY3VycmVudEhlYXBJbnNlcnRpb25PcmRl cjsKKyAgICAgICAgc3RhdGljIHN0ZDo6YXRvbWljPHVpbnRfZmFzdDY0X3Q+IGN1cnJlbnRIZWFw SW5zZXJ0aW9uT3JkZXI7CiAgICAgICAgIG1faGVhcEluc2VydGlvbk9yZGVyID0gY3VycmVudEhl YXBJbnNlcnRpb25PcmRlcisrOwogCiAgICAgICAgIGJvb2wgd2FzRmlyc3RUaW1lckluSGVhcCA9 IG1faGVhcEluZGV4ID09IDA7CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9UaW1lci5o Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1RpbWVyLmgJKHJldmlzaW9u IDE3OTc4MikKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1RpbWVyLmgJKHdvcmtpbmcgY29w eSkKQEAgLTk5LDcgKzk5LDcgQEAgcHJpdmF0ZToKICAgICBkb3VibGUgbV91bmFsaWduZWROZXh0 RmlyZVRpbWU7IC8vIG1fbmV4dEZpcmVUaW1lIG5vdCBjb25zaWRlcmluZyBhbGlnbm1lbnQgaW50 ZXJ2YWwKICAgICBkb3VibGUgbV9yZXBlYXRJbnRlcnZhbDsgLy8gMCBpZiBub3QgcmVwZWF0aW5n CiAgICAgaW50IG1faGVhcEluZGV4OyAvLyAtMSBpZiBub3QgaW4gaGVhcAotICAgIHVuc2lnbmVk IG1faGVhcEluc2VydGlvbk9yZGVyOyAvLyBVc2VkIHRvIGtlZXAgb3JkZXIgYW1vbmcgZXF1YWwt ZmlyZS10aW1lIHRpbWVycworICAgIHVpbnRfZmFzdDY0X3QgbV9oZWFwSW5zZXJ0aW9uT3JkZXI7 IC8vIFVzZWQgdG8ga2VlcCBvcmRlciBhbW9uZyBlcXVhbC1maXJlLXRpbWUgdGltZXJzCiAgICAg VmVjdG9yPFRpbWVyQmFzZSo+KiBtX2NhY2hlZFRocmVhZEdsb2JhbFRpbWVySGVhcDsKIAogI2lm bmRlZiBOREVCVUcK