Skip to content

[DO NOT MERGE] GTK: Fix early exit from wxDropSource::DoDragDrop #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 35 commits into
base: coremail_stable
Choose a base branch
from

Conversation

crml2024
Copy link

@crml2024 crml2024 commented Jul 23, 2024

Missing of mouse event is not an error.

It's not very clear whether g_lastButtonNumber is needed to be correct. It is needed. So we can't not directly remove the condition.

徐扬斌 added 30 commits May 6, 2024 13:57
Since we've already had CalculateOverallSize, we should avoid duplicate the error-prone algorithm here.
It has bug found by the assertion within TryCollapseLayout in our application.
Do Not emit wxEVT_LEAVE_WINDOW when GdkEventCrossing mode is GDK_CROSSING_GRAB or GDK_CROSSING_GTK_GRAB
…p to fix CEF renderer process crash.

Begin with Chromium 97, EventLatencyTracingRecorder was added to trace the performance.
And the timestamp 0 used will lead to 'DCHECK(!dispatch_timestamp.is_null());' in function
EventLatencyTracingRecorder::RecordEventLatencyTraceEvent
see:
https://github.com/chromium/chromium/blob/109.0.5414.120/cc/metrics/event_latency_tracing_recorder.cc

Way to reproduce the crash:
Right click to popup the context-menu created by CEF.
Then the CEF renderer process will crash.

Set the timestamp to [[NSProcessInfo processInfo] systemUptime] to fix it.
… CEF.

DoDragDrop() may make CEF cease to work.
One way to reproduce the problem:
Do drag-n-drop repeatly, that is, start drag immediately after one drop,
then there're chances we can see large amount of DoDragDrop() in the callstack
of the UI-thread. And finally leads to stack overflow.
Messages processed by CEF are handled when wxEVT_IDLE is triggered.
The basic loop are:
-> Step1, cocoa call OSXDefaultModeObserverCallBack(in src/osx/core/evtloop_cf.cpp)
-> Step2, wxEVT_IDLE is triggered.
-> Step3, wxWebViewChroium::OnIdle() takes the cake.
-> Step4, CefDoMessageLoopWork() do the jobs for CEF.

If DoDragDrop() is invoked during CefDoMessageLoopWork(), and together there comes a recursive call to it,
then the previous DoDragDrop() can't return anymore, and will, in turn, prevent CefDoMessageLoopWork()
from returning to the caller. And finally overflow the stack.

And now we add the global dnd flag to stay away from this kind of tragic.
In MacOSX/cocoa, wxPopupTransientWindow needs to do mouse capture handling during OnIdle().
But it's done wrongly when decide whether the mouse is within the popup.
Not fixed with simply checking the mouse pos against the client rect of the popup.
On MacOSX, the wxWebviewWebkit will eagerly capture the hotkeys despite the fact
that it might be the one in focus. This will interfere other widgets' functions.
It should only handle the hotkeys when it is the firstResponder.

More about firstResponder:
https://developer.apple.com/documentation/appkit/nswindow/1419440-firstresponder
The wxDropSource has API for icons of different dnd states indication.
But it is not supported at all on cocoa. No icons at all on OSX.
Now add the proper support for this API.
DoDragDrop() not assert 'It must be called in response to a mouse down or drag event.'.
But when the user start DnD something from a frame rendered by CEF or similar framework,
it should be perfectly valid.
We found the bottleneck in this function.
Optimize it to speed up things and it do fix problems observed in our app.
… visible).

GTK3 defaults to enable the 'overlay-scrolling' features, that is, only show scrollbar when the mouse/touching
hover the scrollbar area. It is undesirable to many of our clients.
Yet wxWidgets does NOT exposing any API for us to do the GTK customization.
So we need to add our 'CMExtension' to workaround this problem.

This MAY be a temporary solution. We may make a new patch in the future by introducing a new properly designed
API to wxWidgets for this sort of native/per-platform customization.
The old DND reorder implementation is buggy and wrong in many ways.
This is a rework that fix almost everything with better user experiences, better dropped pos indication.
ToDo: Better explanation code comments for several main changes. I'm now too busy to leave an explanation
with enough details.
… make it a no-op, i.e. refuse the reroder.

The old behavior is to position the dragged column to the rightmost. It is counter-intuitive.
Forbid the reordering make more sense and is a de facto accepted standard.
…ocoa modal dialog.

