Skip to content
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

testtray: removing a tray item makes the test executable crash #12002

Closed
madebr opened this issue Jan 17, 2025 · 6 comments · Fixed by #12003
Closed

testtray: removing a tray item makes the test executable crash #12002

madebr opened this issue Jan 17, 2025 · 6 comments · Fixed by #12003
Milestone

Comments

@madebr
Copy link
Contributor

madebr commented Jan 17, 2025

I can easily crash testtray on Windows 10 by removing a tray item.

Crashing scenario 1:
  1. Run testtray
  2. Click on "Tray control menu" -> "Create Checkbox"
  3. Click on "Tray control menu" -> "New Checkbox" -> "Remove"
  4. Segmentation fault
    SDL_RemoveTrayEntry_REAL(SDL_TrayEntry *) SDL_tray.c:344
    SDL_RemoveTrayEntry(SDL_TrayEntry *) SDL_dynapi_procs.h:1252
    remove_entry(void *, SDL_TrayEntry *) testtray.c:97
    TrayWindowProc(HWND__ *, unsigned int, unsigned long long, long long) SDL_tray.c:130
    <unknown> 0x00007ffcc791ef5c
    <unknown> 0x00007ffcc791e684
    WIN_PumpEvents(SDL_VideoDevice *) SDL_windowsevents.c:2409
    SDL_PumpEventsInternal(bool) SDL_events.c:1376
    SDL_WaitEventTimeout_Device(SDL_VideoDevice *, SDL_Window *, SDL_Event *, unsigned long long, long long) SDL_events.c:1471
    SDL_WaitEventTimeoutNS(SDL_Event *, long long) SDL_events.c:1634
    SDL_WaitEvent_REAL(SDL_Event *) SDL_events.c:1542
    SDL_WaitEvent(SDL_Event *) SDL_dynapi_procs.h:1008
    SDL_main(int, char **) testtray.c:616
    SDL_RunApp_REAL(int, char **, int (*)(int, char **), void *) SDL_sysmain_runapp.c:88
    SDL_RunApp_DEFAULT(int, char **, int (*)(int, char **), void *) SDL_dynapi_procs.h:804
    SDL_RunApp(int, char **, int (*)(int, char **), void *) SDL_dynapi_procs.h:804
    main(int, char **) SDL_main_impl.h:103
    invoke_main() 0x00007ff7a4291af9
    __scrt_common_main_seh() 0x00007ff7a429199e
    __scrt_common_main() 0x00007ff7a429185e
    mainCRTStartup(void *) 0x00007ff7a4291b8e
    <unknown> 0x00007ffcc6957374
    <unknown> 0x00007ffcc7ebcc91
    
Crashing scenario 2:
  1. Run testtray

  2. Click on "Tray control menu" -> "Create Button"

  3. Click on "Tray control menu" -> "New Button" -> "Remove"

    The following message is printed on the terminal:

    Attempt to remove a menu that isn't a submenu. This shouldn't happen.

  4. Click on "Tray control menu" -> "New Button" -> "Remove" (try to remove item 2nd time)

  5. Segmentation fault

    try_realloc_chunk(malloc_state *, malloc_chunk *, unsigned long long, int) SDL_malloc.c:4853
    dlrealloc(void *, unsigned long long) SDL_malloc.c:5248
    SDL_realloc_REAL(void *, unsigned long long) SDL_malloc.c:6489
    SDL_RemoveTrayEntry_REAL(SDL_TrayEntry *) SDL_tray.c:362
    SDL_RemoveTrayEntry(SDL_TrayEntry *) SDL_dynapi_procs.h:1252
    remove_entry(void *, SDL_TrayEntry *) testtray.c:87
    TrayWindowProc(HWND__ *, unsigned int, unsigned long long, long long) SDL_tray.c:130
    <unknown> 0x00007ffcc791ef5c
    <unknown> 0x00007ffcc791e684
    WIN_PumpEvents(SDL_VideoDevice *) SDL_windowsevents.c:2409
    SDL_PumpEventsInternal(bool) SDL_events.c:1376
    SDL_WaitEventTimeout_Device(SDL_VideoDevice *, SDL_Window *, SDL_Event *, unsigned long long, long long) SDL_events.c:1471
    SDL_WaitEventTimeoutNS(SDL_Event *, long long) SDL_events.c:1634
    SDL_WaitEvent_REAL(SDL_Event *) SDL_events.c:1542
    SDL_WaitEvent(SDL_Event *) SDL_dynapi_procs.h:1008
    SDL_main(int, char **) testtray.c:616
    SDL_RunApp_REAL(int, char **, int (*)(int, char **), void *) SDL_sysmain_runapp.c:88
    SDL_RunApp_DEFAULT(int, char **, int (*)(int, char **), void *) SDL_dynapi_procs.h:804
    SDL_RunApp(int, char **, int (*)(int, char **), void *) SDL_dynapi_procs.h:804
    main(int, char **) SDL_main_impl.h:103
    invoke_main() 0x00007ff7a4291af9
    __scrt_common_main_seh() 0x00007ff7a429199e
    __scrt_common_main() 0x00007ff7a429185e
    mainCRTStartup(void *) 0x00007ff7a4291b8e
    <unknown> 0x00007ffcc6957374
    <unknown> 0x00007ffcc7ebcc91
    
