Bug 32900

Summary: [GTK] Crashes cleaning clipboard contents
Product: WebKit Reporter: Gustavo Noronha (kov) <gns>
Component: WebKitGTKAssignee: Gustavo Noronha (kov) <gns>
Status: RESOLVED FIXED    
Severity: Normal CC: danilo, diegoe, eric, jonathon, mrobinson, pclouds, slomo, tevaum, webkit.review.bot
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
stack trace
none
Make EditorClientGtk refcounted, and use explict refs to protect it for the callbacks
gns: commit-queue-
Null-check the focus controller, because it may be null when the callback is called, leading to crash
gns: commit-queue-
Fix crash by passing WebView object to the clipboard functions instead of Page
gns: commit-queue-
Fix crash by passing WebView instead of Page xan.lopez: review+, gns: commit-queue-

Description Gustavo Noronha (kov) 2009-12-23 06:45:02 PST
It looks like, with page cache enabled, the destruction of the EditorClient being delayed may end up with it crashing. That is because a callback is being called late - after the destruction of the client.
Comment 1 Gustavo Noronha (kov) 2009-12-23 07:01:51 PST
Created attachment 45436 [details]
stack trace

How I reproduce it: I run Epiphany, go to a page, select some text, open a new tab, close the first tab, select some new text, rinse and repeat until you get a crash.
Comment 2 Gustavo Noronha (kov) 2009-12-23 07:03:43 PST
Created attachment 45437 [details]
Make EditorClientGtk refcounted, and use explict refs to protect it for the callbacks
Comment 3 WebKit Review Bot 2009-12-23 07:08:32 PST
style-queue ran check-webkit-style on attachment 45437 [details] without any errors.
Comment 4 Estêvão Samuel Procópio Amaral 2009-12-23 08:31:35 PST
Having the same problem here. Waiting for the patch to be reviewed. Anyone?!?
Comment 5 Duy Nguyen 2009-12-27 21:59:02 PST
With Gustavo's patch, I got two stack traces (probably not reproducible). The latter may due to some other bug.

(gdb) bt
#0  0xb719af2b in WebKit::clipboard_clear_contents_cb () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#1  0xb6156843 in clipboard_unset () from /usr/lib/libgtk-x11-2.0.so.0
#2  0xb61568d1 in selection_clear_event_cb () from /usr/lib/libgtk-x11-2.0.so.0
#3  0xb603284b in _gtk_marshal_BOOLEAN__BOXED () from /usr/lib/libgtk-x11-2.0.so.0
#4  0xb53bb2eb in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
#5  0xb53cc388 in signal_emit_unlocked_R () from /usr/lib/libgobject-2.0.so.0
#6  0xb53cd5ac in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#7  0xb53cdb8f in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
#8  0xb613b2b8 in gtk_widget_event_internal () from /usr/lib/libgtk-x11-2.0.so.0
#9  0xb602d304 in gtk_main_do_event () from /usr/lib/libgtk-x11-2.0.so.0
#10 0xb5ec75dd in gdk_event_dispatch () from /usr/lib/libgdk-x11-2.0.so.0
#11 0xb52ec3bf in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#12 0xb52eccb6 in g_main_context_iterate () from /usr/lib/libglib-2.0.so.0
#13 0xb52eceb1 in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
#14 0xb602d5dc in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#15 0x08072109 in main (argc=-2082109099, argv=0xbfc50984) at ephy-main.c:778


