Bug 126979 - [GTK] Web process sometimes crashes when printing in synchronous mode
Summary: [GTK] Web process sometimes crashes when printing in synchronous mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 126977
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-14 05:19 PST by Carlos Garcia Campos
Modified: 2014-01-15 09:33 PST (History)
8 users (show)

See Also:


Attachments
Patch (17.68 KB, patch)
2014-01-15 02:53 PST, Carlos Garcia Campos
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-01-14 05:19:34 PST
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.
Comment 1 Carlos Garcia Campos 2014-01-15 02:53:16 PST
Created attachment 221254 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2014-01-15 04:26:25 PST
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?
Comment 3 Carlos Garcia Campos 2014-01-15 04:56:31 PST
(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).
Comment 4 Carlos Garcia Campos 2014-01-15 05:00:41 PST
Adding wk2 owner because of the change in WebChromeClient
Comment 5 Gustavo Noronha (kov) 2014-01-15 06:05:50 PST
(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!
Comment 6 Carlos Garcia Campos 2014-01-15 09:33:35 PST
Committed r162073: <http://trac.webkit.org/changeset/162073>