Bug 101135 – [EFL] Remove redundant pipe write to prevent pipe buffer full.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Byungwoo Lee]

EFL uses ecore_pipe_write() to wake up main run loop,
and the function uses pipe write with O_NONBLOCK disabled.

With O_NONBLOCK disabled, when written data is more than PIPE_BUF,
pipe write will block until it can be written. (according to the pipe() man page)

Currently, wakeUp() always write pipe to wake up.
But if wake up is already requested and not did, additional pipe write is not needed.

This redundant pipe write makes pipe buffer full status.

[Byungwoo Lee]

To make this patch, Bug 101132 is needed.

[Byungwoo Lee]

Created attachment 172213 [details]
Patch

Waiting for Bug 101132

[Byungwoo Lee]

Created attachment 172729 [details]
Patch

[Byungwoo Lee]

(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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 174823 [details]
Patch to reproduce deadlock

[Byungwoo Lee]

(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())

[Chris Dumez]

(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.

[Chris Dumez]

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.

[Byungwoo Lee]

(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 :)

[Byungwoo Lee]

Created attachment 178817 [details]
Patch

[Chris Dumez]

Comment on attachment 178817 [details]
Patch

LGTM.

[Gyuyoung Kim]

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 ?

[Byungwoo Lee]

Created attachment 179235 [details]
Patch

[Byungwoo Lee]

Comment on attachment 179235 [details]
Patch

Fixed typo.
I tested unit test and layout test with this, and there was no regression.

[WebKit Review Bot]

Comment on attachment 179235 [details]
Patch

Clearing flags on attachment: 179235

Committed r137580: <http://trac.webkit.org/changeset/137580>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.