30876 2009-10-28 13:13:41 -0700 [GTK] Make timeout callbacks hold the GDK lock 2014-04-08 18:21:37 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) PC Linux RESOLVED INVALID P2 Normal --- 1 xan.lopez webkit-unassigned alex danilo eric evan ggaren gustavo jmalonzo mrobinson zecke oldest_to_newest 158675 0 xan.lopez 2009-10-28 13:13:41 -0700 Timeouts, idles, and other similar things are not executed holding the GDK lock (since they are implemented in glib, which does not have access to that). In multithreaded GTK+ applications only one thread can use GTK+ at any given moment, so if you use timeouts or idles you need to be careful and hold the GDK lock if you are going to call GTK+ APIs. WebKitGTK+ is multithreaded, so this applies to us. I have identified three timeouts in our codebase, all of them not holding the GDK lock. Two are in the DRT app, and the other one is the timeout used to schedule functions to be called in the main thread. I guess the DRT changes are uncontroversial, but kov fears that making the other timeout hold the lock could be a performance regression in some cases. It seems to me, though, that there are some obvious (?) situations where JS will end up calling GTK+, like window.open, so I if I'm not wrong we have to either a) do this b) identify all the places that can be called from JS and protect them with locks. b) is similar to our current situation with calling webkit_init on demand, which seems to be a healthy source of bugs. I'm CCing ggaren and posting my proposed patch to kick off some discussion. 158679 1 42051 xan.lopez 2009-10-28 13:18:53 -0700 Created attachment 42051 gdklock.diff 158862 2 zecke 2009-10-29 05:01:55 -0700 I have a slightly modified concern. In the long run we want to build a separate JSC so and this might not require a GUI and linking to GDK might seem excessive. From what I see the modified function in JSC will not be executed from within JSC though.. Did you check WebCore/platform/gtk/SharedTimerGtk.cpp? is that safe? 158997 3 xan.lopez 2009-10-29 11:50:48 -0700 (In reply to comment #2) > I have a slightly modified concern. In the long run we want to build a separate > JSC so and this might not require a GUI and linking to GDK might seem > excessive. From what I see the modified function in JSC will not be executed > from within JSC though.. That's right, and that sucks. So maybe we'll have to go the "find all the places that need protection and protect them...". > Did you check WebCore/platform/gtk/SharedTimerGtk.cpp? is that safe? No, it's exactly the same. g_timeout_add/g_idle_add don't hold the GDK lock in their callbacks, so if some other thread kicks in when that is being executed, and the callback ends up calling GTK+, you'll have two threads using GTK+ at the same time. 159927 4 gustavo 2009-11-02 14:31:34 -0800 <kov> if any gdk function is called, we need to hold the lock before doing that, otherwise we'll fail on multithreaded programs <ggaren> kov: scheduleDispatchFunctionsOnMainThread can be called by anyone, so, yes. kov: processWork will call into webkit, so, yes. <ggaren> kov: waitToDumpWatchdogFired will probably not call into any gui code So, looks like we do need that; we will now have to figure out how to make this without linking jsc to gdk, if possible =D 421217 5 danilo 2011-06-15 06:17:22 -0700 I wonder if this could be behind https://bugs.webkit.org/show_bug.cgi?id=58789 as well? 421285 6 mrobinson 2011-06-15 07:58:05 -0700 (In reply to comment #5) > I wonder if this could be behind https://bugs.webkit.org/show_bug.cgi?id=58789 as well? If that's happening on the main thread and doesn't involve GDK/GTK, probably not, but perhaps. 998977 7 mrobinson 2014-04-08 18:21:37 -0700 This probably isn't an issue with WebKit2 any longer, but feel free to reopen if I'm wrong. 42051 2009-10-28 13:18:53 -0700 2010-06-10 20:18:20 -0700 gdklock.diff gdklock.diff text/plain 6144 xan.lopez RnJvbSA3Zjc1YzdkMWVlYjAxOTQxNWYwODM0MzBhZDJiNDhiZDMxNWNlODI2IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBYYW4gTG9wZXogPHhsb3BlekBpZ2FsaWEuY29tPgpEYXRlOiBX ZWQsIDI4IE9jdCAyMDA5IDIyOjE3OjI3ICswMjAwClN1YmplY3Q6IFtQQVRDSF0gMjAwOS0xMC0y OCAgWGFuIExvcGV6ICA8eGxvcGV6QGlnYWxpYS5jb20+CgogICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgoKICAgICAgICBbR1RLXSBNYWtlIHRpbWVvdXQgY2FsbGJhY2tzIGhvbGQg dGhlIEdESyBsb2NrCiAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTMwODc2CgogICAgICAgIEdpdmUgSlNDIGFjY2VzcyB0byBHVEsrIEFQSXMsIG5lZWRlZCBm b3IgZ2RrX3RocmVhZHNfYWRkX3RpbWVvdXQuCgogICAgICAgICogR05VbWFrZWZpbGUuYW06CgpX ZWJLaXRUb29sczoKCjIwMDktMTAtMjggIFhhbiBMb3BleiAgPHhsb3BlekBpZ2FsaWEuY29tPgoK ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KCiAgICAgICAgW0dUS10gTWFrZSB0 aW1lb3V0IGNhbGxiYWNrcyBob2xkIHRoZSBHREsgbG9jawogICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMDg3NgoKICAgICAgICBIb2xkIHRoZSBHREsgbG9j ayBpbiB0aGUgRFJUIHRpbWVvdXRzLCBzaW5jZSB3ZSBhcmUgYQogICAgICAgIG11bHRpLXRocmVh ZGVkIGFwcC4KCiAgICAgICAgKiBEdW1wUmVuZGVyVHJlZS9ndGsvRHVtcFJlbmRlclRyZWUuY3Bw OgogICAgICAgICh3ZWJWaWV3TG9hZEZpbmlzaGVkKToKICAgICAgICAqIER1bXBSZW5kZXJUcmVl L2d0ay9MYXlvdXRUZXN0Q29udHJvbGxlckd0ay5jcHA6CiAgICAgICAgKExheW91dFRlc3RDb250 cm9sbGVyOjpzZXRXYWl0VG9EdW1wKToKCkphdmFTY3JpcHRDb3JlOgoKMjAwOS0xMC0yOCAgWGFu IExvcGV6ICA8eGxvcGV6QGlnYWxpYS5jb20+CgogICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgoKICAgICAgICBbR1RLXSBNYWtlIHRpbWVvdXQgY2FsbGJhY2tzIGhvbGQgdGhlIEdE SyBsb2NrCiAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTMw ODc2CgogICAgICAgIEhvbGQgdGhlIEdESyBsb2NrIHdoZW4gZXhlY3V0aW5nIGNvZGUgaW4gdGhl IG1haW4gdGhyZWFkIGZyb20gSlMsCiAgICAgICAgc2luY2UgaXQgY2FuIHBvdGVudGlhbGx5IGNh bGwgaW50byBHVEsrLgoKICAgICAgICAqIHd0Zi9ndGsvTWFpblRocmVhZEd0ay5jcHA6CiAgICAg ICAgKFdURjo6c2NoZWR1bGVEaXNwYXRjaEZ1bmN0aW9uc09uTWFpblRocmVhZCk6Ci0tLQogQ2hh bmdlTG9nICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgfCAgIDExICsr KysrKysrKysrCiBHTlVtYWtlZmlsZS5hbSAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICB8ICAgIDIgKysKIEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAg ICAgICAgICAgIHwgICAxMyArKysrKysrKysrKysrCiBKYXZhU2NyaXB0Q29yZS93dGYvZ3RrL01h aW5UaHJlYWRHdGsuY3BwICAgICAgICAgICB8ICAgIDMgKystCiBXZWJLaXRUb29scy9DaGFuZ2VM b2cgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB8ICAgMTUgKysrKysrKysrKysrKysrCiBX ZWJLaXRUb29scy9EdW1wUmVuZGVyVHJlZS9ndGsvRHVtcFJlbmRlclRyZWUuY3BwICB8ICAgIDIg Ky0KIC4uLi9EdW1wUmVuZGVyVHJlZS9ndGsvTGF5b3V0VGVzdENvbnRyb2xsZXJHdGsuY3BwIHwg ICAgMiArLQogNyBmaWxlcyBjaGFuZ2VkLCA0NSBpbnNlcnRpb25zKCspLCAzIGRlbGV0aW9ucygt KQoKZGlmZiAtLWdpdCBhL0NoYW5nZUxvZyBiL0NoYW5nZUxvZwppbmRleCBkYzQzZTBlLi5kZDI3 NWQ4IDEwMDY0NAotLS0gYS9DaGFuZ2VMb2cKKysrIGIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQg QEAKKzIwMDktMTAtMjggIFhhbiBMb3BleiAgPHhsb3BlekBpZ2FsaWEuY29tPgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtHVEtdIE1ha2UgdGltZW91 dCBjYWxsYmFja3MgaG9sZCB0aGUgR0RLIGxvY2sKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTMwODc2CisKKyAgICAgICAgR2l2ZSBKU0MgYWNjZXNzIHRv IEdUSysgQVBJcywgbmVlZGVkIGZvciBnZGtfdGhyZWFkc19hZGRfdGltZW91dC4KKworICAgICAg ICAqIEdOVW1ha2VmaWxlLmFtOgorCiAyMDA5LTEwLTI2ICBIb2xnZXIgSGFucyBQZXRlciBGcmV5 dGhlciAgPHplY2tlQHNlbGZpc2gub3JnPgogCiAgICAgICAgIFJ1YmJlci1zdGFtcGVkIGJ5IERh cmluIEFkbGVyLgpkaWZmIC0tZ2l0IGEvR05VbWFrZWZpbGUuYW0gYi9HTlVtYWtlZmlsZS5hbQpp bmRleCAwODAwZTgzLi4zNzBjYjI1IDEwMDY0NAotLS0gYS9HTlVtYWtlZmlsZS5hbQorKysgYi9H TlVtYWtlZmlsZS5hbQpAQCAtMTcwLDYgKzE3MCw3IEBAIGxpYkphdmFTY3JpcHRDb3JlX2xhX1NP VVJDRVMgPSBcCiBsaWJKYXZhU2NyaXB0Q29yZV9sYV9MSUJBREQgPSBcCiAJJChVTklDT0RFX0xJ QlMpIFwKIAkkKEdMSUJfTElCUykgXAorCSQoR1RLX0xJQlMpIFwKIAktbHB0aHJlYWQKIAogbGli SmF2YVNjcmlwdENvcmVfbGFfQ1hYRkxBR1MgPSBcCkBAIC0xODEsNiArMTgyLDcgQEAgbGliSmF2 YVNjcmlwdENvcmVfbGFfQ0ZMQUdTID0gXAogCS1PMyBcCiAJJChnbG9iYWxfY2ZsYWdzKSBcCiAJ JChHTElCX0NGTEFHUykgXAorCSQoR1RLX0NGTEFHUykgXAogCSQoVU5JQ09ERV9DRkxBR1MpCiAK IGxpYkphdmFTY3JpcHRDb3JlX2xhX0NQUEZMQUdTID0gXApkaWZmIC0tZ2l0IGEvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDZkMWYwYjUu LjRmMWQ3MzEgMTAwNjQ0Ci0tLSBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZworKysgYi9KYXZh U2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNiBAQAorMjAwOS0xMC0yOCAgWGFuIExv cGV6ICA8eGxvcGV6QGlnYWxpYS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgW0dUS10gTWFrZSB0aW1lb3V0IGNhbGxiYWNrcyBob2xkIHRoZSBH REsgbG9jaworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MzA4NzYKKworICAgICAgICBIb2xkIHRoZSBHREsgbG9jayB3aGVuIGV4ZWN1dGluZyBjb2RlIGlu IHRoZSBtYWluIHRocmVhZCBmcm9tIEpTLAorICAgICAgICBzaW5jZSBpdCBjYW4gcG90ZW50aWFs bHkgY2FsbCBpbnRvIEdUSysuCisKKyAgICAgICAgKiB3dGYvZ3RrL01haW5UaHJlYWRHdGsuY3Bw OgorICAgICAgICAoV1RGOjpzY2hlZHVsZURpc3BhdGNoRnVuY3Rpb25zT25NYWluVGhyZWFkKToK KwogMjAwOS0xMC0yNiAgSG9sZ2VyIEhhbnMgUGV0ZXIgRnJleXRoZXIgIDx6ZWNrZUBzZWxmaXNo Lm9yZz4KIAogICAgICAgICBSdWJiZXItc3RhbXBlZCBieSBEYXJpbiBBZGxlci4KZGlmZiAtLWdp dCBhL0phdmFTY3JpcHRDb3JlL3d0Zi9ndGsvTWFpblRocmVhZEd0ay5jcHAgYi9KYXZhU2NyaXB0 Q29yZS93dGYvZ3RrL01haW5UaHJlYWRHdGsuY3BwCmluZGV4IDc2MjQyNDcuLjllODhhMjYgMTAw NjQ0Ci0tLSBhL0phdmFTY3JpcHRDb3JlL3d0Zi9ndGsvTWFpblRocmVhZEd0ay5jcHAKKysrIGIv SmF2YVNjcmlwdENvcmUvd3RmL2d0ay9NYWluVGhyZWFkR3RrLmNwcApAQCAtMzEsNiArMzEsNyBA QAogI2luY2x1ZGUgIk1haW5UaHJlYWQuaCIKIAogI2luY2x1ZGUgPGdsaWIuaD4KKyNpbmNsdWRl IDxndGsvZ3RrLmg+CiAKIG5hbWVzcGFjZSBXVEYgewogCkBAIC00Niw3ICs0Nyw3IEBAIHN0YXRp YyBnYm9vbGVhbiB0aW1lb3V0RmlyZWQoZ3BvaW50ZXIpCiAKIHZvaWQgc2NoZWR1bGVEaXNwYXRj aEZ1bmN0aW9uc09uTWFpblRocmVhZCgpCiB7Ci0gICAgZ190aW1lb3V0X2FkZCgwLCB0aW1lb3V0 RmlyZWQsIDApOworICAgIGdka190aHJlYWRzX2FkZF90aW1lb3V0KDAsIHRpbWVvdXRGaXJlZCwg MCk7CiB9CiAKIH0gLy8gbmFtZXNwYWNlIFdURgpkaWZmIC0tZ2l0IGEvV2ViS2l0VG9vbHMvQ2hh bmdlTG9nIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCmluZGV4IGNhNjc5YmMuLmQ1ZThlMjYgMTAw NjQ0Ci0tLSBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZworKysgYi9XZWJLaXRUb29scy9DaGFuZ2VM b2cKQEAgLTEsMyArMSwxOCBAQAorMjAwOS0xMC0yOCAgWGFuIExvcGV6ICA8eGxvcGV6QGlnYWxp YS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg W0dUS10gTWFrZSB0aW1lb3V0IGNhbGxiYWNrcyBob2xkIHRoZSBHREsgbG9jaworICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzA4NzYKKworICAgICAgICBI b2xkIHRoZSBHREsgbG9jayBpbiB0aGUgRFJUIHRpbWVvdXRzLCBzaW5jZSB3ZSBhcmUgYQorICAg ICAgICBtdWx0aS10aHJlYWRlZCBhcHAuCisKKyAgICAgICAgKiBEdW1wUmVuZGVyVHJlZS9ndGsv RHVtcFJlbmRlclRyZWUuY3BwOgorICAgICAgICAod2ViVmlld0xvYWRGaW5pc2hlZCk6CisgICAg ICAgICogRHVtcFJlbmRlclRyZWUvZ3RrL0xheW91dFRlc3RDb250cm9sbGVyR3RrLmNwcDoKKyAg ICAgICAgKExheW91dFRlc3RDb250cm9sbGVyOjpzZXRXYWl0VG9EdW1wKToKKwogMjAwOS0xMC0y NyAgS2VubmV0aCBSb2hkZSBDaHJpc3RpYW5zZW4gIDxrZW5uZXRoQHdlYmtpdC5vcmc+CiAKICAg ICAgICAgUnViYmVyc3RhbXBlZCBieSBPbGl2ZXIgSHVudC4KZGlmZiAtLWdpdCBhL1dlYktpdFRv b2xzL0R1bXBSZW5kZXJUcmVlL2d0ay9EdW1wUmVuZGVyVHJlZS5jcHAgYi9XZWJLaXRUb29scy9E dW1wUmVuZGVyVHJlZS9ndGsvRHVtcFJlbmRlclRyZWUuY3BwCmluZGV4IGNmODZiMzAuLjVjODZi YTUgMTAwNjQ0Ci0tLSBhL1dlYktpdFRvb2xzL0R1bXBSZW5kZXJUcmVlL2d0ay9EdW1wUmVuZGVy VHJlZS5jcHAKKysrIGIvV2ViS2l0VG9vbHMvRHVtcFJlbmRlclRyZWUvZ3RrL0R1bXBSZW5kZXJU cmVlLmNwcApAQCAtNTY0LDcgKzU2NCw3IEBAIHN0YXRpYyB2b2lkIHdlYlZpZXdMb2FkRmluaXNo ZWQoV2ViS2l0V2ViVmlldyogdmlldywgV2ViS2l0V2ViRnJhbWUqIGZyYW1lLCB2b2lkCiAgICAg ICAgIHJldHVybjsKIAogICAgIGlmIChXb3JrUXVldWU6OnNoYXJlZCgpLT5jb3VudCgpKQotICAg ICAgICBnX3RpbWVvdXRfYWRkKDAsIHByb2Nlc3NXb3JrLCAwKTsKKyAgICAgICAgZ2RrX3RocmVh ZHNfYWRkX3RpbWVvdXQoMCwgcHJvY2Vzc1dvcmssIDApOwogICAgICBlbHNlCiAgICAgICAgIGR1 bXAoKTsKIH0KZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0R1bXBSZW5kZXJUcmVlL2d0ay9MYXlv dXRUZXN0Q29udHJvbGxlckd0ay5jcHAgYi9XZWJLaXRUb29scy9EdW1wUmVuZGVyVHJlZS9ndGsv TGF5b3V0VGVzdENvbnRyb2xsZXJHdGsuY3BwCmluZGV4IGIwMWRjZjQuLmJjMzE5MjQgMTAwNjQ0 Ci0tLSBhL1dlYktpdFRvb2xzL0R1bXBSZW5kZXJUcmVlL2d0ay9MYXlvdXRUZXN0Q29udHJvbGxl ckd0ay5jcHAKKysrIGIvV2ViS2l0VG9vbHMvRHVtcFJlbmRlclRyZWUvZ3RrL0xheW91dFRlc3RD b250cm9sbGVyR3RrLmNwcApAQCAtMjY0LDcgKzI2NCw3IEBAIHZvaWQgTGF5b3V0VGVzdENvbnRy b2xsZXI6OnNldFdhaXRUb0R1bXAoYm9vbCB3YWl0VW50aWxEb25lKQogCiAgICAgbV93YWl0VG9E dW1wID0gd2FpdFVudGlsRG9uZTsKICAgICBpZiAobV93YWl0VG9EdW1wICYmICF3YWl0VG9EdW1w V2F0Y2hkb2cpCi0gICAgICAgIHdhaXRUb0R1bXBXYXRjaGRvZyA9IGdfdGltZW91dF9hZGRfc2Vj b25kcyh0aW1lb3V0U2Vjb25kcywgd2FpdFRvRHVtcFdhdGNoZG9nRmlyZWQsIDApOworICAgICAg ICB3YWl0VG9EdW1wV2F0Y2hkb2cgPSBnZGtfdGhyZWFkc19hZGRfdGltZW91dF9zZWNvbmRzKHRp bWVvdXRTZWNvbmRzLCB3YWl0VG9EdW1wV2F0Y2hkb2dGaXJlZCwgMCk7CiB9CiAKIGludCBMYXlv dXRUZXN0Q29udHJvbGxlcjo6d2luZG93Q291bnQoKQotLSAKMS42LjUuMQoK