@madebr madebr changed the title testtray: removing a tray item does not make it disappear from the menu testtray: removing a tray item makes the test executable crash Jan 17, 2025
@slouken slouken added this to the 3.2.0 milestone Jan 17, 2025
@slouken
Copy link
Collaborator

slouken commented Jan 17, 2025

@Semphriss, can you take a look?

@Semphriss
Copy link
Contributor

Turns out I had forgotten to initialize a variable. Specifically, the one that stores the parent entry of a submenu. I'll push a fix in a few moments.

@madebr
Copy link
Contributor Author

madebr commented Jan 18, 2025

testtray behaves as expected now on Windows 10.
Very nice feature!

I had to apply this patch to satisfy --trackmem:

--- a/test/testtray.c
+++ b/test/testtray.c
@@ -638,6 +638,7 @@ clean_window:
     }

 quit:
+    SDL_free(trays);
     SDL_Quit();
     SDLTest_CommonDestroyState(state);

Is this ok to apply @Semphriss ?

Nevermind, no. I get a failure when closing the tray first and then the window.

@Semphriss
Copy link
Contributor

You're right, I forgot to free it at the end. Normally, I can just add:

    if (!trays_destroyed) {
        SDL_free(tray);
    }

... right before the clean_all label (to mirror when trays is initialized) and it should be good. I'll make a PR when I've got some time, or you can open it if you'd like.

@madebr
Copy link
Contributor Author

madebr commented Jan 18, 2025

Suggested patch, using a custom event.

Use a custom event
--- a/test/testtray.c
+++ b/test/testtray.c
@@ -2,6 +2,8 @@
 #include <SDL3/SDL_main.h>
 #include <SDL3/SDL_test.h>
 
+static Uint32 close_tray_event;
+
 static void SDLCALL tray_quit(void *ptr, SDL_TrayEntry *entry)
 {
     SDL_Event e;
@@ -9,18 +11,11 @@ static void SDLCALL tray_quit(void *ptr, SDL_TrayEntry *entry)
     SDL_PushEvent(&e);
 }
 
-static bool trays_destroyed = false;
-
 static void SDLCALL tray_close(void *ptr, SDL_TrayEntry *entry)
 {
-    SDL_Tray **trays = (SDL_Tray **) ptr;
-
-    trays_destroyed = true;
-
-    SDL_DestroyTray(trays[0]);
-    SDL_DestroyTray(trays[1]);
-
-    SDL_free(trays);
+    SDL_Event e;
+    e.type = close_tray_event;
+    SDL_PushEvent(&e);
 }
 
 static void SDLCALL apply_icon(void *ptr, const char * const *filelist, int filter)
@@ -49,7 +44,7 @@ static void SDLCALL change_icon(void *ptr, SDL_TrayEntry *entry)
         { "All files", "*" },
     };
 
-    SDL_ShowOpenFileDialog(apply_icon, ptr, NULL, filters, 2, NULL, 0);
+    SDL_ShowOpenFileDialog(apply_icon, ptr, NULL, filters, SDL_arraysize(filters), NULL, 0);
 }
 
 static void SDLCALL print_entry(void *ptr, SDL_TrayEntry *entry)
@@ -107,6 +102,8 @@ static void SDLCALL append_button_to(void *ptr, SDL_TrayEntry *entry)
     SDL_TrayEntry *new_ctrl_disabled;
     SDL_TrayEntry *new_example;
 
+    close_tray_event = SDL_RegisterEvents(1);
+
     new_ctrl = SDL_InsertTrayEntryAt(SDL_GetTrayEntryParent(entry), -1, "New button", SDL_TRAYENTRY_SUBMENU);
 
     if (!new_ctrl) {
@@ -569,17 +566,8 @@ int main(int argc, char **argv)
     SDL_TrayEntry *entry_close = SDL_InsertTrayEntryAt(menu, -1, "Close", SDL_TRAYENTRY_BUTTON);
     CHECK(entry_close);
 
-    /* TODO: Track memory! */
-    SDL_Tray **trays = SDL_malloc(sizeof(SDL_Tray *) * 2);
-    if (!trays) {
-        goto clean_all;
-    }
-
-    trays[0] = tray;
-    trays[1] = tray2;
-
     SDL_SetTrayEntryCallback(entry_quit, tray_quit, NULL);
-    SDL_SetTrayEntryCallback(entry_close, tray_close, trays);
+    SDL_SetTrayEntryCallback(entry_close, tray_close, NULL);
 
     SDL_InsertTrayEntryAt(menu, -1, NULL, 0);
 
@@ -619,18 +607,18 @@ int main(int argc, char **argv)
         } else if (e.type == SDL_EVENT_WINDOW_CLOSE_REQUESTED) {
             SDL_DestroyWindow(w);
             w = NULL;
+        } else if (e.type == close_tray_event) {
+            SDL_DestroyTray(tray);
+            SDL_DestroyTray(tray2);
+            tray = NULL;
+            tray2 = NULL;
         }
     }
 
 clean_all:
-    if (!trays_destroyed) {
-        SDL_DestroyTray(tray2);
-    }
-
+    SDL_DestroyTray(tray2);
 clean_tray1:
-    if (!trays_destroyed) {
-        SDL_DestroyTray(tray);
-    }
+    SDL_DestroyTray(tray);
 
 clean_window:
     if (w) {

@Semphriss
Copy link
Contributor

I've tried your patch but I'm getting occasional segfaults when closing the tray; I'll look into it sometime later tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants