Bug 101135

To make this patch, Bug 101132 is needed.

Created attachment 172213 [details] Patch Waiting for Bug 101132

Created attachment 172729 [details] Patch

(In reply to comment #3) > Created an attachment (id=172729) [details] > Patch Made separate patch that can be applied without Bug 101132 and Bug 98580. I tested layout test and API test. There was no regression with this patch. This will solve lockup problem described in Bug 99494 which is resolved as duplicated with bug 98580.

Byungwoo, I'm hitting a deadlock when implementing modal dialog support in EFL WK2. It seems related to what you're fixing. However, I have tried applying both your patches and the deadlock is still here. Could you please look into it? I'm going to upload a patch to help reproduce the deadlock. Here is the procedure: 1. Apply the deadlock patch I'm uploading (it already contains both your fixes) 2. Build WebKit EFL with "--no-tiled-backing-store" (to make sure the issue is not related to accelerated compositing) 3. Open the following page with MiniBrowser: http://samples.msdn.microsoft.com/workshop/samples/author/dhtml/refs/showModalDialogLaunch.htm 4. Click on the "Click to show modal dialog" button This will open a new window and hit the deadlock. Input in that window (and the parent one) is ignored, and resizing the window will make it black. If you look at WebPage::runModal() (in WebProcess/WebPage), it send the runModal message to the UIProcess then call RunLoop::run(). When RunLoop::run() is called, we're stuck. I'm not sure it is related to your problem but it seemed related so I thought I would mention it here.

Created attachment 174823 [details] Patch to reproduce deadlock

(In reply to comment #5) > Byungwoo, I'm hitting a deadlock when implementing modal dialog support in EFL WK2. It seems related to what you're fixing. However, I have tried applying both your patches and the deadlock is still here. Could you please look into it? > > I'm going to upload a patch to help reproduce the deadlock. Here is the procedure: > 1. Apply the deadlock patch I'm uploading (it already contains both your fixes) > 2. Build WebKit EFL with "--no-tiled-backing-store" (to make sure the issue is not related to accelerated compositing) > 3. Open the following page with MiniBrowser: http://samples.msdn.microsoft.com/workshop/samples/author/dhtml/refs/showModalDialogLaunch.htm > 4. Click on the "Click to show modal dialog" button > > This will open a new window and hit the deadlock. Input in that window (and the parent one) is ignored, and resizing the window will make it black. > > If you look at WebPage::runModal() (in WebProcess/WebPage), it send the runModal message to the UIProcess then call RunLoop::run(). When RunLoop::run() is called, we're stuck. > > I'm not sure it is related to your problem but it seemed related so I thought I would mention it here. I checked the reproduce step and I could meet the lockup. But the internal status is different with the problem that can be solved with this patch. I found below with the reproduce step. 1. WebProcess IPC Thread received IPC messages from UIProcess. 2. WebProcess IPC Thread pushed work item to the RunLoop work queue and called RunLoop::wakeUp() function. 3. RunLoop::wakeUp() function is successfully finished. 4. The pipe callback (RunLoop::wakeUpEvent()) is not called. I think that the lockup that you shared is a different issue that this patch focuses. (I'm not sure but it seems related with the nested ecore main loop, ) The lockup problem of this patch is that, both RunLoop::wakeUp() function and RunLoop::wakeUpEvent() are not finished because of some deadlock between the pipe buffer full status and work queue mutex lock. But The lockup problem of modal is that, main loop is not wakeup after the RunLoop::wakeUp() is called successfully. (more clearly, ecore pipe callback function is not called after the ecore_pipe_write())

(In reply to comment #7) > (In reply to comment #5) > > Byungwoo, I'm hitting a deadlock when implementing modal dialog support in EFL WK2. It seems related to what you're fixing. However, I have tried applying both your patches and the deadlock is still here. Could you please look into it? > > > > I'm going to upload a patch to help reproduce the deadlock. Here is the procedure: > > 1. Apply the deadlock patch I'm uploading (it already contains both your fixes) > > 2. Build WebKit EFL with "--no-tiled-backing-store" (to make sure the issue is not related to accelerated compositing) > > 3. Open the following page with MiniBrowser: http://samples.msdn.microsoft.com/workshop/samples/author/dhtml/refs/showModalDialogLaunch.htm > > 4. Click on the "Click to show modal dialog" button > > > > This will open a new window and hit the deadlock. Input in that window (and the parent one) is ignored, and resizing the window will make it black. > > > > If you look at WebPage::runModal() (in WebProcess/WebPage), it send the runModal message to the UIProcess then call RunLoop::run(). When RunLoop::run() is called, we're stuck. > > > > I'm not sure it is related to your problem but it seemed related so I thought I would mention it here. > > > I checked the reproduce step and I could meet the lockup. > > But the internal status is different with the problem that can be solved with this patch. > > I found below with the reproduce step. > 1. WebProcess IPC Thread received IPC messages from UIProcess. > 2. WebProcess IPC Thread pushed work item to the RunLoop work queue and called RunLoop::wakeUp() function. > 3. RunLoop::wakeUp() function is successfully finished. > 4. The pipe callback (RunLoop::wakeUpEvent()) is not called. > > I think that the lockup that you shared is a different issue that this patch focuses. > (I'm not sure but it seems related with the nested ecore main loop, ) > > The lockup problem of this patch is that, both RunLoop::wakeUp() function and RunLoop::wakeUpEvent() are not finished because of some deadlock between the pipe buffer full status and work queue mutex lock. > > But The lockup problem of modal is that, main loop is not wakeup after the RunLoop::wakeUp() is called successfully. (more clearly, ecore pipe callback function is not called after the ecore_pipe_write()) Ok, thanks a lot for looking into it Byungwoo! This is very useful.

Comment on attachment 172729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172729&action=review > Source/WebCore/platform/efl/RunLoopEfl.cpp:101 > + static_cast<RunLoop*>(data)->m_wakeUpEventRequested = false; Isn't there a thread safety problem here? wakeUp() can be called from any thread and it updates m_wakeUpEventRequested flag, wakeUpEvent() is executed in the main thread and updates the same flag. Looks to me like m_wakeUpEventRequested should be mutex protected. Please correct me if I'm wrong.

(In reply to comment #9) > (From update of attachment 172729 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172729&action=review > > > Source/WebCore/platform/efl/RunLoopEfl.cpp:101 > > + static_cast<RunLoop*>(data)->m_wakeUpEventRequested = false; > > Isn't there a thread safety problem here? wakeUp() can be called from any thread and it updates m_wakeUpEventRequested flag, wakeUpEvent() is executed in the main thread and updates the same flag. Looks to me like m_wakeUpEventRequested should be mutex protected. Please correct me if I'm wrong. Yes! For the standard procedure, adding mutex lock for m_wakeUpEventRequested will be better to ensure thread-safety. (mutex name might be m_wakeUpEventRequestedLock) Actually, thread-safety of the wakeUp() is handled in Bug 101132. So if this patch is landed first, I'll rebase the Bug 101132 accordingly. Ideally, this patch should protect all the redundant pipe write if there is a former pipe write that is not processed. But as you mentioned, multiple thread can access the flag at one time and this can make some unintended status. But fortunately, clearing the flag is happened only on thread (main run loop thread). So at worst, the maximum number of redundant pipe write will be a number of threads that calls wakeUp() function. Without the locking for the m_wakeUpEventRequested, this patch will work at most cases. but for the perfection, adding locker for the flag is needed. Thanks for pointing this :)

Created attachment 178817 [details] Patch

Comment on attachment 178817 [details] Patch LGTM.

Comment on attachment 178817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178817&action=review This patch is same issue with Bug 101132. We need to use a lock for ecore_pipe_write for now because ecore_pipe_write is intended for a thread. Set r+ based on informal review thread. > Source/WebCore/ChangeLog:12 > + uses PISIX pipe write with O_NONBLOCK disabled. PISIX -> POSIX ?

Created attachment 179235 [details] Patch

Comment on attachment 179235 [details] Patch Fixed typo. I tested unit test and layout test with this, and there was no regression.

Comment on attachment 179235 [details] Patch Clearing flags on attachment: 179235 Committed r137580: <http://trac.webkit.org/changeset/137580>

All reviewed patches have been landed. Closing bug.