Summary: | [GTK] Crash when printing via javascript in WebKit2 | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | clopez, commit-queue, gustavo, ltilve, mrobinson | ||||||
Priority: | P2 | Keywords: | Gtk | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2013-11-08 06:49:25 PST
I have a patch for this one too, I'll submit it later today Created attachment 216401 [details]
Patch
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 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); } (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. > } > 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); } (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. Created attachment 216519 [details]
Updated patch following Martin's suggestions
Committed r159042: <http://trac.webkit.org/changeset/159042> |