The cocoa framework is not bug-free.
Some version(I'd called it buggy) may throw unexpected exceptions.
In order to avoid the ShowModal() to died and break the modal stack, and finally comes you
a assertion failure on broken modal stack, we should catch exceptions here.
When wxGTKWindow::SetFocus, we will bring up the toplevel window to the user before
we set the focus to the child widget.
And now, we no longer need to consider whether gtk_window_is_active(tlw).
Because of these two reasons:
1. if the gtk_window_is_active(tlw), gtk_window_preset() don't harm anyway;
2. when foreign XWindow embedded(not via GtkPlug), maybe use XWindow controlled by GDK as their parent,
   for example, a CEF browser window(which is now an XWindow) take ours as parent,
   our tlw active state maybe altered(when CEF window take the keyboard focus) and unknown to us.
   i.e. gtk_window_is_active(tlw) may still true while the focus is already taken by the CEF window.
   we need to force to bring up our tlw to the user at this moment.
Since we've already had CalculateOverallSize, we should avoid duplicate the error-prone algorithm here.
It has bug found by the assertion within TryCollapseLayout in our application.
Do Not emit wxEVT_LEAVE_WINDOW when GdkEventCrossing mode is GDK_CROSSING_GRAB or GDK_CROSSING_GTK_GRAB
…p to fix CEF renderer process crash.

Begin with Chromium 97, EventLatencyTracingRecorder was added to trace the performance.
And the timestamp 0 used will lead to 'DCHECK(!dispatch_timestamp.is_null());' in function
EventLatencyTracingRecorder::RecordEventLatencyTraceEvent
see:
https://github.com/chromium/chromium/blob/109.0.5414.120/cc/metrics/event_latency_tracing_recorder.cc

Way to reproduce the crash:
Right click to popup the context-menu created by CEF.
Then the CEF renderer process will crash.

Set the timestamp to [[NSProcessInfo processInfo] systemUptime] to fix it.
… CEF.

DoDragDrop() may make CEF cease to work.
One way to reproduce the problem:
Do drag-n-drop repeatly, that is, start drag immediately after one drop,
then there're chances we can see large amount of DoDragDrop() in the callstack
of the UI-thread. And finally leads to stack overflow.
Messages processed by CEF are handled when wxEVT_IDLE is triggered.
The basic loop are:
-> Step1, cocoa call OSXDefaultModeObserverCallBack(in src/osx/core/evtloop_cf.cpp)
-> Step2, wxEVT_IDLE is triggered.
-> Step3, wxWebViewChroium::OnIdle() takes the cake.
-> Step4, CefDoMessageLoopWork() do the jobs for CEF.

If DoDragDrop() is invoked during CefDoMessageLoopWork(), and together there comes a recursive call to it,
then the previous DoDragDrop() can't return anymore, and will, in turn, prevent CefDoMessageLoopWork()
from returning to the caller. And finally overflow the stack.

And now we add the global dnd flag to stay away from this kind of tragic.
In MacOSX/cocoa, wxPopupTransientWindow needs to do mouse capture handling during OnIdle().
But it's done wrongly when decide whether the mouse is within the popup.
Not fixed with simply checking the mouse pos against the client rect of the popup.
On MacOSX, the wxWebviewWebkit will eagerly capture the hotkeys despite the fact
that it might be the one in focus. This will interfere other widgets' functions.
It should only handle the hotkeys when it is the firstResponder.

More about firstResponder:
https://developer.apple.com/documentation/appkit/nswindow/1419440-firstresponder
The wxDropSource has API for icons of different dnd states indication.
But it is not supported at all on cocoa. No icons at all on OSX.
Now add the proper support for this API.
DoDragDrop() not assert 'It must be called in response to a mouse down or drag event.'.
But when the user start DnD something from a frame rendered by CEF or similar framework,
it should be perfectly valid.
We found the bottleneck in this function.
Optimize it to speed up things and it do fix problems observed in our app.
… visible).

GTK3 defaults to enable the 'overlay-scrolling' features, that is, only show scrollbar when the mouse/touching
hover the scrollbar area. It is undesirable to many of our clients.
Yet wxWidgets does NOT exposing any API for us to do the GTK customization.
So we need to add our 'CMExtension' to workaround this problem.

This MAY be a temporary solution. We may make a new patch in the future by introducing a new properly designed
API to wxWidgets for this sort of native/per-platform customization.
The old DND reorder implementation is buggy and wrong in many ways.
This is a rework that fix almost everything with better user experiences, better dropped pos indication.
ToDo: Better explanation code comments for several main changes. I'm now too busy to leave an explanation
with enough details.
… make it a no-op, i.e. refuse the reroder.

The old behavior is to position the dragged column to the rightmost. It is counter-intuitive.
Forbid the reordering make more sense and is a de facto accepted standard.
徐扬斌 and others added 5 commits July 3, 2024 18:47
…ocoa modal dialog.

The cocoa framework is not bug-free.
Some version(I'd called it buggy) may throw unexpected exceptions.
In order to avoid the ShowModal() to died and break the modal stack, and finally comes you
a assertion failure on broken modal stack, we should catch exceptions here.
When wxGTKWindow::SetFocus, we will bring up the toplevel window to the user before
we set the focus to the child widget.
And now, we no longer need to consider whether gtk_window_is_active(tlw).
Because of these two reasons:
1. if the gtk_window_is_active(tlw), gtk_window_preset() don't harm anyway;
2. when foreign XWindow embedded(not via GtkPlug), maybe use XWindow controlled by GDK as their parent,
   for example, a CEF browser window(which is now an XWindow) take ours as parent,
   our tlw active state maybe altered(when CEF window take the keyboard focus) and unknown to us.
   i.e. gtk_window_is_active(tlw) may still true while the focus is already taken by the CEF window.
   we need to force to bring up our tlw to the user at this moment.
DO NOT replace the global GDK event handler with our 'wxgtk_main_do_event'.
Because this trick rely on one uncertain assumption:
No one besides us, had done gdk_event_handler_set() already.
In most case, this might be true.
But a single exception can destroy this trick completely.
For example, when we want to embed CEF, Chromium will use a custom gdk_event_handler too.
This trick will render all CEF browser window/view no longer usable.
GTK3 should be blamed for not offering any gdk_event_handler_get() or similar stuff for a safe trick here.
In GTK4, there's a more sane way to get the job done.
So let's do it in a more conservative way.
If there're GdkEvents, we handle them via 'wxgtk_main_do_event',
all other events should be handle by one gtk_main_iteration().
I'm not sure whether this is really okay, but it seems a nicer and less intrusive way to do things.
Missing of mouse event is not an error.
It's not very clear whether g_lastButtonNumber is needed to be correct.
@crml2024 crml2024 marked this pull request as draft July 23, 2024 13:42
@crml2024 crml2024 changed the title GTK: Fix early exit from wxDropSource::DoDragDrop [DO NOT MERGE] GTK: Fix early exit from wxDropSource::DoDragDrop Jul 24, 2024
@JCYang JCYang force-pushed the coremail_stable branch from 426ccd6 to 7da1c0f Compare April 9, 2025 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants