Bug 196839 - UI↔Web deadlock when printing with a JavaScript alert visible
Summary: UI↔Web deadlock when printing with a JavaScript alert visible
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-11 17:11 PDT by Tim Horton
Modified: 2019-05-09 12:17 PDT (History)
9 users (show)

See Also:


Attachments
Patch (14.37 KB, patch)
2019-04-11 17:12 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2019-04-11 17:11:42 PDT
UI↔Web deadlock when printing with a JavaScript alert visible
Comment 1 Tim Horton 2019-04-11 17:12:38 PDT
Created attachment 367265 [details]
Patch
Comment 2 Tim Horton 2019-04-11 17:12:40 PDT
<rdar://problem/49157642>
Comment 3 Andy Estes 2019-04-11 17:44:12 PDT
Comment on attachment 367265 [details]
Patch

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

This is all SPI and the only caller of _webViewPrintFormatter doesn't cache the return value, so I think this is ok as-is, but I do think the design of -_webViewPrintFormatter is weird.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6318
> +    if (_page->process().connection()->hasOutstandingOutgoingSynchronousReplies())
> +        return nil;

What if a client happens to call this while there are outstanding synchronous replies but doesn't plan to start printing until later?

What about always returning a _WKWebViewPrintFormatter but giving it a new boolean property that clients can check?
Comment 4 Alexey Proskuryakov 2019-04-11 20:18:39 PDT
Comment on attachment 367265 [details]
Patch

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

> Source/WebKit/ChangeLog:23
> +        synchronous message (like, say, an alert()) results in an app-destroying deadlock.

Is this a regression from switching to delayed reply messages? It used to be that sync messages were handled while waiting for responses.
Comment 5 Tim Horton 2019-04-11 20:34:35 PDT
(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 367265 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367265&action=review
> 
> > Source/WebKit/ChangeLog:23
> > +        synchronous message (like, say, an alert()) results in an app-destroying deadlock.
> 
> Is this a regression from switching to delayed reply messages? It used to be
> that sync messages were handled while waiting for responses.

More likely from http://trac.webkit.org/changeset/236546/webkit, which has had an endless tail of similar regressions.
Comment 6 Alexey Proskuryakov 2019-04-12 12:04:18 PDT
Indeed.
Comment 7 WebKit Commit Bot 2019-04-17 13:20:46 PDT
Comment on attachment 367265 [details]
Patch

Clearing flags on attachment: 367265

Committed r244400: <https://trac.webkit.org/changeset/244400>
Comment 8 WebKit Commit Bot 2019-04-17 13:20:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Truitt Savell 2019-04-17 16:06:23 PDT
Reverted r244400 for reason:

Caused testing to exit early with assertionon Debug WK2

Committed r244407: <https://trac.webkit.org/changeset/244407>
Comment 10 Tim Horton 2019-04-27 14:03:36 PDT
dispatchSyncMessage 1
sendSyncReply 1
dispatchSyncMessage 1
sendSyncReply 1
sendSyncReply 0
sendSyncReply -1
sendSyncReply -2
sendSyncReply -3
sendSyncReply -4

welp
Comment 11 Tim Horton 2019-04-27 23:12:53 PDT
(In reply to Tim Horton from comment #10)
> dispatchSyncMessage 1
> sendSyncReply 1
> dispatchSyncMessage 1
> sendSyncReply 1
> sendSyncReply 0
> sendSyncReply -1
> sendSyncReply -2
> sendSyncReply -3
> sendSyncReply -4
> 
> welp

1   0x10fcde076 IPC::Connection::sendSyncReply(std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >)
2   0x10fdf649d Messages::NetworkProcess::ResetParametersToDefaultValues::send(std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >&&, IPC::Connection&)
3   0x10fe452ea auto void IPC::handleMessageAsync<Messages::NetworkProcess::ResetParametersToDefaultValues, WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(PAL::SessionID, WTF::CompletionHandler<void ()>&&)>(IPC::Connection&, IPC::Decoder&, WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(PAL::SessionID, WTF::CompletionHandler<void ()>&&))::'lambda'(auto&&...)::operator()<>(auto&&...)

I guess async messages use sendSyncReply too? Confused.
Comment 12 Tim Horton 2019-04-27 23:24:14 PDT
(In reply to Tim Horton from comment #11)
> (In reply to Tim Horton from comment #10)
> > dispatchSyncMessage 1
> > sendSyncReply 1
> > dispatchSyncMessage 1
> > sendSyncReply 1
> > sendSyncReply 0
> > sendSyncReply -1
> > sendSyncReply -2
> > sendSyncReply -3
> > sendSyncReply -4
> > 
> > welp
> 
> 1   0x10fcde076
> IPC::Connection::sendSyncReply(std::__1::unique_ptr<IPC::Encoder,
> std::__1::default_delete<IPC::Encoder> >)
> 2   0x10fdf649d
> Messages::NetworkProcess::ResetParametersToDefaultValues::send(std::__1::
> unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >&&,
> IPC::Connection&)
> 3   0x10fe452ea auto void
> IPC::handleMessageAsync<Messages::NetworkProcess::
> ResetParametersToDefaultValues, WebKit::NetworkProcess, void
> (WebKit::NetworkProcess::*)(PAL::SessionID, WTF::CompletionHandler<void
> ()>&&)>(IPC::Connection&, IPC::Decoder&, WebKit::NetworkProcess*, void
> (WebKit::NetworkProcess::*)(PAL::SessionID, WTF::CompletionHandler<void
> ()>&&))::'lambda'(auto&&...)::operator()<>(auto&&...)
> 
> I guess async messages use sendSyncReply too? Confused.

Also this is all happening on a variety of threads, too, so it wasn't safe anyway