174714 2017-07-21 09:06:11 -0700 Drop WTF::initializeDate, if we can stop using -fno-threadsafe-statics 2017-07-23 09:47:00 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified REOPENED https://bugs.webkit.org/show_bug.cgi?id=174752 P2 Normal --- 174748 1 ysuzuki ysuzuki ap benjamin buildbot cdumez cmarcelo commit-queue darin dbates dewei_zhu fujii mark.lam rniwa saam sam slewis oldest_to_newest 1330976 0 ysuzuki 2017-07-21 09:06:11 -0700 [WTF] Drop initializeDate 1330977 1 316092 ysuzuki 2017-07-21 09:07:34 -0700 Created attachment 316092 Patch 1330990 2 316092 commit-queue 2017-07-21 09:59:50 -0700 Comment on attachment 316092 Patch Clearing flags on attachment: 316092 Committed r219732: <http://trac.webkit.org/changeset/219732> 1330991 3 commit-queue 2017-07-21 09:59:51 -0700 All reviewed patches have been landed. Closing bug. 1331018 4 316092 ap 2017-07-21 10:49:42 -0700 Comment on attachment 316092 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316092&action=review > Source/WTF/ChangeLog:8 > + minYear static variable will be initialized in an exclusive manner, which is ensured by C++ "static" semantics. Is that true? I thought we were compiling with no-threadsafe-statics. 1331019 5 mark.lam 2017-07-21 10:51:25 -0700 (In reply to Alexey Proskuryakov from comment #4) > Comment on attachment 316092 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316092&action=review > > > Source/WTF/ChangeLog:8 > > + minYear static variable will be initialized in an exclusive manner, which is ensured by C++ "static" semantics. > > Is that true? I thought we were compiling with no-threadsafe-statics. I think our NeverDestroyed implementation relies on threadsafe-statics. Otherwise, we'll have issues, no? 1331021 6 316092 ysuzuki 2017-07-21 10:54:16 -0700 Comment on attachment 316092 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316092&action=review >>> Source/WTF/ChangeLog:8 >>> + minYear static variable will be initialized in an exclusive manner, which is ensured by C++ "static" semantics. >> >> Is that true? I thought we were compiling with no-threadsafe-statics. > > I think our NeverDestroyed implementation relies on threadsafe-statics. Otherwise, we'll have issues, no? Right, if we compiled WebKit with no-threadsafe-statics, we should already encounter issues with NeverDestroyed. I remember that I grepped this a bit while ago, and I can't find any no-threadsafe-statics in WebKit build settings. Only libwebrtc uses it. But WebKit itself does not. I've grepped again now and I can't find it. 1331083 7 darin 2017-07-21 13:19:14 -0700 We do compile with no-threadsafe-statics. We only use initialization NeverDestroyed when thread safety is not needed. 1331084 8 darin 2017-07-21 13:19:42 -0700 Many of the uses of LazyNeverDestroyed that use std::call_once are for thread safety. 1331085 9 darin 2017-07-21 13:20:20 -0700 Maybe I am wrong. If we really stopped using -fno-threadsafe-statics, then I can do another round of cleanup and get rid of almost all uses of LazyNeverDestroyed! 1331179 10 ap 2017-07-21 16:17:33 -0700 WebKit is built with -fno-threadsafe-statics. Perhaps there is some configuration that enables that, but all that I checked do not. See e.g. https://build.webkit.org/builders/Apple%20Sierra%20Release%20%28Build%29/builds/3721/steps/compile-webkit/logs/stdio 1331184 11 ap 2017-07-21 16:22:17 -0700 Checking wtf/DateMath.cpp specifically, it is also compiled with -fno-threadsafe-statics. 1331283 12 darin 2017-07-21 21:47:09 -0700 So did we create a thread safety problem or not? Can equivalentYearForDST be called from multiple threads? 1331284 13 darin 2017-07-21 21:47:40 -0700 I think we did, and I think this patch probably needs to be rolled out. 1331290 14 ysuzuki 2017-07-21 22:09:54 -0700 (In reply to Darin Adler from comment #7) > We do compile with no-threadsafe-statics. > > We only use initialization NeverDestroyed when thread safety is not needed. I think we have a problem. I remember that Mark changed NeverDestroyed<String>'s ASCIILiteral parameter to StaticStringImpl because this is not safe to access NeverDestroyed<String>. It means that some of NeverDestroyed<String> can be accessed from multiple threads. If so, without threadsafe statics, it is dangerous. I think we will see 1. constructor is called twice, it may break structure during the other thread uses it. 2. we may return half-baked structure if the other thread is constructing it right now. (In reply to Alexey Proskuryakov from comment #10) > WebKit is built with -fno-threadsafe-statics. Perhaps there is some > configuration that enables that, but all that I checked do not. > > See e.g. > https://build.webkit.org/builders/Apple%20Sierra%20Release%20%28Build%29/ > builds/3721/steps/compile-webkit/logs/stdio Wow, that's a problem! I didn't know that this is enabled since any xcodeproje does not specify it. We should adopt this option in CMake build files as well to clarify our build configuration. Filed. https://bugs.webkit.org/show_bug.cgi?id=174747 (In reply to Darin Adler from comment #13) > I think we did, and I think this patch probably needs to be rolled out. Yes, we should roll out. 1331292 15 commit-queue 2017-07-21 22:11:35 -0700 Re-opened since this is blocked by bug 174748 1331299 16 ysuzuki 2017-07-21 22:35:30 -0700 For example, I think JSC/runtime/JSGlobalObjectFunctions.cpp's Bitmaps are racy if threadsafe statics is not ensured. 1331362 17 darin 2017-07-22 10:35:28 -0700 To make our programming easier we can start compiling all of WebKit with thread-safe statics if we can do performance tests and prove that it does not come at an unacceptable performance cost. At the time we originally turned that flag on it was critical for performance. Until then if we accidentally write code with thread safety problems then we should rewrite to avoid those problems. 316092 2017-07-21 09:07:34 -0700 2017-07-21 09:59:50 -0700 Patch bug-174714-20170722010733.patch text/plain 2704 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjE5NzI3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDU3ODZhNjg2NGE3NTc4MWY2OTQzMjky YTEyOWQwNDc4MjE2MDVlZjQuLjY5NGVkODJmN2ExNjYzYTc2YTFhZmM5NGYzNjE3ZDk1Y2VlOTll YzIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDUgKzEsMjEgQEAKIDIwMTctMDctMjEgIFl1c3VrZSBTdXp1a2kgIDx1dGF0 YW5lLnRlYUBnbWFpbC5jb20+CiAKKyAgICAgICAgW1dURl0gRHJvcCBpbml0aWFsaXplRGF0ZQor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTc0NzE0CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgbWluWWVhciBz dGF0aWMgdmFyaWFibGUgd2lsbCBiZSBpbml0aWFsaXplZCBpbiBhbiBleGNsdXNpdmUgbWFubmVy LCB3aGljaCBpcyBlbnN1cmVkIGJ5IEMrKyAic3RhdGljIiBzZW1hbnRpY3MuCisgICAgICAgIFRo dXMsIHdlIGRvIG5vdCBuZWVkIHRvIGNhbGwgaW5pdGlhbGl6ZURhdGUoKSBleHBsaWNpdGx5Lgor CisgICAgICAgICogd3RmL0RhdGVNYXRoLmNwcDoKKyAgICAgICAgKFdURjo6ZXF1aXZhbGVudFll YXJGb3JEU1QpOgorICAgICAgICAoV1RGOjppbml0aWFsaXplRGF0ZXMpOiBEZWxldGVkLgorICAg ICAgICAqIHd0Zi9UaHJlYWRpbmcuY3BwOgorICAgICAgICAoV1RGOjppbml0aWFsaXplVGhyZWFk aW5nKToKKworMjAxNy0wNy0yMSAgWXVzdWtlIFN1enVraSAgPHV0YXRhbmUudGVhQGdtYWlsLmNv bT4KKwogICAgICAgICBbV1RGXSBOZXdseSBhZGRlZCBBdG9taWNTdHJpbmdJbXBsIHNob3VsZCB1 c2UgQnVmZmVySW50ZXJuYWwgc3RhdGljIHN0cmluZyBpZiBTdHJpbmdJbXBsIGlzIHN0YXRpYwog ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTc0NTAxCiAK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL0RhdGVNYXRoLmNwcCBiL1NvdXJjZS9XVEYvd3Rm L0RhdGVNYXRoLmNwcAppbmRleCA1N2UyNGRkZjRlZDgzMjczYjBiYjY0ZTAwMmEwNDFhYTFjZWFh ZTY1Li4zMmZkNWM0NzJkYTQ4Yzk2NDg1NjIwMzY0ZjA3N2NhNDYwZjUzOTYzIDEwMDY0NAotLS0g YS9Tb3VyY2UvV1RGL3d0Zi9EYXRlTWF0aC5jcHAKKysrIGIvU291cmNlL1dURi93dGYvRGF0ZU1h dGguY3BwCkBAIC0zNDYsNyArMzQ2LDcgQEAgaW50IGVxdWl2YWxlbnRZZWFyRm9yRFNUKGludCB5 ZWFyKQogICAgIC8vIEl0IGlzIG9rIGlmIHRoZSBjYWNoZWQgeWVhciBpcyBub3QgdGhlIGN1cnJl bnQgeWVhciBhcyBsb25nIGFzIHRoZSBydWxlcwogICAgIC8vIGZvciBEU1QgZGlkIG5vdCBjaGFu Z2UgYmV0d2VlbiB0aGUgdHdvIHllYXJzOyBpZiB0aGV5IGRpZCB0aGUgYXBwIHdvdWxkIG5lZWQK ICAgICAvLyB0byBiZSByZXN0YXJ0ZWQuCi0gICAgc3RhdGljIGludCBtaW5ZZWFyID0gbWluaW11 bVllYXJGb3JEU1QoKTsKKyAgICBzdGF0aWMgY29uc3QgaW50IG1pblllYXIgeyBtaW5pbXVtWWVh ckZvckRTVCgpIH07CiAgICAgaW50IG1heFllYXIgPSBtYXhpbXVtWWVhckZvckRTVCgpOwogCiAg ICAgaW50IGRpZmZlcmVuY2U7CkBAIC01NTEsMTcgKzU1MSw2IEBAIExvY2FsVGltZU9mZnNldCBj YWxjdWxhdGVMb2NhbFRpbWVPZmZzZXQoZG91YmxlIG1zLCBUaW1lVHlwZSBpbnB1dFRpbWVUeXBl KQogI2VuZGlmCiB9CiAKLXZvaWQgaW5pdGlhbGl6ZURhdGVzKCkKLXsKLSNpZiAhQVNTRVJUX0RJ U0FCTEVECi0gICAgc3RhdGljIGJvb2wgYWxyZWFkeUluaXRpYWxpemVkOwotICAgIEFTU0VSVCgh YWxyZWFkeUluaXRpYWxpemVkKTsKLSAgICBhbHJlYWR5SW5pdGlhbGl6ZWQgPSB0cnVlOwotI2Vu ZGlmCi0KLSAgICBlcXVpdmFsZW50WWVhckZvckRTVCgyMDAwKTsgLy8gTmVlZCB0byBjYWxsIG9u Y2UgdG8gaW5pdGlhbGl6ZSBhIHN0YXRpYyB1c2VkIGluIHRoaXMgZnVuY3Rpb24uCi19Ci0KIHN0 YXRpYyBpbmxpbmUgZG91YmxlIHltZGhtc1RvU2Vjb25kcyhpbnQgeWVhciwgbG9uZyBtb24sIGxv bmcgZGF5LCBsb25nIGhvdXIsIGxvbmcgbWludXRlLCBkb3VibGUgc2Vjb25kKQogewogICAgIGlu dCBtZGF5ID0gZmlyc3REYXlPZk1vbnRoW2lzTGVhcFllYXIoeWVhcildW21vbiAtIDFdOwpkaWZm IC0tZ2l0IGEvU291cmNlL1dURi93dGYvVGhyZWFkaW5nLmNwcCBiL1NvdXJjZS9XVEYvd3RmL1Ro cmVhZGluZy5jcHAKaW5kZXggZGI0NDYyYTU1ZDM2YTE1YmUxMGRjMWYwOWY4NTFiOGNjZjQ1MGNm Ny4uODEwOWI4MWJlMDRiNmFlZDIzMzcxMWUyYTZkNGRiZDE2NDZlZWNlMSAxMDA2NDQKLS0tIGEv U291cmNlL1dURi93dGYvVGhyZWFkaW5nLmNwcAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9UaHJlYWRp bmcuY3BwCkBAIC0yNDgsNyArMjQ4LDYgQEAgdm9pZCBpbml0aWFsaXplVGhyZWFkaW5nKCkKICAg ICAgICAgVGhyZWFkSG9sZGVyOjppbml0aWFsaXplT25jZSgpOwogICAgICAgICBpbml0aWFsaXpl UmFuZG9tTnVtYmVyR2VuZXJhdG9yKCk7CiAgICAgICAgIHd0ZlRocmVhZERhdGEoKTsKLSAgICAg ICAgaW5pdGlhbGl6ZURhdGVzKCk7CiAgICAgICAgIFRocmVhZDo6aW5pdGlhbGl6ZVBsYXRmb3Jt VGhyZWFkaW5nKCk7CiAgICAgfSk7CiB9Cg==