(gdb) bt
#0  0x00610065 in ?? ()
#1  0xb751a138 in WebCore::Editor::clientIsEditable () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#2  0xb76b7288 in WebCore::Frame::isContentEditable () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#3  0xb75a9138 in WebCore::HTMLElement::isContentEditable () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#4  0xb74bfded in WebCore::Node::isContentEditable () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#5  0xb74c0942 in WebCore::Node::rootEditableElement () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#6  0xb7780b89 in WebCore::RenderBlock::isSelectionRoot () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#7  0xb7780c68 in WebCore::RenderBlock::shouldPaintSelectionGaps () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#8  0xb778cfda in WebCore::RenderBlock::paintSelection () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#9  0xb778d2fe in WebCore::RenderBlock::paintObject () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#10 0xb777fbbd in WebCore::RenderBlock::paint () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#11 0xb778688d in WebCore::RenderBlock::paintChildren () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#12 0xb7786a26 in WebCore::RenderBlock::paintContents () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#13 0xb778d1f3 in WebCore::RenderBlock::paintObject () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#14 0xb777fbbd in WebCore::RenderBlock::paint () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#15 0xb778688d in WebCore::RenderBlock::paintChildren () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#16 0xb7786a26 in WebCore::RenderBlock::paintContents () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#17 0xb778d1f3 in WebCore::RenderBlock::paintObject () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#18 0xb777fbbd in WebCore::RenderBlock::paint () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#19 0xb77d781c in WebCore::RenderLayer::paintLayer () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#20 0xb77d7921 in WebCore::RenderLayer::paintLayer () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#21 0xb77d81cb in WebCore::RenderLayer::paint () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#22 0xb76c6265 in WebCore::FrameView::paintContents () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#23 0xb770bbde in WebCore::ScrollView::paint () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#24 0xb71b780e in webkit_web_view_expose_event () from /home/pclouds/opt/webkit/lib/libwebkit-1.0.so.2
#25 0xb602c84b in _gtk_marshal_BOOLEAN__BOXED () from /usr/lib/libgtk-x11-2.0.so.0
#26 0xb53b4495 in g_type_class_meta_marshal () from /usr/lib/libgobject-2.0.so.0
#27 0xb53b62eb in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
#28 0xb53c7737 in signal_emit_unlocked_R () from /usr/lib/libgobject-2.0.so.0
#29 0xb53c85ac in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#30 0xb53c8b8f in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
#31 0xb61352b8 in gtk_widget_event_internal () from /usr/lib/libgtk-x11-2.0.so.0
#32 0xb60273a9 in gtk_main_do_event () from /usr/lib/libgtk-x11-2.0.so.0
#33 0xb5eaa41c in _gdk_window_process_updates_recurse () from /usr/lib/libgdk-x11-2.0.so.0
#34 0xb5eaa3ce in _gdk_window_process_updates_recurse () from /usr/lib/libgdk-x11-2.0.so.0
#35 0xb5ecf944 in _gdk_windowing_window_process_updates_recurse () from /usr/lib/libgdk-x11-2.0.so.0
#36 0xb5ea6580 in gdk_window_process_updates_internal () from /usr/lib/libgdk-x11-2.0.so.0
#37 0xb5ea8745 in gdk_window_process_all_updates () from /usr/lib/libgdk-x11-2.0.so.0
#38 0xb5ea8764 in gdk_window_update_idle () from /usr/lib/libgdk-x11-2.0.so.0
#39 0xb5e87e1a in gdk_threads_dispatch () from /usr/lib/libgdk-x11-2.0.so.0
#40 0xb52e30c2 in g_idle_dispatch () from /usr/lib/libglib-2.0.so.0
#41 0xb52e73bf in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#42 0xb52e7cb6 in g_main_context_iterate () from /usr/lib/libglib-2.0.so.0
#43 0xb52e7eb1 in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
#44 0xb60275dc in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#45 0x08072109 in main (argc=159697152, argv=0xbf8dd694) at ephy-main.c:778
Comment 6 Gustavo Noronha (kov) 2009-12-29 11:30:24 PST
Just a heads up - Martin Robinson has a patch to improve DND, and it essentially replaces/moves the code I am touching in my patch. See https://bugs.webkit.org/show_bug.cgi?id=30623
Comment 7 Eric Seidel (no email) 2010-01-05 13:45:01 PST
Should this still be up for review then?
Comment 8 Gustavo Noronha (kov) 2010-01-06 04:47:01 PST
Comment on attachment 45437 [details]
Make EditorClientGtk refcounted, and use explict refs to protect it for the callbacks

