Bug 221488 - WebGL IPC messages are delivered out of order
Summary: WebGL IPC messages are delivered out of order
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on: 221396
Blocks: webglgpup 219641
  Show dependency treegraph
 
Reported: 2021-02-05 10:30 PST by Kimmo Kinnunen
Modified: 2021-02-10 14:40 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.65 KB, patch)
2021-02-05 10:37 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
new approach (3.85 KB, patch)
2021-02-10 12:13 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2021-02-05 10:30:47 PST
WebGL IPC messages are delivered out of order

Sync messages are delivered before earlier async messages, if some other WebKit part waits on sync replies.
Comment 1 Kimmo Kinnunen 2021-02-05 10:37:34 PST
Created attachment 419430 [details]
Patch
Comment 2 Kenneth Russell 2021-02-05 16:02:15 PST
Comment on attachment 419430 [details]
Patch

Looks good to me. (The EWS failures are all failures to apply the patch.) Chromium had this exact bug in its IPC subsystem some time ago and I recall working on the fix. r+
Comment 3 Alexey Proskuryakov 2021-02-08 11:03:37 PST
I'm somewhat skeptical that this won't introduce deadlocks in more cases than you are warning about in the comment. I won't have an opportunity to think it through, so feel free to ignore my comment, but it may be good to give Chris some time to comment.
Comment 4 Chris Dumez 2021-02-08 11:11:10 PST
Comment on attachment 419430 [details]
Patch

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

> Source/WebKit/Platform/IPC/Connection.h:77
> +    // Note: causes timeouts if all parties in the synchronous message chain send synchronous messages with this flag

This does not look right to me. We forbid re-entrency from IPC in the WebProcess due to security bugs. Therefore, the other process must always break the deadlock by dispatching when waiting for a sync message.
Comment 5 Chris Dumez 2021-02-08 12:20:08 PST
(In reply to Chris Dumez from comment #4)
> Comment on attachment 419430 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419430&action=review
> 
> > Source/WebKit/Platform/IPC/Connection.h:77
> > +    // Note: causes timeouts if all parties in the synchronous message chain send synchronous messages with this flag
> 
> This does not look right to me. We forbid re-entrency from IPC in the
> WebProcess due to security bugs. Therefore, the other process must always
> break the deadlock by dispatching when waiting for a sync message.

This is the logic that prevents WebProcess re-entrency on IPC:
    // Use this flag to force synchronous messages to be treated as asynchronous messages in the WebProcess.
    // Otherwise, the WebProcess would process incoming synchronous IPC while waiting for a synchronous IPC
    // reply from the Network process, which would be unsafe.
    m_connection->setOnlySendMessagesAsDispatchWhenWaitingForSyncReplyWhenProcessingSuchAMessage(true);

We are currently missing this logic on the GPUProcess connection but this is a bug that I will fix.
Comment 6 Kimmo Kinnunen 2021-02-10 12:13:54 PST
Created attachment 419886 [details]
new approach
Comment 7 EWS 2021-02-10 13:04:20 PST
Committed r272680: <https://commits.webkit.org/r272680>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419886 [details].
Comment 8 Radar WebKit Bug Importer 2021-02-10 14:40:42 PST
<rdar://problem/74206986>