Bug 186455 – [Win] Remove RunLoopWin and WorkQueueWin

• Component: Web Template Framework
• Status: RESOLVED INVALID

[Yusuke Suzuki]

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.

[Basuke Suzuki]

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.

[Basuke Suzuki]

> This patch removes RunLoopWin, and WorkQueueWin. Windows start using RunLoopGeneric and WorkQueueGeneric.

Can you attach you patch?

[Basuke Suzuki]

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.

[Alex Christensen]

I'd have to see the patch in order to comment.  In general, if nothing breaks we can do it.

[Yusuke Suzuki]

Created attachment 342536 [details]
Patch

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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.

[Anders Carlsson]

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.

[Yusuke Suzuki]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Basuke Suzuki]

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