Created attachment 245625 [details] Test case Load this with debug WK: <body onpageshow="window.print()"> <audio src=""> <script> window.print(); </script> Note: according to the backtrace, this might be a GTK specific issue but I couldn't verify it on another port. It'd be great if someone else could check it. Backtrace: ASSERTION FAILED: !m_adoptionIsRequired ../../Source/WTF/wtf/RefCounted.h(45) : void WTF::RefCountedBase::ref() Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fff8affd700 (LWP 6793)] 0x00007fffed72443d in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321 321 *(int *)(uintptr_t)0xbbadbeef = 0; #0 0x00007fffed72443d in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321 #1 0x00007ffff252729e in WTF::RefCountedBase::ref (this=0x7fff9835f600) at ../../Source/WTF/wtf/RefCounted.h:45 #2 0x00007ffff2996a2f in WTF::refIfNotNull<WebKit::PrinterListGtk> (ptr=0x7fff9835f600) at ../../Source/WTF/wtf/PassRefPtr.h:36 #3 0x00007ffff29967b7 in WTF::RefPtr<WebKit::PrinterListGtk>::RefPtr (this=0x7fffffffc3c0, ptr=0x7fff9835f600) at ../../Source/WTF/wtf/RefPtr.h:43 #4 0x00007ffff299648f in WebKit::PrinterListGtk::shared () at ../../Source/WebKit2/WebProcess/WebPage/gtk/PrinterListGtk.cpp:40 #5 0x00007ffff2838c1a in WebKit::WebChromeClient::print (this=0x6066c0, frame=0x7ffff7f39a00) at ../../Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:664 #6 0x00007ffff34a5d20 in WebCore::Chrome::print (this=0x7ffff7f234b0, frame=0x7ffff7f39a00) at ../../Source/WebCore/page/Chrome.cpp:462 #7 0x00007ffff34cce8c in WebCore::DOMWindow::print (this=0x7ffff7f15a80) at ../../Source/WebCore/page/DOMWindow.cpp:1063 #8 0x00007ffff4138190 in WebCore::jsDOMWindowPrototypeFunctionPrint (exec=0x7fffffffc4b0) at DerivedSources/WebCore/JSDOMWindow.cpp:21367 #9 0x00007fff99d500a8 in ?? () #10 0x00007fffffffc500 in ?? () #11 0x00007fffed6d3ca8 in llint_entry () from /home/reni/data/REPOS/webkit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18
I'm on it
Created attachment 245720 [details] Patch
Comment on attachment 245720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245720&action=review > Source/WebKit2/WebProcess/WebPage/gtk/PrinterListGtk.cpp:40 > -RefPtr<PrinterListGtk> PrinterListGtk::shared() > +PassRefPtr<PrinterListGtk> PrinterListGtk::shared() > { > if (s_sharedPrinterList) > - return s_sharedPrinterList; > + return adoptRef(s_sharedPrinterList); This is tricky (and probably buggy) but there was a reason to do it this way, see my comments in: https://bugs.webkit.org/show_bug.cgi?id=126979#c3 We should probably find a better way to do this.
Comment on attachment 245720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245720&action=review > Source/WebKit2/ChangeLog:9 > + PassRefPtr. Apart from that it was not adopting the reference of We are moving away from PassRefPtr and using RefPtr as return value now.
Comment on attachment 245720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245720&action=review > Source/WebKit2/WebProcess/WebPage/gtk/PrinterListGtk.cpp:-37 > -RefPtr<PrinterListGtk> PrinterListGtk::shared() You can likely return a PrinterListGtk& instead. > Source/WebKit2/WebProcess/WebPage/gtk/PrinterListGtk.cpp:39 > if (s_sharedPrinterList) You should probably use the same pattern for other singletons: static NeverDestroyed<PrinterListGtk> sharedPrinterList; return sharedPrinterList;
(In reply to comment #5) > Comment on attachment 245720 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245720&action=review > > > Source/WebKit2/WebProcess/WebPage/gtk/PrinterListGtk.cpp:-37 > > -RefPtr<PrinterListGtk> PrinterListGtk::shared() > > You can likely return a PrinterListGtk& instead. > > > Source/WebKit2/WebProcess/WebPage/gtk/PrinterListGtk.cpp:39 > > if (s_sharedPrinterList) > > You should probably use the same pattern for other singletons: > static NeverDestroyed<PrinterListGtk> sharedPrinterList; > return sharedPrinterList; This is not exactly a singleton, and we do really want to destroy it on every print operation. The shared() method here is very confusing, I should have used getOrCreate instead. I think we can achieve the same using Ref<>, though.
Created attachment 245861 [details] Patch
Comment on attachment 245861 [details] Patch Oops, I set the wrong flag.
Created attachment 245862 [details] Rebased patch
Comment on attachment 245862 [details] Rebased patch The whole thing was indeed really confusing. The new name fits better indeed. Have you checked that the ASSERT is not hit with your change?
(In reply to comment #10) > Comment on attachment 245862 [details] > Rebased patch > > The whole thing was indeed really confusing. The new name fits better indeed. I agree > Have you checked that the ASSERT is not hit with your change? No I haven't. The assert happens with you do RefPtr<Foo> f = adoptRef(new Foo); I think, which is no longer the case.
Comment on attachment 245862 [details] Rebased patch I told Carlos on IRC that the ASSERT was triggered by "return s_sharedPrinterList;" not on the adoptPtr. He confirmed that the ASSERT is hit even with his patch, so it deserves another look.
Comment on attachment 245862 [details] Rebased patch Clearing flags, since the patch is not exactly wrong but it doesn't fix the assert, because the issue is not the smart pointers.
So, the problem is not the smart pointer, but the nested main loop used by gtk_enumerate_printers again (see also bug #126979). We call PrinterListGtk::getOrCreate() in WebChromeClient::print() to ensure that for sync printing, the print operation doesn't run gtk_enumerate_printers(), because the nested main loop might process the GSource of the EndPrinting message, and finish the print operation unexpectedly causing a crash. The problem now is very similar. In this case there are two different calls to WebChromeClient::print(). The first one creates the PrinterListGtk(), but while gtk_enumerate_printers() is getting the printers, its nested main loop processes the GSource for the second print operation that ends up in WebChromeClient::print() again. So, PrinterListGtk::getOrCreate() is called again, but the previous one hasn't finished, the global pointer has been updated, but the constructor hasn't finished, so we try to get a reference of an object that is still being constructed. This is not easy to fix, in both cases WebChromeClient::print() is called from the same thread, so we can't use a mutex or we would end up blocking the main thread, and gtk_enumerate_printers would never finish. The only think I can think of is running gtk_enumerate_printers() in a helper thread, blocking the main thread until the helper finishes (joining the thread, since print() is sync, a second print() should never be processed until the previous one has finished). But the nested main loop of gtk_enumerate_printers uses the default main context unconditionally, so the following print operation would still be handled by the nested main loop, but now in the thread, causing the thread to never finish. We could pass wait=False to gtk_enumerate_printers() and use our own main loop and context. That way the following print would be sent to the default main context, but the main loop would be blocked waiting for the helper thread to finish. This approach would work if it wasn't because the cups printing backend of GTK+ uses custom GSources attached to the default main context instead of the default context of the current thread. I've written a patch for GTK+, but we would still need to decide what to do when the GTK+ version is not recent enough (once we have a GTK+ version with the fix). See https://bugzilla.gnome.org/show_bug.cgi?id=743857
Created attachment 246027 [details] New patch This is a different approach, because using a secondary thread is too risky, it could be easy to make the web process hang forever waiting for the thread to finish if something changes in GTK+ or if a third-party GTK+ print backend is used. We also avoid having to depend on a specific GTK+ version for this fix. With the threads fix, we were showing the print dialog twice, once the first one is closed the second one shows up, which doesn't make much sense but it's what the test case does. However, I 've realized that both firefox and chromium show the print dialog only once for that test case. So, I think the best and easiest approach is to just ignore the second print if the first print is still enumerating the printers. I've tried to write a unit tests for this, for I couldn't reproduce the issue in a test case, note that this depends on how fast things happen, and in unit tests things are usually faster. This is a very edge case in any case, so the important thing is to no assert.
Comment on attachment 246027 [details] New patch View in context: https://bugs.webkit.org/attachment.cgi?id=246027&action=review r=me with one nit. > Source/WebKit2/WebProcess/WebPage/gtk/PrinterListGtk.cpp:37 > +RefPtr<PrinterListGtk> PrinterListGtk::getOrCreate() getOrCreate() generally means that we're always getting a valid pointer. That's why I think we should name it maybeGetOrCreate().
(In reply to comment #16) > Comment on attachment 246027 [details] > New patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246027&action=review > > r=me with one nit. Thanks! > > Source/WebKit2/WebProcess/WebPage/gtk/PrinterListGtk.cpp:37 > > +RefPtr<PrinterListGtk> PrinterListGtk::getOrCreate() > > getOrCreate() generally means that we're always getting a valid pointer. > That's why I think we should name it maybeGetOrCreate(). Well, there are a lot of ::create() methods that can return nullptr and they are not maybeCreate() :-)
Committed r179744: <http://trac.webkit.org/changeset/179744>