Bug 141035

Summary:

ASSERTION FAILED: !m_adoptionIsRequired in WTF::RefCountedBase::ref

Product:

WebKit

Reporter:

Renata Hodovan <rhodovan.u-szeged>

Component:

WebKit Misc.

Assignee:

Sergio Villar Senin <svillar>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, cgarcia, gustavo, kling, svillar

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

116980

Attachments:

Description	Flags
Test case	none
Patch	cdumez: review-
Patch	none
Rebased patch	none
New patch	svillar: review+

Renata Hodovan

Reported 2015-01-29 08:50:47 PST

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

Attachments
Test case (88 bytes, text/html) 2015-01-29 08:50 PST, Renata Hodovan	no flags	Details
Patch (2.29 KB, patch) 2015-01-30 07:35 PST, Sergio Villar Senin	cdumez: review-	Details Formatted Diff Diff
Patch (3.94 KB, patch) 2015-02-02 01:26 PST, Carlos Garcia Campos	no flags	Details Formatted Diff Diff
Rebased patch (3.90 KB, patch) 2015-02-02 01:41 PST, Carlos Garcia Campos	no flags	Details Formatted Diff Diff
New patch (6.70 KB, patch) 2015-02-04 05:38 PST, Carlos Garcia Campos	svillar: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Sergio Villar Senin

Comment 1 2015-01-30 00:45:44 PST

I'm on it

Sergio Villar Senin

Comment 2 2015-01-30 07:35:01 PST

Created attachment 245720 [details] Patch

Carlos Garcia Campos

Comment 3 2015-01-30 08:32:46 PST

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.

Chris Dumez

Comment 4 2015-01-30 11:11:20 PST

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.

Chris Dumez

Comment 5 2015-01-30 11:14:42 PST

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;

Carlos Garcia Campos

Comment 6 2015-02-02 01:21:31 PST

(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.

Carlos Garcia Campos

Comment 7 2015-02-02 01:26:28 PST

Created attachment 245861 [details] Patch

Carlos Garcia Campos

Comment 8 2015-02-02 01:35:20 PST

Comment on attachment 245861 [details] Patch Oops, I set the wrong flag.

Carlos Garcia Campos

Comment 9 2015-02-02 01:41:52 PST

Created attachment 245862 [details] Rebased patch

Sergio Villar Senin

Comment 10 2015-02-02 02:00:22 PST

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?

Carlos Garcia Campos

Comment 11 2015-02-02 02:36:07 PST

(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.

Sergio Villar Senin

Comment 12 2015-02-02 03:08:47 PST

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.

Carlos Garcia Campos

Comment 13 2015-02-02 09:32:46 PST

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.

Carlos Garcia Campos

Comment 14 2015-02-02 09:49:17 PST

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

Carlos Garcia Campos

Comment 15 2015-02-04 05:38:15 PST

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.

Sergio Villar Senin

Comment 16 2015-02-06 06:14:38 PST

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().

Carlos Garcia Campos

Comment 17 2015-02-06 06:18:12 PST

(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() :-)

Carlos Garcia Campos

Comment 18 2015-02-06 06:46:30 PST

Committed r179744: <http://trac.webkit.org/changeset/179744>

Note You need to log in before you can comment on or make changes to this bug.