Removed from review queue for now.
Comment 9 Diego Escalante Urrelo 2010-01-13 19:52:00 PST
Happens also if you select text somewhere else (other apps). Fun :)
Comment 10 Gustavo Noronha (kov) 2010-01-14 17:09:19 PST
Created attachment 46621 [details]
Null-check the focus controller, because it may be null when the callback is called, leading to crash

ok, I found what's wrong with Martin's approach, it seems; wasn't able to crash Epiphany anymore with this
Comment 11 Eric Seidel (no email) 2010-01-14 17:11:53 PST
Comment on attachment 46621 [details]
Null-check the focus controller, because it may be null when the callback is called, leading to crash

OK.
Comment 12 Gustavo Noronha (kov) 2010-01-14 17:27:32 PST
Patch landed as 53304.
Comment 13 Gustavo Noronha (kov) 2010-01-15 07:52:58 PST
*** Bug 33387 has been marked as a duplicate of this bug. ***
Comment 14 Sebastian Dröge (slomo) 2010-01-16 05:01:55 PST
Unfortunately this is not enough, still crashes for me with latest trunk of epiphany and webkit:

#0  0x00007ffff4fa2353 in WebKit::clearClipboardContentsCallback(_GtkClipboard*, void*) ()
   from /usr/local/lib/libwebkit-1.0.so.2
#1  0x00007ffff3f9438a in clipboard_unset (clipboard=0x813ba0)
    at /gtk+2.0-2.19.3/gtk/gtkclipboard.c:693
#2  0x00007ffff3f943fc in selection_clear_event_cb (
    widget=<value optimized out>, event=<value optimized out>)
    at /gtk+2.0-2.19.3/gtk/gtkclipboard.c:349
#3  0x00007ffff3e483d3 in _gtk_marshal_BOOLEAN__BOXED (closure=0x85fa40, 
    return_value=0x7fffffffcbe0, n_param_values=<value optimized out>, 
    param_values=0xef8840, invocation_hint=<value optimized out>, 
    marshal_data=0x7ffff3f943e0) at /gtk+2.0-2.19.3/gtk/gtkmarshalers.c:84
#4  0x00007ffff1e717ce in IA__g_closure_invoke (closure=0x85fa40, 
    return_value=0x7fffffffcbe0, n_param_values=2, param_values=0xef8840, 
    invocation_hint=0x7fffffffcba0)
    at /build/buildd/glib2.0-2.23.1/gobject/gclosure.c:767
#5  0x00007ffff1e86776 in signal_emit_unlocked_R (node=0x73f600, 
    detail=<value optimized out>, instance=<value optimized out>, 
    emission_return=<value optimized out>, 
    instance_and_params=<value optimized out>)
    at /build/buildd/glib2.0-2.23.1/gobject/gsignal.c:3247
#6  0x00007ffff1e87a7e in IA__g_signal_emit_valist (instance=0x82f070, 
    signal_id=<value optimized out>, detail=0, var_args=0x7fffffffcd90)
---Type <return> to continue, or q <return> to quit---
    at /build/buildd/glib2.0-2.23.1/gobject/gsignal.c:2990
#7  0x00007ffff1e88453 in IA__g_signal_emit (instance=0x3f00000040, 
    signal_id=4158854264, detail=3792310656)
    at /build/buildd/glib2.0-2.23.1/gobject/gsignal.c:3037
#8  0x00007ffff3f75d0f in gtk_widget_event_internal (widget=0x82f070, 
    event=0x12f8920) at /gtk+2.0-2.19.3/gtk/gtkwidget.c:4941
#9  0x00007ffff3e403a5 in IA__gtk_main_do_event (event=0x12f8920)
    at /gtk+2.0-2.19.3/gtk/gtkmain.c:1601
