1590965 - nsClipboard::Observe() is spinning the event loop

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

nsClipboard::Observe() calls into gtk_clipboard_store(), which runs a nested event loop.
Spinning the event loop permits calls to nsIObserver::remove(), for example, which is forbidden during notifications

This has led to multiple kinds of UaF problems (due to remove not functioning as expected).
e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1506911

Comment 1

[Karl Tomlinson (:karlt)]

ShutdownXPCOM() processes pending events before advancing to "xpcom-shutdown-threads" and so I assume dispatching an event to call gtk_clipboard_store() would suffice.

I wonder whether both "quit-application" and "xpcom-shutdown" need be observed.

"xpcom-shutdown" dates back to https://hg.mozilla.org/mozilla-central/rev/296dd5f8849a#l3.84
"quit-application" dates back to https://hg.mozilla.org/mozilla-central/rev/270a70535807#l1.94

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: bug 1590965 call gtk_clipboard_store() from an event r?stransky

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: bug 1590965 clear GtkClipboard on nsIClipboard::EmptyClipboard r?stransky

This notifies GTK that the data is no longer available for clipboard_get_cb(),
so that GTK will no longer advertise nor attempt to store the data.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Bug 1590965 call gtk_clipboard_store() regardless of whether Gecko is providing the clipboard r?stransky

This stores the clipboard even if it was set in a GTK dialog.

Depends on D50764

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: Bug 1590965 store clipboard only once on shutdown r?stransky

Depends on D50538

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/70331e97107e45f1e81f2e53b6003e1c89144247
https://hg.mozilla.org/integration/autoland/rev/22f28d146e38491acf458d17119973713e257cba
https://hg.mozilla.org/integration/autoland/rev/1ced33f6635376f0a609fb7a4a26900d5933576f
https://hg.mozilla.org/integration/autoland/rev/f68c212c8b2a39cb6e26e17477a8d848e19cd6d3
https://hg.mozilla.org/mozilla-central/rev/70331e97107e
https://hg.mozilla.org/mozilla-central/rev/22f28d146e38
https://hg.mozilla.org/mozilla-central/rev/1ced33f66353
https://hg.mozilla.org/mozilla-central/rev/f68c212c8b2a

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 9104082 [details]
bug 1590965 call gtk_clipboard_store() from an event r?stransky

Please consider this a request for all patches on this bug.

Beta/Release Uplift Approval Request

• User impact if declined: Potentially unexpected results on shutdown of browser (parent) process, such as a UaF.
  This would be very hard to exploit and so there is no pressing need for uplift to beta (or esr).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The logic has changed a bit, which introduces some risk.
• String changes made/needed: None.

Comment 8

[Pascal Chevrel:pascalc]

Karl, would it be ok to let it ride the 72 train given that there is medium risk introduced by the logic change?

Comment 9

[Karl Tomlinson (:karlt)]

Yes, I think that is a reasonable option here.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout as requested by Karl:
https://hg.mozilla.org/integration/autoland/rev/063086c09dc6b15acc643d85fa7cc8c30d4a5e0e
https://hg.mozilla.org/mozilla-central/rev/063086c09dc6

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/2905fbf1625f2613c3532bdf64b93676fb3d6ae1
https://hg.mozilla.org/mozilla-central/rev/2905fbf1625f

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/610f0e00fbbaf19e502b48b28d33c11e757c3526
https://hg.mozilla.org/integration/autoland/rev/e327b9536917f0247ba28d5e5f8c737f7338317a
https://hg.mozilla.org/mozilla-central/rev/610f0e00fbba
https://hg.mozilla.org/mozilla-central/rev/e327b9536917