Bug 196839 – UI↔Web deadlock when printing with a JavaScript alert visible

• Status: REOPENED
• Component: New Bugs

[Tim Horton]

UI↔Web deadlock when printing with a JavaScript alert visible

[Tim Horton]

Created attachment 367265 [details]
Patch

[Tim Horton]

<rdar://problem/49157642>

[Andy Estes]

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?

[Alexey Proskuryakov]

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.

[Tim Horton]

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

[Alexey Proskuryakov]

Indeed.

[WebKit Commit Bot]

Comment on attachment 367265 [details]
Patch

Clearing flags on attachment: 367265

Committed r244400: <https://trac.webkit.org/changeset/244400>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

Reverted r244400 for reason:

Caused testing to exit early with assertionon Debug WK2

Committed r244407: <https://trac.webkit.org/changeset/244407>

[Tim Horton]

dispatchSyncMessage 1
sendSyncReply 1
dispatchSyncMessage 1
sendSyncReply 1
sendSyncReply 0
sendSyncReply -1
sendSyncReply -2
sendSyncReply -3
sendSyncReply -4

welp

[Tim Horton]

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

[Tim Horton]

(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