#10 0x00007ffff384142c in gdk_event_dispatch (source=<value optimized out>, 
    callback=<value optimized out>, user_data=<value optimized out>)
    at /gtk+2.0-2.19.3/gdk/x11/gdkevents-x11.c:2372
#11 0x00007ffff13c2a7e in g_main_dispatch (context=0x71b770)
    at /build/buildd/glib2.0-2.23.1/glib/gmain.c:1960
#12 IA__g_main_context_dispatch (context=0x71b770)
    at /build/buildd/glib2.0-2.23.1/glib/gmain.c:2513
#13 0x00007ffff13c6438 in g_main_context_iterate (context=0x71b770, 
    block=<value optimized out>, dispatch=<value optimized out>, 
    self=<value optimized out>)
    at /build/buildd/glib2.0-2.23.1/glib/gmain.c:2591
#14 0x00007ffff13c6895 in IA__g_main_loop_run (loop=0x7a54e0)
    at /build/buildd/glib2.0-2.23.1/glib/gmain.c:2799
#15 0x00007ffff3e40777 in IA__gtk_main () at /gtk+2.0-2.19.3/gtk/gtkmain.c:1219
#16 0x0000000000435b33 in main (argc=1, argv=0x7fffffffe2c8) at ephy-main.c:739
Comment 15 Estêvão Samuel Procópio Amaral 2010-01-16 10:09:02 PST
Well... sure it's more stable with this patch. But I'm still getting some random segfaults and I think it's still related to clipboard. Here is a full bt:

Program received signal SIGSEGV, Segmentation fault.
0xb5d609ff in WTF::RefCountedBase::derefBase (this=0x670072) at ./JavaScriptCore/wtf/RefCounted.h:67
67              ASSERT(!m_deletionHasBegun);
Current language:  auto
The current source language is "auto; currently c++".
(gdb) bt full
#0  0xb5d609ff in WTF::RefCountedBase::derefBase (this=0x670072) at ./JavaScriptCore/wtf/RefCounted.h:67
        __PRETTY_FUNCTION__ = "bool WTF::RefCountedBase::derefBase()"
#1  0xb5d744d7 in WTF::RefCounted<WebCore::Range>::deref (this=0x670072) at ./JavaScriptCore/wtf/RefCounted.h:108
No locals.
#2  0xb660bfa8 in WTF::RefPtr<WebCore::Range>::operator= (this=0xbcd777c, optr=0x0) at ./JavaScriptCore/wtf/RefPtr.h:132
        ptr = 0x670072
#3  0xb660a058 in WebKit::clipboard_clear_contents_cb (clipboard=0x82304b8, data=0xbcd7748) at WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:209
        client = 0xbcd7748
#4  0xb5a9dfef in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#5  0xb5a9e046 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#6  0xb593b322 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#7  0xb5447f62 in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#8  0xb545c3a8 in ?? () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#9  0xb545d5b8 in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#10 0xb545dba6 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#11 0xb5a7d1be in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#12 0xb593408f in gtk_main_do_event () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#13 0xb5757b2a in ?? () from /usr/lib/libgdk-x11-2.0.so.0
No symbol table info available.
#14 0xb53a7b38 in g_main_context_dispatch () from /lib/libglib-2.0.so.0
No symbol table info available.
#15 0xb53ab3d0 in ?? () from /lib/libglib-2.0.so.0
No symbol table info available.
#16 0xb53ab83f in g_main_loop_run () from /lib/libglib-2.0.so.0
No symbol table info available.
#17 0xb5934539 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#18 0x0806f7bb in main (argc=1, argv=0xbfffefd4) at ephy-main.c:739
        option_context = <value optimized out>
        option_group = <value optimized out>
        proxy = <value optimized out>
        error = 0x0
        user_time = 65083348
Comment 16 Danilo Šegan 2010-01-16 10:36:54 PST
Yeah, it's definitely more stable. But I still get a different stacktrace myself with webkit r53331 from yesterday or so:

#0  clearClipboardContentsCallback (clipboard=<value optimized out>, data=0x7f5b62f03b40)
    at ../WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.cpp:137
        frame = 0x3f00000040
        dataObject = <value optimized out>
