Bug 124043 – [GTK] Crash when printing via javascript in WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It happens because the print operation when started by the WebProcess due to a DOM operation is supposed to be synchronous and our print operation is always asynchronous.

[Carlos Garcia Campos]

I have a patch for this one too, I'll submit it later today

[Carlos Garcia Campos]

Created attachment 216401 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 216401 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.cpp:732
> +    GMainLoop* mainLoop;
> +    unsigned idlePriority;
> +    if (m_printMode == PrintInfo::PrintModeSync) {
> +        ASSERT(data->mainLoop);
> +        mainLoop = data->mainLoop.get();
> +
> +        // Make sure the print pages idle has more priority than IPC messages comming from
> +        // the IO thread, so that the EndPrinting message is always handled once the print
> +        // operation has finished. See https://bugs.webkit.org/show_bug.cgi?id=122801.
> +        idlePriority = G_PRIORITY_DEFAULT - 10;
> +    } else {
> +        mainLoop = 0;
> +        idlePriority = G_PRIORITY_DEFAULT_IDLE + 10;
> +    }
> +    m_printPagesIdleId = gdk_threads_add_idle_full(idlePriority, printPagesIdle, data.leakPtr(), printPagesIdleDone);
> +    if (mainLoop)
> +        g_main_loop_run(mainLoop);

What do you think about simplifying this to something like:

// Make sure the print pages idle has more priority than IPC messages comming from
// the IO thread, so that the EndPrinting message is always handled once the print
// operation has finished. See https://bugs.webkit.org/show_bug.cgi?id=122801.
unsigned idlePriority = m_printMode == PrintInfo::PrintModeSync ? G_PRIORITY_DEFAULT - 10 : G_PRIORITY_DEFAULT_IDLE + 10;
m_printPagesIdleId = gdk_threads_add_idle_full(idlePriority, printPagesIdle, data.leakPtr(), printPagesIdleDone);
if (m_printMode == PrintInfo::PrintModeSync) {
    ASSERT(data->mainLoop);
    g_main_loop_run(data->mainLoop);
}

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 216401 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216401&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.cpp:732
> > +    GMainLoop* mainLoop;
> > +    unsigned idlePriority;
> > +    if (m_printMode == PrintInfo::PrintModeSync) {
> > +        ASSERT(data->mainLoop);
> > +        mainLoop = data->mainLoop.get();
> > +
> > +        // Make sure the print pages idle has more priority than IPC messages comming from
> > +        // the IO thread, so that the EndPrinting message is always handled once the print
> > +        // operation has finished. See https://bugs.webkit.org/show_bug.cgi?id=122801.
> > +        idlePriority = G_PRIORITY_DEFAULT - 10;
> > +    } else {
> > +        mainLoop = 0;
> > +        idlePriority = G_PRIORITY_DEFAULT_IDLE + 10;
> > +    }
> > +    m_printPagesIdleId = gdk_threads_add_idle_full(idlePriority, printPagesIdle, data.leakPtr(), printPagesIdleDone);
> > +    if (mainLoop)
> > +        g_main_loop_run(mainLoop);
> 
> What do you think about simplifying this to something like:
> 
> // Make sure the print pages idle has more priority than IPC messages comming from
> // the IO thread, so that the EndPrinting message is always handled once the print
> // operation has finished. See https://bugs.webkit.org/show_bug.cgi?id=122801.
> unsigned idlePriority = m_printMode == PrintInfo::PrintModeSync ? G_PRIORITY_DEFAULT - 10 : G_PRIORITY_DEFAULT_IDLE + 10;
> m_printPagesIdleId = gdk_threads_add_idle_full(idlePriority, printPagesIdle, data.leakPtr(), printPagesIdleDone);
> if (m_printMode == PrintInfo::PrintModeSync) {
>     ASSERT(data->mainLoop);
>     g_main_loop_run(data->mainLoop);

This is not possible, at this point data contains a NULL pointer, because of the leakPtr(). That's why we first need to save the pointer.

> }

[Martin Robinson]

> This is not possible, at this point data contains a NULL pointer, because of the leakPtr(). That's why we first need to save the pointer.


Good point. How about this: 

// Make sure the print pages idle has more priority than IPC messages comming from
// the IO thread, so that the EndPrinting message is always handled once the print
// operation has finished. See https://bugs.webkit.org/show_bug.cgi?id=122801.
unsigned idlePriority = m_printMode == PrintInfo::PrintModeSync ? G_PRIORITY_DEFAULT - 10 : G_PRIORITY_DEFAULT_IDLE + 10;
GMainLoop* mainLoop = data->mainLoop;
m_printPagesIdleId = gdk_threads_add_idle_full(idlePriority, printPagesIdle, data.leakPtr(), printPagesIdleDone);

if (m_printMode == PrintInfo::PrintModeSync) {
    ASSERT(mainLoop);
    g_main_loop_run(mainLoop);
}

[Carlos Garcia Campos]

(In reply to comment #6)
> > This is not possible, at this point data contains a NULL pointer, because of the leakPtr(). That's why we first need to save the pointer.
> 
> 
> Good point. How about this: 
> 
> // Make sure the print pages idle has more priority than IPC messages comming from
> // the IO thread, so that the EndPrinting message is always handled once the print
> // operation has finished. See https://bugs.webkit.org/show_bug.cgi?id=122801.
> unsigned idlePriority = m_printMode == PrintInfo::PrintModeSync ? G_PRIORITY_DEFAULT - 10 : G_PRIORITY_DEFAULT_IDLE + 10;
> GMainLoop* mainLoop = data->mainLoop;

GMainLoop* mainLoop = data->mainLoop.get(); :-)

> m_printPagesIdleId = gdk_threads_add_idle_full(idlePriority, printPagesIdle, data.leakPtr(), printPagesIdleDone);
> 
> if (m_printMode == PrintInfo::PrintModeSync) {
>     ASSERT(mainLoop);
>     g_main_loop_run(mainLoop);
> }

Ok, looks good to me.

[Carlos Garcia Campos]

Created attachment 216519 [details]
Updated patch following Martin's suggestions

[Carlos Garcia Campos]

Committed r159042: <http://trac.webkit.org/changeset/159042>