Bug 85066 – [GTK] TestWebKitAPI/WebKit2/TestMouseMoveAfterCrash is flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

(TestMouseMoveAfterCrash:4200): GLib-GIO-ERROR **: creating GSocket from fd 13: Bad file descriptor

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WebKit2
[ RUN      ] WebKit2.MouseMoveAfterCrash
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WebKit2
[ RUN      ] WebKit2.ParentFrame
[       OK ] WebKit2.ParentFrame (194 ms)
[----------] 1 test from WebKit2 (194 ms total)

[----------] Global test environment tear-down

[Philippe Normand]

I'll skip it for now. Flakiness happens on the 64-bit Release bot at least.

[Lauro Moura Maranhao Neto]

In Nix we have a test[1] related to WebProcess instant crash/respawn and we were also having this kind of "Bad file descriptor" crashes. After looking into it, there were some fixes for WorkQueueGtk.cpp and ProcessLauncherGtk.cpp which made the crashes disappear, although the test keeps failing.

I don't know if this is a correct way of debugging but copying the EXPECT_JS_TRUE line to right after the first set of mouse moves (before the process termination) and removing the web process pause shows that the mouse moves aren't reported.


[1] https://github.com/WebKitNix/webkitnix/blob/master/Tools/TestWebKitAPI/Tests/nix/WebViewWebProcessCrashed.cpp

[Lauro Moura Maranhao Neto]

Created attachment 196632 [details]
Patch for WorkQueue and ProcessLauncher

Fixes for "bad file descriptor/socket operation on non-socket descriptor" crashes in WorkQueueGtk.cpp and ProcessLauncher.cpp. Maybe they should go on a separate bug.

From the commit message:

    - Avoid calling connectionDidClose() twice.
    
    From g_source_create_source:
    
    "It is meaningless to specify G_IO_ERR or G_IO_HUP in condition; these
    conditions will always be reported output if they are true."
    
    As readyReadHandler() will be called anyway for G_IO_HUP e G_IO_ERR, let
    it in charge of calling connectionDidClose instead of adding another
    event source, just like EFL and Qt does.
    
    - Check the status of m_cancellable().
    
    EventSource::cancel() cancels m_cancellable, which triggers this
    source with the current condition. A check for the cancellable status
    is needed to check whether it was cancelled or not.
    
    - Do not close the child socket when it's terminated.
    
    This action is delegated to Connection::platformInvalidate.
    Closing it here can raise race conditions when polling the sockets
    from the WorkQueue mainloop.

[Carlos Garcia Campos]

(In reply to comment #3)
> Created an attachment (id=196632) [details]
> Patch for WorkQueue and ProcessLauncher
> 
> Fixes for "bad file descriptor/socket operation on non-socket descriptor" crashes in WorkQueueGtk.cpp and ProcessLauncher.cpp. Maybe they should go on a separate bug.
> 
> From the commit message:
> 
>     - Avoid calling connectionDidClose() twice.
> 
>     From g_source_create_source:
> 
>     "It is meaningless to specify G_IO_ERR or G_IO_HUP in condition; these
>     conditions will always be reported output if they are true."
>
>     As readyReadHandler() will be called anyway for G_IO_HUP e G_IO_ERR, let
>     it in charge of calling connectionDidClose instead of adding another
>     event source, just like EFL and Qt does.
> 
>     - Check the status of m_cancellable().
> 
>     EventSource::cancel() cancels m_cancellable, which triggers this
>     source with the current condition. A check for the cancellable status
>     is needed to check whether it was cancelled or not.

This is properly fixed in this bug where I reworked the WorQueue implementation:

https://bugs.webkit.org/show_bug.cgi?id=112729

Patch has been reviewed but still waiting for a WebKit2 owner to be approved.

[Michael Catanzaro]

Closing per discussion in https://github.com/WebKit/WebKit/pull/17971