#1  0x00007f5b85a5d8ea in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#2  0x00007f5b85a5eecd in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#3  0x00007f5b85a5f0b8 in gtk_clipboard_set_with_owner () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#4  0x00007f5b858bde9c in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#5  0x00000000004840ea in ephy_location_entry_activate (entry=<value optimized out>)
    at /build/buildd/epiphany-browser-2.29.5/lib/widgets/ephy-location-entry.c:1510
        priv = 0x15a0af0
        toplevel = 0x213a480
#6  0x000000000043b061 in ephy_toolbar_activate_location (toolbar=0x1c11180)
    at /build/buildd/epiphany-browser-2.29.5/src/ephy-toolbar.c:455
        proxies = <value optimized out>
        entry = <value optimized out>
        visible = 1
#7  0x00007f5b8472d5ae in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#8  0x00007f5b84742983 in ?? () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#9  0x00007f5b84743d39 in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#10 0x00007f5b84744283 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#11 0x00007f5b85874d13 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#12 0x00007f5b858772e9 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#13 0x00007f5b8472d5ae in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#14 0x00007f5b84742983 in ?? () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#15 0x00007f5b84743bcc in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#16 0x00007f5b84744283 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#17 0x00007f5b85871094 in gtk_accel_group_activate () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#18 0x00007f5b8587119d in gtk_accel_groups_activate () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#19 0x00007f5b85a59113 in gtk_window_activate_key () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#20 0x00007f5b85a59199 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#21 0x00007f5b8593c728 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#22 0x00007f5b8472d5ae in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#23 0x00007f5b8474264d in ?? () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#24 0x00007f5b84743bcc in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#25 0x00007f5b84744283 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#26 0x00007f5b85a4371f in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#27 0x00007f5b85934da4 in gtk_propagate_event () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#28 0x00007f5b85935ca3 in gtk_main_do_event () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#29 0x00007f5b85355cdc in ?? () from /usr/lib/libgdk-x11-2.0.so.0
No symbol table info available.
#30 0x00007f5b8408bbce in g_main_context_dispatch () from /lib/libglib-2.0.so.0
No symbol table info available.
#31 0x00007f5b8408f598 in ?? () from /lib/libglib-2.0.so.0
No symbol table info available.
#32 0x00007f5b8408f9f5 in g_main_loop_run () from /lib/libglib-2.0.so.0
No symbol table info available.
#33 0x00007f5b85936177 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#34 0x0000000000435203 in main (argc=1, argv=0x7fff6214c698)
    at /build/buildd/epiphany-browser-2.29.5/src/ephy-main.c:741
        option_context = <value optimized out>
        option_group = <value optimized out>
        proxy = <value optimized out>
        error = 0x0
        user_time = 196643610

(excuse my lack of symbols in gtk+ and glib libraries)
Comment 17 Gustavo Noronha (kov) 2010-01-16 13:03:05 PST
I posted a patch I think fixes these crashes to https://bugs.webkit.org/show_bug.cgi?id=33746.
Comment 18 Gustavo Noronha (kov) 2010-01-16 16:25:08 PST
*** Bug 33746 has been marked as a duplicate of this bug. ***
Comment 19 Gustavo Noronha (kov) 2010-01-16 16:28:28 PST
Created attachment 46753 [details]
Fix crash by passing WebView object to the clipboard functions instead of Page

This is the same patch I posted to 33746 (which I marked as duplicate to this one) with a couple changes:

1. adopting the recommendation of mrobinson about checking if data is NULL, because in the near future it might be (for non-X11 primary clipboards)
2. check the return of set_with_data, and unref the webView if the call failed, to avoid leaking

