2001075 - Spurious tabs detaching when clicking

Status: REOPENED

(Core :: Widget: Gtk, defect, P3)

Description

[Olivier Fourdan]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:145.0) Gecko/20100101 Firefox/145.0

Steps to reproduce:

No clear reproducer steps unfortunately.

I have my gmail window pinned in Firefox.

It seems that Firefox is getting confused with button clicks because most often than not, FF reckons I dragged the tab off and moves it to a separate window, while I haven't.

Actual results:

The tab gets moved to a new window.

Expected results:

The tab remains in the window.

Comment 1

[Olivier Fourdan]

This happening on Linux using GNOME Shell as a Wayland compositor.

The issue seems fairly new, it wasn't happening a couple of months ago, which would make it a possible regression.

Martin pointed out at this code change as a possible cause:
https://searchfox.org/firefox-main/rev/f8f007a94eb7b4c6f7168cefeb2555a3bdba04c1/widget/gtk/nsWindow.cpp#7709

This was to fix:
https://bugzilla.mozilla.org/show_bug.cgi?id=1979719

Comment 2

[Martin Stránský [:stransky] (ni? me) (PTO till June 6)]

Please try to catch on log with WAYLAND_DEBUG=1 MOZ_LOG="Widget:5,WidgetDrag:5" env variables (wayland + widget D&D log).
Thanks.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Widget: Gtk' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The product::component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit BugBot documentation.

Comment 5

[Olivier Fourdan]

Attachment: firefox-last-5000-messages.log.bz2

Reproduced the issue with the logs enabled, this is the last 5000 entries in the log.

Comment 6

[Thinker Li [:sinker]]

