Bug 186455

Summary: [Win] Remove RunLoopWin and WorkQueueWin
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: Web Template FrameworkAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED INVALID    
Severity: Normal CC: achristensen, andersca, Basuke.Suzuki, bfulgham, don.olmstead, ews-watchlist, Hironori.Fujii, mcatanzaro, youennf
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=210785
Attachments:
Description Flags
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews200 for win-future
none
Archive of layout-test-results from ews117 for mac-sierra none

Description Yusuke Suzuki 2018-06-08 23:00:47 PDT
Why do we have platform-dependent RunLoop? The answer is that we sometimes want to integrate our RunLoop into the existing platform RunLoop, which is driven by the system or main function of the application.
For example, RunLoopCF uses CFRunLoopGetCurrent() to drive the *current* RunLoop, which is driven by the application too.
And RunLoopGLib is the same: It retrieves the *default* RunLoop for the current application, which is shared by the glib application too.

On the other hand, Windows RunLoop implementation is different. It creates a special window handle per RunLoop by CreateWindow. And we use the message loop of this window handle.
But this window handle is not shared by the application. This window handle is only used by RunLoop. We do not have the way to post a message to this window handle without interacting to this RunLoopWin.

So, do we really need to have RunLoopWin implementation? Why not use RunLoopGeneric in Windows instead?

This patch removes RunLoopWin, and WorkQueueWin. Windows start using RunLoopGeneric and WorkQueueGeneric.
Comment 1 Basuke Suzuki 2018-06-11 10:33:02 PDT
For WinCairo, we don't have any reason to keep using the current Windows implementation. We still need to test with generic implementation before moving forward, but we agreed with the direction.

We need to hear the voice of AppleWin guys.
Comment 2 Basuke Suzuki 2018-06-11 11:12:22 PDT
> This patch removes RunLoopWin, and WorkQueueWin. Windows start using RunLoopGeneric and WorkQueueGeneric.

Can you attach you patch?
Comment 3 Basuke Suzuki 2018-06-11 11:50:30 PDT
One issue we found is that very Windows specific feature was added to WorkQueueWin, which are:
        (WTF::WorkQueue::handleCallback):
        (WTF::WorkQueue::registerHandle):
        (WTF::WorkQueue::unregisterAndCloseHandle):
and they are used in IPC/ConnectionWin.cpp.

I don't see the reason why these features are in WorkQueue because they are not in other port. We can separate those features and move them into IPC/Connection.
Comment 4 Alex Christensen 2018-06-11 11:59:13 PDT
I'd have to see the patch in order to comment.  In general, if nothing breaks we can do it.
Comment 5 Yusuke Suzuki 2018-06-12 08:12:12 PDT
Created attachment 342536 [details]
Patch
Comment 6 Yusuke Suzuki 2018-06-12 08:13:22 PDT
(In reply to Basuke Suzuki from comment #3)
> One issue we found is that very Windows specific feature was added to
> WorkQueueWin, which are:
>         (WTF::WorkQueue::handleCallback):
>         (WTF::WorkQueue::registerHandle):
>         (WTF::WorkQueue::unregisterAndCloseHandle):
> and they are used in IPC/ConnectionWin.cpp.
> 
> I don't see the reason why these features are in WorkQueue because they are
> not in other port. We can separate those features and move them into
> IPC/Connection.

Yes. WorkItemContext is not related to WorkQueue. Since it is only used in IPC::Connection, we can just move this there.
Comment 7 Yusuke Suzuki 2018-06-12 08:18:43 PDT
Comment on attachment 342536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342536&action=review

> Source/WTF/wtf/win/WorkQueueWin.cpp:-231
> -    ::QueueUserWorkItem(unregisterWaitAndDestroyItemCallback, workItem.ptr(), WT_EXECUTEDEFAULT);

I'm not sure who keeps this WorkItemContext's liveness here. I guess this was a bug.
Comment 8 Anders Carlsson 2018-06-12 08:41:49 PDT
Comment on attachment 342536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342536&action=review

> Source/WebKit/Platform/IPC/win/ConnectionWin.cpp:284
> +    result = ::RegisterWaitForSingleObject(
> +        &m_readerWaitHandle,
> +        m_readState.hEvent,
> +        [] ((void* data, BOOLEAN timerOrWaitFired) {
> +            ASSERT_UNUSED(timerOrWaitFired, !timerOrWaitFired);
> +            // This is safe since we call ::UnregisterWaitEx directly when closing.
> +            RefPtr<Connection> protectedThis(static_cast<Connection*>(data));
> +            protectedThis->m_connectionQueue->dispatch([protectedThis = WTFMove(protectedThis)] {
> +                protectedThis->readEventHandler();
> +            });
> +            }, this, INFINITE, WT_EXECUTEDEFAULT));
> +    ASSERT(result);
> +
> +    result = ::RegisterWaitForSingleObject(
> +        &m_writerWaitHandle,
> +        m_writeState.hEvent,
> +        [] ((void* data, BOOLEAN timerOrWaitFired) {
> +            ASSERT_UNUSED(timerOrWaitFired, !timerOrWaitFired);
> +            // This is safe since we call ::UnregisterWaitEx directly when closing.
> +            RefPtr<Connection> protectedThis(static_cast<Connection*>(data));
> +            protectedThis->m_connectionQueue->dispatch([protectedThis = WTFMove(protectedThis)] {
> +                protectedThis->writeEventHandler();
> +            });
> +            }, this, INFINITE, WT_EXECUTEDEFAULT));
> +    ASSERT(result);

This doesn't match the WebKit coding style guidelines.
Comment 9 Yusuke Suzuki 2018-06-12 09:17:46 PDT
Comment on attachment 342536 [details]
Patch

Ah, no. Unfortunately, we cannot do this. We have bunch of GetMessage / PostMessage / DispatchMessage / TranslateMessage calls in our tree, and at that time, these messages are implicitly transferred to the pump in the current thread: If RunLoop is working, it receives these message. So I think removing this is a bit difficult.
Comment 10 EWS Watchlist 2018-06-12 11:35:43 PDT
Comment on attachment 342536 [details]
Patch

Attachment 342536 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8148872

Number of test failures exceeded the failure limit.
Comment 11 EWS Watchlist 2018-06-12 11:35:54 PDT
Created attachment 342569 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 12 EWS Watchlist 2018-06-12 13:16:52 PDT
Comment on attachment 342536 [details]
Patch

Attachment 342536 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/8150164

New failing tests:
js/dom/JSON-stringify.html
Comment 13 EWS Watchlist 2018-06-12 13:16:54 PDT
Created attachment 342588 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 14 Basuke Suzuki 2018-06-12 14:39:20 PDT
(In reply to Yusuke Suzuki from comment #9)
> Comment on attachment 342536 [details]
> Patch
> 
> Ah, no. Unfortunately, we cannot do this. We have bunch of GetMessage /
> PostMessage / DispatchMessage / TranslateMessage calls in our tree, and at
> that time, these messages are implicitly transferred to the pump in the
> current thread: If RunLoop is working, it receives these message. So I think
> removing this is a bit difficult.

That's so bad. But at least we can move Windows only features of WorkQueue into IPC::Connection and make a bug fix with confident. I'll open a new bug for that.