Martin also raised the issue of the webView being unrefed in the get function - my original patch did that as well, and I couldn't see how it would be called twice, or in addition to clear yet (by looking at the GTK+ code), but I may be wrong.
Comment 20 Sebastian Dröge (slomo) 2010-01-17 00:11:09 PST
Works good so far but I guess it needs some longer testing if it really fixes all clipboard issues :)
Comment 21 Martin Robinson 2010-01-17 09:16:53 PST
(In reply to comment #19)
> Martin also raised the issue of the webView being unrefed in the get function -
> my original patch did that as well, and I couldn't see how it would be called
> twice, or in addition to clear yet (by looking at the GTK+ code), but I may be
> wrong.

The get function is actually called every time you middle button paste the contents of the clipboard. So if you middle button paste a bunch of times you'll actually see this cause critical warnings like:

(GtkLauncher:13194): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed

The only other change I would make to this patch is to change the order of these lines in PasteboardHelperGtk.cpp:

>    ASSERT(dataObject);
>
>    // This will be true for clipboards other than X11 primary.
>    if (!data)
>        return;
>
>    dataObject->clear();

In the future, we'll want dataObject->clear() to happen regardless of whether or not this is an X11 primary clipboard (data != NULL).

Looks great otherwise!
Comment 22 Gustavo Noronha (kov) 2010-01-17 16:48:26 PST
(In reply to comment #21)
> (In reply to comment #19)
> > Martin also raised the issue of the webView being unrefed in the get function -
> > my original patch did that as well, and I couldn't see how it would be called
> > twice, or in addition to clear yet (by looking at the GTK+ code), but I may be
> > wrong.
> 
> The get function is actually called every time you middle button paste the
> contents of the clipboard. So if you middle button paste a bunch of times
> you'll actually see this cause critical warnings like:
> 
> (GtkLauncher:13194): GLib-GObject-CRITICAL **: g_object_unref: assertion
> `G_IS_OBJECT (object)' failed

I didn't get those here. Is it certain that clear will happen, though?

> >    ASSERT(dataObject);
> >
> >    // This will be true for clipboards other than X11 primary.
> >    if (!data)
> >        return;
> >
> >    dataObject->clear();
> 
> In the future, we'll want dataObject->clear() to happen regardless of whether
> or not this is an X11 primary clipboard (data != NULL).
> 
> Looks great otherwise!

OK!
Comment 23 Gustavo Noronha (kov) 2010-01-17 16:51:10 PST
Created attachment 46772 [details]
Fix crash by passing WebView instead of Page

OK, this addresses the comments Martin made about the patch.
Comment 24 Sebastian Dröge (slomo) 2010-01-17 22:25:13 PST
(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #19)
> > > Martin also raised the issue of the webView being unrefed in the get function -
> > > my original patch did that as well, and I couldn't see how it would be called
> > > twice, or in addition to clear yet (by looking at the GTK+ code), but I may be
> > > wrong.
> > 
> > The get function is actually called every time you middle button paste the
> > contents of the clipboard. So if you middle button paste a bunch of times
> > you'll actually see this cause critical warnings like:
> > 
> > (GtkLauncher:13194): GLib-GObject-CRITICAL **: g_object_unref: assertion
> > `G_IS_OBJECT (object)' failed
> 
> I didn't get those here. Is it certain that clear will happen, though?

From my understanding of the docs the clear function will only ever by called once and will always be called if the clipboard content changes. The get function can (and will) be called multiple times.
Comment 25 Sebastian Dröge (slomo) 2010-01-19 02:54:01 PST
*** Bug 33422 has been marked as a duplicate of this bug. ***
Comment 26 Sebastian Dröge (slomo) 2010-01-19 02:54:11 PST
*** Bug 33421 has been marked as a duplicate of this bug. ***
Comment 27 Xan Lopez 2010-01-19 04:40:08 PST
Comment on attachment 46772 [details]
Fix crash by passing WebView instead of Page

I think the cool thing to do these days would be to use a GOwnPtr in the clear callback for the view? Looks fine to me otherwise, feel free to commit either way.
Comment 28 Gustavo Noronha (kov) 2010-01-19 11:11:58 PST
Landed as r53477.