101132 2012-11-02 17:12:59 -0700 [EFL] Use mutex locker in wakeUp() to ensure thread-safety. 2012-11-28 06:39:27 -0800 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 101135 1 bw80.lee bw80.lee cdumez gyuyoung.kim levin+threading lucas.de.marchi rakuco sw0524.lee webkit.review.bot youngtaeck.song oldest_to_newest 757775 0 bw80.lee 2012-11-02 17:12:59 -0700 RunLoop::wakeUp() can be called by multiple thread. but ecore_pipe_write() function itself doesn't consider the thread-safety. Additional locking for the ecore pipe is needed. 757851 1 172193 bw80.lee 2012-11-02 19:16:35 -0700 Created attachment 172193 Patch 770114 2 172193 cdumez 2012-11-18 05:24:42 -0800 Comment on attachment 172193 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172193&action=review > Source/WebCore/platform/efl/RunLoopEfl.cpp:105 > + MutexLocker locker(m_pipeLock); The caller (RunLoop::dispatch()) seems to be already thread safe (using m_functionQueueLock mutex). Could you elaborate why we need an additional mutex in RunLoop::wakeUp()? Also note that no other port seems to be using a mutex in RunLoop::wakeUp(). We should avoid mutexes unless strictly required as it may impact performance. 773460 3 bw80.lee 2012-11-21 18:12:45 -0800 (In reply to comment #2) > (From update of attachment 172193 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172193&action=review > > > Source/WebCore/platform/efl/RunLoopEfl.cpp:105 > > + MutexLocker locker(m_pipeLock); > > The caller (RunLoop::dispatch()) seems to be already thread safe (using m_functionQueueLock mutex). Could you elaborate why we need an additional mutex in RunLoop::wakeUp()? The mutex that is used in RunLoop::dispatch() is not for the wakeUp() but for the m_functionQueue. So I have a patch about this. (You can see the details in Bug 98580) As you can see in the comment of the bug, RunLoop::wakeUp() is used in the Connection::wakeUpRunLoop() without any protection. So protecting wakeUp() itself is needed. > Also note that no other port seems to be using a mutex in RunLoop::wakeUp(). We should avoid mutexes unless strictly required as it may impact performance. Yes, protecting with mutex can make some performance issue. But as I know, other ports use thread-safe function for wake up loop. Unfortunately, EFL doesn't provide such thing for now. (And if it provide such API, the API will use mutex or some other protection mechanism to ensure the thread-safety) 774222 4 172193 gyuyoung.kim 2012-11-22 22:36:56 -0800 Comment on attachment 172193 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172193&action=review >>> Source/WebCore/platform/efl/RunLoopEfl.cpp:105 >>> + MutexLocker locker(m_pipeLock); >> >> The caller (RunLoop::dispatch()) seems to be already thread safe (using m_functionQueueLock mutex). Could you elaborate why we need an additional mutex in RunLoop::wakeUp()? >> Also note that no other port seems to be using a mutex in RunLoop::wakeUp(). We should avoid mutexes unless strictly required as it may impact performance. > > The mutex that is used in RunLoop::dispatch() is not for the wakeUp() but for the m_functionQueue. So I have a patch about this. (You can see the details in Bug 98580) > As you can see in the comment of the bug, RunLoop::wakeUp() is used in the Connection::wakeUpRunLoop() without any protection. > So protecting wakeUp() itself is needed. Byungwoo, I asked to raster if ecore_pipe_write is thread-safety. According to raster, ecore_pipe_write is intended for the thread. As far as I understand, ecore_pipe_write is one-way communication from thread to mainloop which created a pipe. Then, thread just writes data to the pipe. So, raster said ecore_pipe_write is thread safety. How do you think ? 774235 5 bw80.lee 2012-11-22 23:14:51 -0800 (In reply to comment #4) > (From update of attachment 172193 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172193&action=review > > >>> Source/WebCore/platform/efl/RunLoopEfl.cpp:105 > >>> + MutexLocker locker(m_pipeLock); > >> > >> The caller (RunLoop::dispatch()) seems to be already thread safe (using m_functionQueueLock mutex). Could you elaborate why we need an additional mutex in RunLoop::wakeUp()? > >> Also note that no other port seems to be using a mutex in RunLoop::wakeUp(). We should avoid mutexes unless strictly required as it may impact performance. > > > > The mutex that is used in RunLoop::dispatch() is not for the wakeUp() but for the m_functionQueue. So I have a patch about this. (You can see the details in Bug 98580) > > As you can see in the comment of the bug, RunLoop::wakeUp() is used in the Connection::wakeUpRunLoop() without any protection. > > So protecting wakeUp() itself is needed. > > Byungwoo, I asked to raster if ecore_pipe_write is thread-safety. According to raster, ecore_pipe_write is intended for the thread. As far as I understand, ecore_pipe_write is one-way communication from thread to mainloop which created a pipe. Then, thread just writes data to the pipe. So, raster said ecore_pipe_write is thread safety. How do you think ? Really? I checked the ecore_pipe_write() in ecore v1.7.1. First of all, there is no locking in the function. And the function has two parts. 1. writing the length of data to the pipe. 2. writing the data to the pipe. As my understanding, to ensure thread-safety of the ecore_pipe_write(), the writing operation of all those ([data length][raw data]) should be atomic. If multiple threads call the ecore_pipe_write() at one time, the order of pipe data can be invalid - like [length of A][length of B][data of B][data of A]. I'm not sure that _ecore_pipe_read has some protection or recovery logic about this situation. Please let me know if there are some misunderstanding. 774252 6 sw0524.lee 2012-11-22 23:48:11 -0800 I agree with Byungwoo Lee. It seems to me that ecore_pipe_write() in ecore v1.7.1 is not thread-safe because it has the possibility of message format broken when writing messages at the same times. It is my opinion. 774282 7 cdumez 2012-11-23 00:46:18 -0800 For the record, I agree with byungwoo as well. 774297 8 gyuyoung.kim 2012-11-23 01:18:32 -0800 According to raster, ecore_pipe_write is intended for a thread. So, he recommends to make an ecore_pipe per thread. I also think we need to use a lock for current usage. But, I'm not clear to use ecore_pipe_write here yet. If this is urgent issue, I don't mind to land this code at the moment. If not, I would like to dig this a little further. I wanna check why ecore_pipe_write is used here. 776376 9 bw80.lee 2012-11-26 22:53:26 -0800 (In reply to comment #8) > According to raster, ecore_pipe_write is intended for a thread. So, he recommends to make an ecore_pipe per thread. I also think we need to use a lock for current usage. But, I'm not clear to use ecore_pipe_write here yet. If this is urgent issue, I don't mind to land this code at the moment. If not, I would like to dig this a little further. I wanna check why ecore_pipe_write is used here. Yes, it is the fundamental question that 'Why EFL WebKit uses the ecore pipe to wake up main loop?' :) Maybe there are many people who have the same question, and so do I. Is there anyone who knows the history about this? (Maybe it is related with some priority between the loop event items, but I'm not sure) 776382 10 cdumez 2012-11-26 22:57:51 -0800 (In reply to comment #9) > (In reply to comment #8) > > According to raster, ecore_pipe_write is intended for a thread. So, he recommends to make an ecore_pipe per thread. I also think we need to use a lock for current usage. But, I'm not clear to use ecore_pipe_write here yet. If this is urgent issue, I don't mind to land this code at the moment. If not, I would like to dig this a little further. I wanna check why ecore_pipe_write is used here. > > Yes, it is the fundamental question that 'Why EFL WebKit uses the ecore pipe to wake up main loop?' :) > > Maybe there are many people who have the same question, and so do I. > > Is there anyone who knows the history about this? > (Maybe it is related with some priority between the loop event items, but I'm not sure) Well, EFL also uses ecore pipe to wake up the main loop it seems so WebKit is no different. See for example the implementation of ecore_main_loop_thread_safe_call_async(): it is using ecore pipe if not in the main thread. 776395 11 cdumez 2012-11-26 23:09:02 -0800 (In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > According to raster, ecore_pipe_write is intended for a thread. So, he recommends to make an ecore_pipe per thread. I also think we need to use a lock for current usage. But, I'm not clear to use ecore_pipe_write here yet. If this is urgent issue, I don't mind to land this code at the moment. If not, I would like to dig this a little further. I wanna check why ecore_pipe_write is used here. > > > > Yes, it is the fundamental question that 'Why EFL WebKit uses the ecore pipe to wake up main loop?' :) > > > > Maybe there are many people who have the same question, and so do I. > > > > Is there anyone who knows the history about this? > > (Maybe it is related with some priority between the loop event items, but I'm not sure) > > Well, EFL also uses ecore pipe to wake up the main loop it seems so WebKit is no different. See for example the implementation of ecore_main_loop_thread_safe_call_async(): it is using ecore pipe if not in the main thread. Also note that _ecore_main_loop_thread_safe_call() is protecting pipe write with a mutex as well. I think Byungwoo's patch is good. 776414 12 172193 gyuyoung.kim 2012-11-26 23:42:17 -0800 Comment on attachment 172193 Patch This patch is fine for current implementation. 777583 13 cdumez 2012-11-27 23:54:55 -0800 Byungwoo, are you landing this? :) 777616 14 172193 bw80.lee 2012-11-28 01:12:06 -0800 Comment on attachment 172193 Patch Yes please :) I tested and there was no regression on layout test and API test. 777620 15 172193 webkit.review.bot 2012-11-28 01:18:51 -0800 Comment on attachment 172193 Patch Rejecting attachment 172193 from commit-queue. christophe.dumez@intel.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. 777622 16 cdumez 2012-11-28 01:21:38 -0800 I'm in committers.py but apparently the commit queue has not restarted yet :( 777644 17 172193 webkit.review.bot 2012-11-28 01:53:27 -0800 Comment on attachment 172193 Patch Rejecting attachment 172193 from commit-queue. christophe.dumez@intel.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. 777673 18 172193 webkit.review.bot 2012-11-28 02:51:35 -0800 Comment on attachment 172193 Patch Rejecting attachment 172193 from commit-queue. christophe.dumez@intel.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. 777831 19 172193 webkit.review.bot 2012-11-28 06:02:12 -0800 Comment on attachment 172193 Patch Rejecting attachment 172193 from commit-queue. christophe.dumez@intel.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. 777865 20 172193 webkit.review.bot 2012-11-28 06:39:21 -0800 Comment on attachment 172193 Patch Clearing flags on attachment: 172193 Committed r136006: <http://trac.webkit.org/changeset/136006> 777866 21 webkit.review.bot 2012-11-28 06:39:27 -0800 All reviewed patches have been landed. Closing bug. 172193 2012-11-02 19:16:35 -0700 2012-11-28 06:39:21 -0800 Patch bug-101132-20121103111452.patch text/plain 2033 bw80.lee U3VidmVyc2lvbiBSZXZpc2lvbjogMTMzMzM0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzQ4YTk3MWI2ZWY5Nzhk ODM4MzJjNjkxNWY4N2QzN2ZjN2M0M2UzMC4uNDlmZTg0M2VlNmNmMmVkMDk2YmNlZTg3Y2MxZWMz MWNkNGM0OTQxZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDEyLTExLTAyICBCeXVu Z3dvbyBMZWUgIDxidzgwLmxlZUBzYW1zdW5nLmNvbT4KKworICAgICAgICBbRUZMXSBVc2UgbXV0 ZXggbG9ja2VyIGluIHdha2VVcCgpIHRvIGVuc3VyZSB0aHJlYWQtc2FmZXR5LgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAxMTMyCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIG11dGV4IGxvY2tlciBm b3IgdGhlIGVjb3JlIHBpcGUgdG8gZW5zdXJlIHRocmVhZC1zYWZldHkgb2YKKyAgICAgICAgUnVu TG9vcDo6d2FrZVVwKCkuCisKKyAgICAgICAgUnVuTG9vcDo6d2FrZVVwKCkgY2FuIGJlIGNhbGxl ZCBieSBtdWx0aXBsZSB0aHJlYWQuIEl0IHVzZXMKKyAgICAgICAgZWNvcmVfcGlwZV93cml0ZSgp IGZ1bmN0aW9uIGJ1dCB0aGUgZnVuY3Rpb24gaXMgbm90IHRocmVhZC1zYWZlLgorCisgICAgICAg ICogcGxhdGZvcm0vUnVuTG9vcC5oOgorICAgICAgICAoUnVuTG9vcCk6CisgICAgICAgICogcGxh dGZvcm0vZWZsL1J1bkxvb3BFZmwuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UnVuTG9vcDo6d2Fr ZVVwKToKKwogMjAxMi0xMS0wMiAgTWljaGFlbCBTYWJvZmYgIDxtc2Fib2ZmQGFwcGxlLmNvbT4K IAogICAgICAgICB2aXNpdGVkSGFzaExpbmsoKSBjb252ZXJ0cyA4IGJpdCBVUkxzIGFuZCBhdHRy aWJ1dGVzIHRvIDE2IGJpdHMuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9S dW5Mb29wLmggYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9SdW5Mb29wLmgKaW5kZXggY2U5ODQ1 ZTJhNTc1OTIxYTlmMTVmZTU1ZjliMWNiZWU4YWY0MWE4Ni4uZmJmYjgzN2YxM2I5N2RkNGNkODc0 MDllOWQ4YjdiOTVmYWI1OGJlNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v UnVuTG9vcC5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1J1bkxvb3AuaApAQCAtMTY3 LDcgKzE2NywxMCBAQCBwcml2YXRlOgogICAgIFZlY3RvcjxHUmVmUHRyPEdNYWluTG9vcD4gPiBt X3J1bkxvb3BNYWluTG9vcHM7CiAjZWxpZiBQTEFURk9STShFRkwpCiAgICAgYm9vbCBtX2luaXRF Zmw7CisKKyAgICBNdXRleCBtX3BpcGVMb2NrOwogICAgIE93blB0cjxFY29yZV9QaXBlPiBtX3Bp cGU7CisKICAgICBzdGF0aWMgdm9pZCB3YWtlVXBFdmVudCh2b2lkKiBkYXRhLCB2b2lkKiwgdW5z aWduZWQgaW50KTsKICNlbmRpZgogfTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL2VmbC9SdW5Mb29wRWZsLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2VmbC9SdW5M b29wRWZsLmNwcAppbmRleCBmMjk5ZDhlY2YyNjgyNDBiZGQyNjY2YmE1MmE0NGJjY2JhMTllMjJk Li44ODY4NWUwZTkyMzBmNDA5ZjRlZDZmNGJkNDczNzdhMjU5YjIzYzcwIDEwMDY0NAotLS0gYS9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9lZmwvUnVuTG9vcEVmbC5jcHAKKysrIGIvU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vZWZsL1J1bkxvb3BFZmwuY3BwCkBAIC0xMDIsNiArMTAyLDcgQEAgdm9p ZCBSdW5Mb29wOjp3YWtlVXBFdmVudCh2b2lkKiBkYXRhLCB2b2lkKiwgdW5zaWduZWQgaW50KQog CiB2b2lkIFJ1bkxvb3A6Ondha2VVcCgpCiB7CisgICAgTXV0ZXhMb2NrZXIgbG9ja2VyKG1fcGlw ZUxvY2spOwogICAgIGVjb3JlX3BpcGVfd3JpdGUobV9waXBlLmdldCgpLCB3YWt1cEVjb3JlUGlw ZU1lc3NhZ2UsIGVjb3JlUGlwZU1lc3NhZ2VTaXplKTsKIH0KIAo=