Bug 144549 – [GTK] Paste data is removed from clipboard when closing browser tab

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Select some text on this page, right click, Copy. Paste it into another app. Paste works great, yay! Now do the same again, but close the current tab (in Ephy, or close MiniBrowser if you're using that). There is no longer any data to paste. If the user was copying, say, a long Bugzilla reply, he is now quite frustrated that his copied data was deleted.

[Michael Catanzaro]

Created attachment 256948 [details]
Patch

[Martin Robinson]

Comment on attachment 256948 [details]
Patch

Hrm...I'm not sure about this. I think there can be multiple Pasteboards at one time. Is it possible to simply store the clipboard data as soon as a "copy" occurs in a WebView?

[Carlos Garcia Campos]

(In reply to comment #0)
> Select some text on this page, right click, Copy. Paste it into another app.
> Paste works great, yay! Now do the same again, but close the current tab (in
> Ephy, or close MiniBrowser if you're using that). There is no longer any
> data to paste. If the user was copying, say, a long Bugzilla reply, he is
> now quite frustrated that his copied data was deleted.

Yeah, I've suffered this a lot of times, and I always wondered why. Thanks for working on this.

[Carlos Garcia Campos]

Comment on attachment 256948 [details]
Patch

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

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:96
> +    if (hasData()) {
> +        ASSERT(m_gtkClipboard);
> +        gtk_clipboard_store(m_gtkClipboard);
> +    }

This is not correct, and not the correct place either. First you are asserting m_gtkClipboard is not nullptr, but it can be nullptr. In case of private or drag and drop Pasteboard the clipboard will be nullptr, and the data object might have data. Also, I don't think we need to check if there's data first, since that retrieves the data, something that gtk_clipboard_store will do anyway. And finally, this should be done only when the process finishes, not everytime a Pasteboard object is deleted. GTK+ stores all clipboards in gtk_main or gtk_application_shutdown when the main loop finishes. We don't use gtk_main() in the web process, that's why it has never worked.

[Carlos Garcia Campos]

Created attachment 256963 [details]
Different approach

This approach adds API to PasteboardHelper to store all clipboards that is used from WebProcessMain::platformFinalize(). This doesn't currently work due to bug #147036, though.

[Martin Robinson]

Comment on attachment 256963 [details]
Different approach

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

> Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:70
> +        PasteboardHelper::storeAllClipboards();
> +

How exactly does gtk_clipboard_store work? Does it override the current X11 selection?

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 256963 [details]
> Different approach
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256963&action=review
> 
> > Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:70
> > +        PasteboardHelper::storeAllClipboards();
> > +
> 
> How exactly does gtk_clipboard_store work? Does it override the current X11
> selection?

I don't know the details, the doc says "Stores the current clipboard data somewhere so that it will stay around after the application has quit.", and GTK+ does that in gtk_main (once the loop finishes) and gtk_application_shutdown for all applications using gtk_main or GtkApplication.

[Martin Robinson]

This seems okay to me, but could you do a quick test that this does not override the clipboard contents if the selection belongs to another application when the WebKitGTK+ exits?

[Michael Catanzaro]

Thanks Carlos, your patch looks better indeed.

(In reply to comment #7)
> I don't know the details, the doc says "Stores the current clipboard data
> somewhere so that it will stay around after the application has quit.", and
> GTK+ does that in gtk_main (once the loop finishes) and
> gtk_application_shutdown for all applications using gtk_main or
> GtkApplication.

It goes in the ICCCM clipboard manager:

http://www.freedesktop.org/wiki/Specifications/clipboard-manager-spec/
http://www.x.org/releases/X11R7.7/doc/xorg-docs/icccm/icccm.html

(I have no clue what that is in GNOME; probably mutter I would guess.)

(In reply to comment #8)
> This seems okay to me, but could you do a quick test that this does not
> override the clipboard contents if the selection belongs to another
> application when the WebKitGTK+ exits?

Oh good point. Only the CLIPBOARD selection should be saved, not PRIMARY. But, I am not sure if GTK+ makes sure to ignore gtk_clipboard_store() when called on PRIMARY, or if it's our responsibility to not do that. Probably that will simply never happen, because surely we won't have PRIMARY at the same time as another app does? I am glad PRIMARY does not exist in Wayland; it really complicates things, and I don't pretend to understand it.

[Carlos Garcia Campos]

Yes, primary selection is not affected. This patch makes WebKitGTK+ behave like any other GTK+ application.

[Carlos Garcia Campos]

Thanks for the review, unfortunately this patch doesn't work if patch attached to bug #147036 isn't landed first.

[Carlos Garcia Campos]

Another way to solve this without depending on bug #147036 that it seems it's not going to be fixed, would be making PasteboardHelper singleton destructible, and store the clipboards in the destructor. The pasteboard helpers created in other processes than the web process won't be a problem, since they won't have any clipboard registered. And another way to solve this, would be to move all the pasteboard handling to the ui process so that only the UI process writes into the clipboard, and the clipboard will be saved when gtk_main_quit or gtk_application_shutdown is called, but that would be a lot more work.

[Carlos Garcia Campos]

Created attachment 257747 [details]
Another approach

[Michael Catanzaro]

(In reply to comment #12)
> Another way to solve this without depending on bug #147036 that it seems
> it's not going to be fixed, would be making PasteboardHelper singleton
> destructible, and store the clipboards in the destructor.

This seems cleaner to me anyway.

[Carlos Garcia Campos]

Comment on attachment 256963 [details]
Different approach

Clearing flags here then

[Carlos Garcia Campos]

Committed r187580: <http://trac.webkit.org/changeset/187580>