80982 2012-03-13 05:09:12 -0700 [Qt] RunLoopQt is missing reentrancy guards 2012-05-16 07:23:01 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 81671 1 hausmann hausmann abecsi dinu.jacob webkit.review.bot yael oldest_to_newest 577239 0 hausmann 2012-03-13 05:09:12 -0700 This is mostly relevant to WebKit 2, where the following scenario takes place: Setup: The web process runs two event loops, the main event loop and the event loop with the socket notifier that listens on the socket connected to the uiprocess. 1) The web process receives an incoming message from the ui process to create a new page. 2) The message is received in the WorkQueue's thread of the connection object. It is read from the socket and queued in the list of incoming messages. RunLoop::dispatch() is called with Connection::dispatchMessages as message to call from the main runloop's thread. This will cause the Qt implementation of RunLoop::wakeUp to post a message to the main event loop, causing it to wake up. 2) The main event loop's thread wakes up and calls Connection::dispatchMessages() via RunLoop::performWork(). dispatchMessages() runs a while (true) loop to process all new messages from the queue of incoming messages. 3) In the mean time on the webprocess' message receiving thread, another message is received, one for a specific page - for example loadUrl for the page that was supposed to be created in the previous message. The message is read from the socket, queued in the queue of incoming messages and a message is posted to the main event loop again to wake it up. 4) The message (create new page) is processed, a new WebCore::Page is created, among other things clients like NetworkStateNotifier are created, which initiated the Qt bearer management. Unfortunately the implementation of the latter in some environments does things like calling QEventLoop::exec() while waiting for some system component to respond, which results in the processing of pending events in the main event loop. 5) The wake-up scheduled in step (3) is triggered through the nested event loop, causing dispatchMessages() to be called. This will take the next message (loadUrl) from the queue and start processing it, even though we are still in the process of constructing our initial page. As it happens the message is not processed successfully because the corresponding page id is not registered yet. As a result the start-up sequence is kind of random and sometimes an initially requested url is loaded, sometimes it isn't. The proposed fix is to prevent the Qt implementation of RunLoop to call performWork() recursively. Due to the nature of message queuing, in the above scenario the message would still be received, read and queued in the list of incoming messages. It will consequently be processed by either the while (true) loop of the first, initial dispatchMessages() call or by a subsequent one. 577249 1 131596 hausmann 2012-03-13 05:32:45 -0700 Created attachment 131596 Patch 577279 2 dinu.jacob 2012-03-13 07:07:45 -0700 Very well written detailed description :) This solution makes it Qt specific! 577355 3 hausmann 2012-03-13 08:43:33 -0700 (In reply to comment #2) > Very well written detailed description :) This solution makes it Qt specific! Right, it's a Qt specific solution for what I think is a Qt specific problem at this point. 578354 4 131596 vestbo 2012-03-14 06:43:51 -0700 Comment on attachment 131596 Patch nice! 578393 5 131596 abecsi 2012-03-14 08:02:41 -0700 Comment on attachment 131596 Patch Clearing flags on attachment: 131596 Committed r110699: <http://trac.webkit.org/changeset/110699> 578394 6 abecsi 2012-03-14 08:03:20 -0700 All reviewed patches have been landed. Closing bug. 625401 7 hausmann 2012-05-16 07:23:01 -0700 Change http://trac.webkit.org/changeset/117286 rolled out this change, as it broke modal event loop handling. The symptom of this issue is fixed differently, and there's a growing trend in qt to stop doing evil local-event-loop spinning in synchronous APIs (and instead block on worker threads), so I think it's safe to go back to the previous approach for now. Changing this to WONTFIX in the meantime. 131596 2012-03-13 05:32:45 -0700 2012-03-14 08:02:40 -0700 Patch bug-80982-20120313133242.patch text/plain 2723 hausmann U3VidmVyc2lvbiBSZXZpc2lvbjogMTEwNTQ2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjg3NzYzZjliMmMzMGQ2 ZDRjZTcyYmUwOGY2YmE3ZTY4NzAwM2Y5Yy4uNTI5MjkwNWZmZDAxNWYxMWI1OThkN2Y5NmUwMzZi YjMxMDU1MTYzZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEyLTAzLTEzICBTaW1v biBIYXVzbWFubiAgPHNpbW9uLmhhdXNtYW5uQG5va2lhLmNvbT4KKworICAgICAgICBbUXRdIFJ1 bkxvb3BRdCBpcyBtaXNzaW5nIHJlZW50cmFuY3kgZ3VhcmRzCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04MDk4MgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEF2b2lkIHJlY3Vyc2l2ZSBjYWxscyB0byBSdW5M b29wOjpwZXJmb3JtV29yaygpIHdpdGggYSBzaW1wbGUKKyAgICAgICAgY291bnRpbmcgbWVjaGFu aXNtLCB0byBhdm9pZCBvdXQtb2Ytb3JkZXIgbWVzc2FnZSBkaXNwYXRjaGluZy4KKworICAgICAg ICAqIHBsYXRmb3JtL3F0L1J1bkxvb3BRdC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSdW5Mb29w OjpUaW1lck9iamVjdDo6VGltZXJPYmplY3QpOgorICAgICAgICAoV2ViQ29yZTo6UnVuTG9vcDo6 VGltZXJPYmplY3Q6OnBlcmZvcm1Xb3JrKToKKyAgICAgICAgKFJ1bkxvb3A6OlRpbWVyT2JqZWN0 KToKKwogMjAxMi0wMy0xMiAgTmlrb2xhcyBaaW1tZXJtYW5uICA8bnppbW1lcm1hbm5AcmltLmNv bT4KIAogICAgICAgICBTVkcgQW5pbWF0aW9ucyB1cGRhdGUgYmFzZVZhbCBpbnN0ZWFkIG9mIGFu aW1WYWwKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3F0L1J1bkxvb3BRdC5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9xdC9SdW5Mb29wUXQuY3BwCmluZGV4IDJhODdl MmZjOGY2Njc3NzY5ZjczMTQzZTU3MWUwOWM1YTU0YzZlYmEuLjI4YWI2N2JkZTAxNTVhYzdmNWI4 NjcyNjYyODU4OTQ1NDQ0OTI0ZTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L3F0L1J1bkxvb3BRdC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vcXQvUnVuTG9v cFF0LmNwcApAQCAtMzksMTMgKzM5LDMwIEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKIGNsYXNzIFJ1 bkxvb3A6OlRpbWVyT2JqZWN0IDogcHVibGljIFFPYmplY3QgewogICAgIFFfT0JKRUNUCiBwdWJs aWM6Ci0gICAgVGltZXJPYmplY3QoUnVuTG9vcCogcnVuTG9vcCkgOiBtX3J1bkxvb3AocnVuTG9v cCkKKyAgICBUaW1lck9iamVjdChSdW5Mb29wKiBydW5Mb29wKQorICAgICAgICA6IG1fcnVuTG9v cChydW5Mb29wKQorICAgICAgICAsIG1fcGVuZGluZ1BlcmZvcm1Xb3JrSW52b2NhdGlvbnMoMCkK ICAgICB7CiAgICAgICAgIGludCBtZXRob2RJbmRleCA9IG1ldGFPYmplY3QoKS0+aW5kZXhPZk1l dGhvZCgicGVyZm9ybVdvcmsoKSIpOwogICAgICAgICBtX21ldGhvZCA9IG1ldGFPYmplY3QoKS0+ bWV0aG9kKG1ldGhvZEluZGV4KTsKICAgICB9CiAKLSAgICBRX1NMT1Qgdm9pZCBwZXJmb3JtV29y aygpIHsgbV9ydW5Mb29wLT5wZXJmb3JtV29yaygpOyB9CisgICAgUV9TTE9UIHZvaWQgcGVyZm9y bVdvcmsoKSB7CisgICAgICAgIC8vIEl0IG1heSBoYXBwZW4gdGhhdCBhIHNlY29uZGFyeSB0aHJl YWQgYWRkcyBtb3JlIG1ldGhvZCBpbnZvY2F0aW9ucyB2aWEKKyAgICAgICAgLy8gUnVuTG9vcDo6 ZGlzcGF0Y2goKSwgd2hpY2ggd2lsbCBzY2hlZHVsZSBhIGNhbGwgdG8gdGhpcyBmdW5jdGlvbi4g SWYgZHVyaW5nCisgICAgICAgIC8vIHBlcmZvcm1Xb3JrKCkgZXZlbnQgbG9vcCBtZXNzYWdlcyBn ZXQgcHJvY2Vzc2VkLCBpdCBtYXkgaGFwcGVuIHRoYXQgdGhpcworICAgICAgICAvLyBmdW5jdGlv biBpcyBjYWxsZWQgYWdhaW4uIEluIHRoaXMgY2FzZSB3ZSBzaG91bGQgcHJvdGVjdGVkIG91cnNl bHZlcyBhZ2FpbnN0CisgICAgICAgIC8vIHJlY3Vyc2l2ZSAtIGFuZCB0aHVzIG91dC1vZi1vcmRl ciAtIG1lc3NhZ2UgZGlzcGF0Y2hpbmcgYW5kIGluc3RlYWQgcGVyZm9ybQorICAgICAgICAvLyB0 aGUgd29yayBzZXJpYWxseS4KKyAgICAgICAgbV9wZW5kaW5nUGVyZm9ybVdvcmtJbnZvY2F0aW9u cysrOworICAgICAgICBpZiAobV9wZW5kaW5nUGVyZm9ybVdvcmtJbnZvY2F0aW9ucyA+IDEpCisg ICAgICAgICAgICByZXR1cm47CisKKyAgICAgICAgd2hpbGUgKG1fcGVuZGluZ1BlcmZvcm1Xb3Jr SW52b2NhdGlvbnMpIHsKKyAgICAgICAgICAgIG1fcnVuTG9vcC0+cGVyZm9ybVdvcmsoKTsKKyAg ICAgICAgICAgIG1fcGVuZGluZ1BlcmZvcm1Xb3JrSW52b2NhdGlvbnMtLTsKKyAgICAgICAgfQor ICAgIH0KICAgICBpbmxpbmUgdm9pZCB3YWtlVXAoKSB7IG1fbWV0aG9kLmludm9rZSh0aGlzLCBR dDo6UXVldWVkQ29ubmVjdGlvbik7IH0KIAogcHJvdGVjdGVkOgpAQCAtNTcsNiArNzQsNyBAQCBw cm90ZWN0ZWQ6CiBwcml2YXRlOgogICAgIFJ1bkxvb3AqIG1fcnVuTG9vcDsKICAgICBRTWV0YU1l dGhvZCBtX21ldGhvZDsKKyAgICBpbnQgbV9wZW5kaW5nUGVyZm9ybVdvcmtJbnZvY2F0aW9uczsK IH07CiAKIHN0YXRpYyBRRXZlbnRMb29wKiBjdXJyZW50RXZlbnRMb29wOwo=