(In reply to Olivier Fourdan from comment #5)

Created attachment 9528099 [details]
firefox-last-5000-messages.log.bz2

Reproduced the issue with the logs enabled, this is the last 5000 entries in the log.

I don't see any drag event in the log. Could it be that you waited too long after dragging?
Could you run firefox from a terminal and switching back to the terminal to press ctrl-c to terminate firefox immediately when dragging happen?

Comment 7

[Martin Stránský [:stransky] (ni? me) (PTO till June 6)]

Olivier,
can you attach longer log please? There should be 'WaylandDragWorkaround applied, quit D&D session' log line there:
https://searchfox.org/firefox-main/rev/70425199e9a5fa80aa7419fa51763013a67226e1/widget/gtk/nsWindow.cpp#7729
Thanks.

Comment 8

[Olivier Fourdan]

Attachment: Last 5000 log entries before 'WaylandDragWorkaround applied"

New archive with the last 5000 messages before the "D/WidgetDrag WaylandDragWorkaround applied, quit D&D session" entry

Comment 9

[Olivier Fourdan]

Just an idea, maybe we could prevent dragging away pinned tabs, that would definitely help in my case…
Or just disable the workaround for pinned tabs?

Comment 10

[Martin Stránský [:stransky] (ni? me) (PTO till June 6)]

Thanks, there's the relevant part of the log:

[Parent 2612940: Main Thread]: D/WidgetDrag [D 0][7f14d62f7e00] nsDragSession::nsDragSession()
[Parent 2612940: Main Thread]: D/WidgetDrag [D 0][7f14d62f7e00] nsDragSession::InvokeDragSession
[Parent 2612940: Main Thread]: D/WidgetDrag [D 0][7f14d62f7e00] nsDragSession::InvokeDragSessionImpl
[Parent 2612940: Main Thread]: D/WidgetDrag [D 0][7f14d62f7e00]   numDragItems = 1
[Parent 2612940: Main Thread]: D/WidgetDrag adding target application/x-moz-tabbrowser-tab
[Parent 2612940: Main Thread]: D/WidgetDrag adding target text/x-moz-text-internal
[3312712.400] {Default Queue}  -> wl_compositor#4.create_surface(new id wl_surface#349)
[3312712.409] {Default Queue}  -> wl_data_device_manager#14.create_data_source(new id wl_data_source#429)
[3312712.411] {Default Queue}  -> wl_data_source#429.offer("application/x-moz-tabbrowser-tab")
[3312712.413] {Default Queue}  -> wl_data_source#429.offer("text/x-moz-text-internal")
[3312712.416] {Default Queue}  -> wl_data_source#429.set_actions(3)
[3312712.418] {Default Queue}  -> wl_data_device#30.start_drag(wl_data_source#429, wl_surface#61, wl_surface#349, 309024)
[3312712.427] {Default Queue}  -> wp_cursor_shape_device_v1#34.set_shape(309022, 1)
[Parent 2612940: Main Thread]: D/WidgetDrag invisibleSourceDragBegin (7f14fa4fb030)
[Parent 2612940: Main Thread]: D/WidgetDrag [D 0][7f14d62f7e00] nsDragSession::SourceBeginDrag(7f14fa4fb030)
[Parent 2612940: Main Thread]: D/WidgetDrag [D 0][7f14d62f7e00] nsDragSession::SetDragIcon(7f14fa4fb030)
[Parent 2612940: Main Thread]: D/WidgetDrag [D 0][7f14d62f7e00]   set drag popup [7f150df51300]
[3312728.458] {Default Queue}  -> zwp_text_input_v3#69.disable()
[3312728.466] {Default Queue}  -> zwp_text_input_v3#69.commit()
[3312728.808] {Default Queue}  -> wl_surface#349.set_buffer_scale(1)
[Parent 2612940: Main Thread]: D/Widget moz_container_realize() [7f150df51300] GdkWindow 7f14fa1194a0
[Parent 2612940: Main Thread]: D/Widget moz_container_map() [7f150df51300]
[3312728.981] {Default Queue}  -> wl_data_source#429.offer("application/x-moz-tabbrowser-tab")
[3312728.985] {Default Queue}  -> wl_data_source#429.offer("text/x-moz-text-internal")
[3312728.987] {Default Queue}  -> wl_data_source#429.offer("DELETE")
[Parent 2612940: Main Thread]: D/WidgetDrag [D 0][7f14d62f7e00]   GdkDragContext [7f14fa4fb030] nsWindow [7f15272ea600]

[Parent 2612940: Main Thread]: D/Widget [7f15272ea600]: Button 1 release
[Parent 2612940: Main Thread]: D/WidgetDrag WaylandDragWorkaround applied, quit D&D session
[Parent 2612940: Main Thread]: D/WidgetDrag [D 0][7f14d62f7e00] nsDragSession::EndDragSessionImpl(0) 1

If D&D is performed we're not getting 'Button 1 release' event - that's consumed by D&D event. So this is a race between D&D handler which is starting and button event handler.

Comment 11

[Martin Stránský [:stransky] (ni? me) (PTO till June 6)]

Looks like the correct fix is to fire artificial D&D move event to make D&D backend happy:
https://bugzilla.mozilla.org/show_bug.cgi?id=1979719#c12
and then call the D&D cancel to terminate it correctly.

Comment 12

[Martin Stránský [:stransky] (ni? me) (PTO till June 6)]

Attachment: Bug 2001075 [Wayland] Use ScheduleDropEvent() to cancel bogus D&D operation on Wayland r?emilio

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3eddd4cf7d3d
https://hg.mozilla.org/integration/autoland/rev/5da78bc2a288
[Wayland] Use ScheduleDropEvent() to cancel bogus D&D operation on Wayland r=emilio

Comment 14

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/5da78bc2a288

Comment 15

[Martin Stránský [:stransky] (ni? me) (PTO till June 6)]

If the fix here doesn't stick we can issue D&D move and then D&D drop to make sure we emulate the expected behavior.

Comment 16

[Landry Breuil (:gaston)]

it seems i have a regression related to this commit (according to the timeframe) in my builder using some with --with-system options (cf http://buildbot.rhaalovely.net/#/builders/6)

 0:03.13   --prefix=/usr/local
 0:03.13   --enable-release
 0:03.13   --enable-sandbox
 0:03.13   --enable-forkserver
 0:03.13   --with-wasi-sysroot=/usr/local/share/wasi-sysroot
 0:03.13   --disable-tests
 0:03.13   --enable-lto=thin
 0:03.14   --with-system-nss
 0:03.14   --with-system-nspr
 0:03.14   --with-system-icu
 0:03.14   --with-system-av1
 0:03.14   --disable-dbus
 0:03.14   --with-system-zlib
 0:03.14   --enable-official-branding
 0:03.14   --enable-optimize=-O2 -pipe -g
 0:03.14   --disable-updater
 0:03.14   --enable-default-toolkit=cairo-gtk3
 0:03.14   --enable-project=browser
 0:03.14   --enable-debug-symbols
 0:03.14   --disable-install-strip
 0:03.14   CC=/usr/local/bin/clang-19
 0:03.14   CXX=/usr/local/bin/clang++-19
 0:03.14   LDFLAGS=-Wl,--no-keep-memory -Wl,--threads=4

but not the 'stock' one (http://buildbot.rhaalovely.net/#/builders/3) ..

i think the error message points to this commit (cf http://buildbot.rhaalovely.net/api/v2/logs/92740/raw_inline):

13:58.75 W In file included from StaticComponents.cpp:7:
13:58.75 W In file included from /build/buildslave/mozilla-central-portopt/build/xpcom/components/StaticComponents.h:17:
13:58.75 W In file included from /build/obj/buildslave/mozilla-central/dist/include/mozilla/Components.h:10:
13:58.76 W In file included from /build/buildslave/mozilla-central-portopt/build/xpcom/base/nsCOMPtr.h:28:
13:58.76 E /build/obj/buildslave/mozilla-central/dist/include/mozilla/RefPtr.h:49:38: error: member access into incomplete type '_GdkDragContext'
13:58.76 E    49 |   static void Release(U* aPtr) { aPtr->Release(); }
13:58.76 E       |                                      ^
13:58.76 E /build/obj/buildslave/mozilla-central/dist/include/mozilla/RefPtr.h:409:62: note: in instantiation of member function 'mozilla::RefPtrTraits<_GdkDragContext>::Release' requested here
13:58.76 E   409 |     static void Release(U* aPtr) { mozilla::RefPtrTraits<U>::Release(aPtr); }
13:58.76 E       |                                                              ^
13:58.76 E /build/obj/buildslave/mozilla-central/dist/include/mozilla/RefPtr.h:68:37: note: in instantiation of member function 'RefPtr<_GdkDragContext>::ConstRemovingRefPtrTraits<_GdkDragContext>::Release' requested here
13:58.76 E    68 |       ConstRemovingRefPtrTraits<T>::Release(oldPtr);
13:58.76 E       |                                     ^
13:58.76 E /build/obj/buildslave/mozilla-central/dist/include/mozilla/RefPtr.h:180:5: note: in instantiation of member function 'RefPtr<_GdkDragContext>::assign_assuming_AddRef' requested here
13:58.76 E   180 |     assign_assuming_AddRef(nullptr);
13:58.76 E       |     ^
13:58.76 E /build/buildslave/mozilla-central-portopt/build/xpcom/components/../../widget/gtk/nsDragService.h:335:44: note: in instantiation of member function 'RefPtr<_GdkDragContext>::operator=' requested here
13:58.76 E   335 |   void MarkAsActive() { mSourceDragContext = nullptr; }
13:58.76 E       |                                            ^
13:58.76 E /usr/local/include/gtk-3.0/gdk/gdktypes.h:137:16: note: forward declaration of '_GdkDragContext'
13:58.76 E   137 | typedef struct _GdkDragContext        GdkDragContext;
13:58.76 E       |                ^

i can file a regressing bug if that's preferred.

Comment 17

[Martin Stránský [:stransky] (ni? me) (PTO till June 6)]

https://searchfox.org/firefox-main/rev/e5219f2ad48f6b993c34fc25961e5900ffabfe65/widget/gtk/nsDragService.h#335
So try to move the implementation to nsDragService.cpp then. Do you mind to file the bug?

Comment 18

[Dianna Smith [:diannaS]]

Re-opening as this was reverted via bug 2022238 in main and beta

Comment 19

[Mathew Hodson]

Reverted in 149
https://github.com/mozilla-firefox/firefox/commit/224d0b1fa2b3