[GTK] Web process sometimes crashes when printing in synchronous mode

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The problem is that it's still possible to receive and process the EndPrinting message while the print operation is ongoing. For rendering print pages we are using a nested main loop and idle sources with a higher priority than the idle source used by the I/O thread, so that EndPrinting is never processed until the printing has actually finished. The problem is that sometimes the EndPrinting message is received before starting to render the pages, while we are still enumerating the printers to find the required one. When running in sync mode gtk_enumerate_printers() also uses a nested main loop, so that while waiting for the printer backends to reply, the EndPrinting message can be attended by the run loop, deleting the print context and the print operation. When gtk_enumerate_printers() finishes, the print operation has already been deleted unexpectedly.

[Carlos Garcia Campos]

Created attachment 221254 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 221254 [details]
Patch

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

LGTM, only a few questions to ensure I understood the life-cycle of the printer list correctly.

> Source/WebKit2/ChangeLog:12
> +        UI process like EndPrinting. When the EndPriting message is

Readability would be improved by using longer lines and/or splitting in paragraphs, I think.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:610
> +    RefPtr<PrinterListGtk> printerList = PrinterListGtk::shared();

Do I understand correctly that the printer list is kept alive by this reference, but is destroyed afterwards? I was a bit concerned when I saw PrinterListGtk::s_sharedPrinterList, since that could mean the printer list would only be obtained once, but I think I understand the idea now.

> Source/WebKit2/WebProcess/WebPage/gtk/PrinterListGtk.h:53
> +    Vector<GRefPtr<GtkPrinter>, 4> m_printerList;

Any particular reason for using 4 here?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 221254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221254&action=review
> 
> LGTM, only a few questions to ensure I understood the life-cycle of the printer list correctly.

Thanks!

> > Source/WebKit2/ChangeLog:12
> > +        UI process like EndPrinting. When the EndPriting message is
> 
> Readability would be improved by using longer lines and/or splitting in paragraphs, I think.

my emacs. . . 

> > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:610
> > +    RefPtr<PrinterListGtk> printerList = PrinterListGtk::shared();
> 
> Do I understand correctly that the printer list is kept alive by this reference, but is destroyed afterwards? I was a bit concerned when I saw PrinterListGtk::s_sharedPrinterList, since that could mean the printer list would only be obtained once, but I think I understand the idea now.

Yes, it's tricky. The thing is that for sync printing, we need to make sure that the PrinterList object has already been created, for async printing the PrinterList object is created in WebPrintOperationGtk::startPrint(). The shared method returns a RefPtr, but when created the reference is adopted. So what happens is the following:

Sync printing:

WebChromeClient::print() -> creates the PrinterList
WebPrintOperationGtk::startPrint() -> gets a ref of the existing PrinterList
WebPrintOperationGtk::startPrint() -> releases its ref
WebChromeClient::print() -> PrinterList is destroyed

Async printing:

WebPrintOperationGtk::startPrint() -> creates the PrinterList
WebPrintOperationGtk::startPrint() ->  PrinterList is destroyed

> > Source/WebKit2/WebProcess/WebPage/gtk/PrinterListGtk.h:53
> > +    Vector<GRefPtr<GtkPrinter>, 4> m_printerList;
> 
> Any particular reason for using 4 here?

Nope, I assumed there are usually less than 4 printers (print to file, real printer and possibly a cups-pdf virtual printer).

[Carlos Garcia Campos]

Adding wk2 owner because of the change in WebChromeClient

[Gustavo Noronha (kov)]

(In reply to comment #3)
> > > Source/WebKit2/ChangeLog:12
> > > +        UI process like EndPrinting. When the EndPriting message is
> > 
> > Readability would be improved by using longer lines and/or splitting in paragraphs, I think.
> 
> my emacs. . . 

Mx- set-fill-column <RET> 92 <RET>

\o/ =)
 
> > Do I understand correctly that the printer list is kept alive by this reference, but is destroyed afterwards? I was a bit concerned when I saw PrinterListGtk::s_sharedPrinterList, since that could mean the printer list would only be obtained once, but I think I understand the idea now.
> 
> Yes, it's tricky. The thing is that for sync printing, we need to make sure that the PrinterList object has already 
…

Makes sense, thanks!

[Carlos Garcia Campos]

Committed r162073: <http://trac.webkit.org/changeset/162073>