[4.20] 'd_shadow_box_proc' crashes 'popup_dialog'
Dennis

I think I've found a bug.

[4.20] 'd_shadow_box_proc' crashes 'popup_dialog'
under certain circumstances (details below code window):

"popup_crash.c"

1// small app to demonstrate allegro 4.20 crashing on "popup_dialog"
2// when the programme was linked to the debug version of the allegro library
3// and when there is a "d_shadow_box_proc" whose w and h are set to 0
4 
5#include "allegro.h"
6 
7DIALOG test_popup[] =
8{
9 { d_shadow_box_proc, 0,0, 320, 200, 0,0,0,0,0,0,NULL,NULL,NULL },
10 { d_button_proc, 10,10,60,20,0,0,13,D_EXIT,0,0,"&OK 1",NULL,NULL},
11 { NULL, 0,0,0,0,0,0,0,0,0,0,NULL,NULL,NULL}
12};
13 
14DIALOG test_popup_crash[] =
15{
16 { d_shadow_box_proc, 0,0,0,0,0,0,0,0,0,0,NULL,NULL,NULL },
17 { d_button_proc, 10,10,60,20,0,0,13,D_EXIT,0,0,"&OK 2",NULL,NULL},
18 { NULL, 0,0,0,0,0,0,0,0,0,0,NULL,NULL,NULL}
19};
20 
21 
22int main(int argc, char **argv)
23{
24 allegro_init();
25 install_keyboard();
26 set_color_depth(32);
27 set_gfx_mode(GFX_AUTODETECT_WINDOWED, 640, 480, 0, 0);
28 install_mouse();
29 set_dialog_color(test_popup,makecol(0,0,0),makecol(255,255,255));
30 set_dialog_color(test_popup_crash,makecol(0,0,0),makecol(255,255,255));
31 popup_dialog(test_popup,0);
32 popup_dialog(test_popup_crash,0);
33 allegro_exit();
34 return 0;
35}
36END_OF_MAIN()

I compiled this using mingw32 version 3.4.2

gcc -O2 -Wall -c popup_crash.c
gcc -o popup.exe *.o -mwindows -lalleg
gcc -o popupd.exe *.o -mwindwos -lalld

So popup.exe was linked to the release version of the Allegro 4.20 library, while popupd.exe was linked to the debug version.

Now what this should demonstrate is that running popup.exe will show two popup dialogs one after another and then exit normally.

popupd.exe should crash on the second popup, which I believe is directly related to the d_shadow_box_proc, which dimensions are 0 in that second popup dialog.

I get the same behaviour when I compile everything with MSVC 2005,(crash in debug version, no crash in release version) so I believe this is a bug in Allegro.

not necessarily important background info: (I found this on investigating a crash in one of my apps that never happened in the release build and always in the debug build. The reason why my d_shadow_box_proc s are per default dimensioned as 0, was simply that i'm using a custom d_stretch_dialog_proc as the second last object in each of my dialogs to adjust the dimensions of the first object to always be large enough to include all other objects.)

Kitty Cat
Quote:

popupd.exe should crash on the second popup, which I believe is directly related to the d_shadow_box_proc, which dimensions are 0 in that second popup dialog

The "crash" is actually a failed ASSERT. Under Unix, you'd get a nice "Aborted" message in the console (as opposed to Segment Violation, Illegal Instruction, or whatever else). Trying to create a 0-sized bitmap will trigger an ASSERT which will immediately SIGABRT the program.

Dennis

Well windows says "Runtime Error! Application terminated in an unusual way." and I thought that's the same as a crash.
>I< am not trying to create the 0 sized bitmap in this case.
create_bitmap(0,0) is called from popup_dialog in this special above mentioned case.
Why it doesn't terminate abnormally in the release version is beyond my scope.

There is another thing that should get corrected in popup dialog though.
That 'bmp' in there is always created with the dimensions of the first dialog object, which leads to wrong behaviour if there are any self-stretching objects(like in my application) that change their dimensions on dialog initialization(when they receive the message MSG_START).
Therefore popup_dialog might want to send that MSG_START message to the dialog, before creating that 'bmp' to lead to correct behaviour.

Of course I could send that message myself before calling popup_dialog.
(and I will do that to prevent the error in my app for now..)
I think that popup_dialog should be responsible for that though.

for viewing convenience, here's a copy of 'popup_dialog' in src/gui.proc

1int popup_dialog(DIALOG *dialog, int focus_obj)
2{
3 BITMAP *bmp;
4 BITMAP *gui_bmp;
5 int ret;
6 ASSERT(dialog);
7 
8 bmp = create_bitmap(dialog->w, dialog->h);
9 gui_bmp = gui_get_screen();
10 if (bmp) {
11 scare_mouse_area(dialog->x, dialog->y, dialog->w, dialog->h);
12 blit(gui_bmp, bmp, dialog->x, dialog->y, 0, 0, dialog->w, dialog->h);
13 unscare_mouse();
14 }
15 else
16 *allegro_errno = ENOMEM;
17 
18 ret = do_dialog(dialog, focus_obj);
19 
20 if (bmp) {
21 scare_mouse_area(dialog->x, dialog->y, dialog->w, dialog->h);
22 blit(bmp, gui_bmp, 0, 0, dialog->x, dialog->y, dialog->w, dialog->h);
23 unscare_mouse();
24 destroy_bitmap(bmp);
25 }
26 
27 return ret;
28}

Kitty Cat
Quote:

for viewing convenience, here's a copy of 'popup_dialog' in src/gui.proc

And as you can see, there's no real way for it to safely send the MSG_START message itself without re-implmenting the dialog player loop.

TFM said:

Like do_dialog(), but it stores the data on the screen before  drawing  the  dialog
and  restores  it when the dialog is closed. The screen area to be stored is calcu-
lated from the dimensions of the first object in  the  dialog,  so  all  the  other
objects should lie within this one.

Double check the last sentence. ;)

Dennis
Quote:

Quote:

Like do_dialog(), but it stores the data on the screen before drawing the dialog
and restores it when the dialog is closed. The screen area to be stored is calcu-
lated from the dimensions of the first object in the dialog, so all the other
objects should lie within this one.

Double check the last sentence.

"...before drawing the dialog..." yes.
"...so all other objects should lie within this one..." that's why I have self-stretching dialog objects that stretch on dialog initialization, because I relied on:

The docs for MSG_START said:

"Tells the object to initialise itself. The dialog manager sends this to all objects in the dialog just before it displays the dialog."

Quote:

And as you can see, there's no real way for it to safely send the MSG_START message itself without re-implmenting the dialog player loop.

What would be wrong about adding a single line, in front of 'bmp = create_..' that says something like if(dialog!=NULL) dialog_message(dialog,MSG_START,...);?
Would that be unsafe in any way?

Kitty Cat

A dialog object should not receive MSG_START multiple times in a row. More often than not, MSG_START will cause some objects to allocate memory, so multiple start messages will cause memory leaks. The only way to get around that would be to call a MSG_CLOSE right after, and that's.. ugly (and could just as easilly negate the resize).

Evert
Quote:

The dialog manager sends this to all objects in the dialog just before it displays the dialog.

Which is what it does. What it does not do is send MSG_START again if it redraws the dialog.
That said, it's fairly easy to implement your own version of pop_dialog() or do_dialog() that deals with this.

Quote:

Why it doesn't terminate abnormally in the release version is beyond my scope.

The error isn't caught. In my experience, 0-sized bitmaps don't (always) generate a crash; they're still not allowed though.

Quote:

What would be wrong about adding a single line, in front of 'bmp = create_..' that says something like if(dialog!=NULL) dialog_message(dialog,MSG_START,...);?

What would be wrong is that do_dialog() already does that. As I said, you can easily write your own wrapper that does more advanced stuff if youwant to do that, but doing this from within Allegro's function changes the behavior of the function, which might break existing programmes.

Ron Novy
Kitty Cat said:

The only way to get around that would be to call a MSG_CLOSE right after, and that's.. ugly (and could just as easilly negate the resize).

I believe it's actually MSG_END and not MSG_CLOSE.

If you are not happy with the way popup_dialog is working then the best thing to do is copy the popup_dialog function out of 'gui.c', and any relevant static functions into a new file and add it to your project. Just remember to change the name of the function and then you can modify it as much as you like without affecting the library itself. All the d_xxx_proc objects rely on the current functionality so the wrong change could mean more crashes. You could also create a dummy object at the start of the dialog with a default size but you would probably be better off using the allegro popup_dialog function as it was intended.

Dennis
Quote:

Which is what it does. What it does not do is send MSG_START again if it redraws the dialog.

I did not think it would do that before every redraw. I'm not having an issue with that. I just assumed that MSG_START is send at least once, before the dialog is displayed for the first time.

I still think that popup_dialog is not functioning properly, let me explain:

popup_dialog saves a portion of the bitmap the dialog is to be drawn onto before running the dialog(via do_dialog) to later restore this portion.

So popup_dialog should be conceptually related to anything that's related to displaying the dialog correctly and to clean up (restoring) the display area underneath it after running it.

If a dialog object does stretch itself, because it assumes that (as mentioned in the manual) it gets a one time chance to initialize itself before the dialog is displayed for the first time then the saving of that screen portion should respect that, because the dimensions of an object are directly display behaviour related and the saving of a bitmap portion to later restore it, is also directly display behaviour related.

I do understand now by your arguments that sending MSG_START twice could break stuff, but that doesn't change my mind that popup_dialog is behaving wrong, because of the aforementioned logic(display behaviour) which I think is a valid point.
popup_dialog should just send MSG_START once then and not make use of do_dialog to prevent sending it twice.
I guess this could be done by just copying the do_dialog code into popup_dialog and then adding the bmp creation and restoring in that copy, after MSG_START was send once to the dialog.

The easiest way out of the issue for me, as suggested by KittyCat on IRC, is to not use popup_dialog and instead use do_dialog and just redrawing the previous dialog below it after it exits. The current implementation of popup_dialog feels obsolete to me then, because it does not respect objects that stretch on initialization of a dialog and thus can't save and restore the used display space properly.:-/
(Stretching objects is not too uncommon I think. Just think of objects that stretch to fit the text inside them(in turn related to font size).)

I agree though that I have to write my own function now or use do_dialog. I would just appreciate it, if my arguments were at least heard and understood right.

Ron Novy

You could use do_dialog and write an object proc to use at the top of a dialog that would calculate the stretch and size of the dialog and save the background (if needed) at MSG_START and draw the shadow_box. Store the bmp pointer in dp... Since its the first object and contains all other objects it could handle the stretching and only save/restore the necessary parts of the screen.... I'm sure lex gui does something like that.

Dennis

That would be hackish. I prefer to write a proper implementation of popup_dialog instead, which is easier because, as you said, all the code is already there. I just need to copy and modify it slightly.:)
I'm not actually asking for advice how to fix my code, I'm more like trying to convince you guys that the current implementation of popup_dialog is logically faulty with respect to the specific display-behaviour(proper saving/restoring of a display area) which it is supposed to provide.

Evert

I disagree.
Resizable widgets are something the Allegro GUI can't do, popup_dialog() is consistent with that. Neither of Allegro's dialog functions deals with moving dialogs or multiple dialogs on screen either, but those are fairly straightforward to add on top of the existing functions.
The behavior also isn't inconsistent with the documentation, so it doesn't qualify as a bug either.

That said, it's possible to work around all of these issues without too much trouble. You run into the same issue if you try to implement moving dialogs. I worked around that by creating a custom dialog object, at the start of the dialog, that does the double buffer itself. It's what I'd recommend you to do. I should note that I use neither do_dialog() nor popup_dialog() because I want to continue running the logic loop while the dialog is displayed, which in a way bypasses the issue.

Ron Novy

Not that this would help but I've been working on a non-blocking windowing system for allegro that uses modified versions of the dialog procedures and uses an AL_DESKTOP structure to store all the global menu, window and dialog stuff. It's pretty cool because it uses the allegro style objects (slightly modified) so it would be easy to create a window template but it's no where near completion. If anyone has any ideas or info on how coding a windowing gui let me know but I'm out for the day so I'm marking this thread just in case.

Dennis
Quote:

Resizable widgets are something the Allegro GUI can't do, popup_dialog() is consistent with that.

But the API allows and encourages having custom dialog objects, so it should at least respect that some objects might want to do some initialization that affects displaying behaviour, which is why I think popup_dialog is not sufficiently advanced.
It's true that the documentation says that popup_dialog uses the dimensions of the first object to do the saving/restoring, but it doesn't say that it will use the dimensions that are in there, before the object actually got a chance to properly initialize itself.

Quote:

The behavior also isn't inconsistent with the documentation, so it doesn't qualify as a bug either.

The initial thing I thought was a bug wasn't the behaviour, but the crash(or failed ASSERT) that I know now is because of that implicit create_bitmap(0,0) call.
I would consider that a bug, because it is a "show stopper" to the calling programme and the programme doesn't get a chance to shutdown properly if that happens.

The behaviour of popup_dialog however, even if it doesn't crash, I would also not call a bug, but a wrong(or insufficient) implementation of a desired behaviour.
If you still can't agree to that, then I hope you can at least agree that the documentation of popup_dialog should probably mention that the objects don't get a chance to initialize themselves properly, before the dimensions of the first object are being taken for the saving and restoring of the overdrawn bitmap area.
That should be added to the documentation.
Another option would be, to add to the documentation of MSG_START:
In case of popup_dialog, this message will be send to the objects, only AFTER popup_dialog has used the probably not initialized dimensions of the first object to manage the saving and restoring of the affected display area.

Both changes to the documentation will look very ugly and hilarious of course, so a proper implementation of popup_dialog is preferred.
Note that I'm not trying to irritate you or to spoil the documentation or even Allegro. I'm only interested in making it actually better and I think with a rewrite of popup_dialog(that does not send MSG_START twice and does not rely on do_dialog), this could be done without breaking existing programmes.
(Under that light, your statement "That said, it's possible to work around all of these issues without too much trouble." actually also applies to changing the popup_dialog function.)

Quote:

[..]It's what I'd recommend you to do. I should note that I use neither do_dialog() nor popup_dialog() because I want to continue running the logic loop while the dialog is displayed, which in a way bypasses the issue.

Thanks for your suggestions. I'm using popup_dialog only for very few sub dialogs that require to be answered before the programme can go on. I think I can live with making them use do_dialog instead. For my advanced main dialog that used lots of customized objects and behaviours, I am already using a customized dialog runner.

Evert
Quote:

But the API allows and encourages having custom dialog objects, so it should at least respect that some objects might want to do some initialization that affects displaying behaviour

You can, there's no problem with the GUI code per se; but it's impossible to foresee all possible things a user might want to do in a function like popup_dialog(). popup_dialog() stumbles on dialogs that do not have the size that they are declared with, or are drawn to a different location. It's just not designed to handle those situations.

Quote:

It's true that the documentation says that popup_dialog uses the dimensions of the first object to do the saving/restoring, but it doesn't say that it will use the dimensions that are in there, before the object actually got a chance to properly initialize itself.

Right, so the order is undefined by the documentation.

Quote:

I hope you can at least agree that the documentation of popup_dialog should probably mention that the objects don't get a chance to initialize themselves properly, before the dimensions of the first object are being taken for the saving and restoring of the overdrawn bitmap area.
That should be added to the documentation.
Another option would be, to add to the documentation of MSG_START:
In case of popup_dialog, this message will be send to the objects, only AFTER popup_dialog has used the probably not initialized dimensions of the first object to manage the saving and restoring of the affected display area.

Both changes to the documentation will look very ugly and hilarious of course,

The latter certainly, the former depends on how you word it.
I would alter the documentation to something like:

Quote:

Like do_dialog(), but it stores the data on the screen before drawing
the dialog and restores it when the dialog is closed. The screen area
to be stored is calculated from the dimensions of the first object in
the dialog, so all the other objects should lie within this one.
Note that this is done before the dialog is initialised.

Quote:

so a proper implementation of popup_dialog is preferred.

I maintain that the current implementation is fine: it does what it's supposed to do.

Quote:

Note that I'm not trying to irritate you

Good. ;) You're not, you know.

Quote:

I'm only interested in making it actually better and I think with a rewrite of popup_dialog(that does not send MSG_START twice and does not rely on do_dialog), this could be done without breaking existing programmes.

Probably. However, it will duplicate a lot of code and it means possible fixes to do_dialog() would need to be applied to both functions, making it harder to maintain.

Quote:

Under that light, your statement "That said, it's possible to work around all of these issues without too much trouble." actually also applies to changing the popup_dialog function.

You can do it without changing popup_dialog() by using a custom dialog object that handles the creation and maintenance of the backbuffer.

Dennis
Quote:

Probably. However, it will duplicate a lot of code and it means possible fixes to do_dialog() would need to be applied to both functions, making it harder to maintain.

It could be done without duplicating code and thus the point of harder maintainability becomes void.
e.g. by renaming the current do_dialog function to do_dialog_ex and adding a new parameter bool handle_display_save_restore.
Inserting the bmp creation and restoring into there, then making do_dialog use do_dialog_ex with that parameter set to false and popup_dialog using it with the parameter set to true.

I've prepared a little something. (untested but if my eyes don't fool me, calling that new do_dialog_ex function with the third parameter as false will not change the behaviour of the current do_dialog at all, while calling it with that parameter as true will be nearly the same behaviour as the current popup_dialog(with the improvement that it respects initialization of the dialog prior to saving the display area))

all functions taken from src\gui.c of Allegro 4.20:
following code box includes the functions
do_dialog_ex
do_dialog and
popup_dialog
as described above.

1 /* do_dialog_ex: // modified do_dialog
2 * The basic dialog manager. The list of dialog objects should be
3 * terminated by one with a null dialog procedure. Returns the index of
4 * the object which caused it to exit.
5 * New parameter:
6 * The third parameter allows to control automatic saving and restoring of
7 * the underlying display area.
8 */
9int do_dialog_ex(DIALOG *dialog, int focus_obj, bool handle_display_save_restore)
10{
11 BITMAP *bmp;
12 BITMAP *mouse_screen = _mouse_screen;
13 BITMAP *gui_bmp = gui_get_screen();
14 int screen_count = _gfx_mode_set_count;
15 int ret;
16 void *player;
17 ASSERT(dialog);
18 
19 if (!is_same_bitmap(_mouse_screen, gui_bmp) && !(gfx_capabilities&GFX_HW_CURSOR))
20 show_mouse(gui_bmp);
21 
22 player = init_dialog(dialog, focus_obj);
23 
24 // Save display area (after initializing the dialog)
25 if(handle_display_save_restore) {
26 bmp = create_bitmap(dialog->w, dialog->h);
27 gui_bmp = gui_get_screen();
28 if (bmp) {
29 scare_mouse_area(dialog->x, dialog->y, dialog->w, dialog->h);
30 blit(gui_bmp, bmp, dialog->x, dialog->y, 0, 0, dialog->w, dialog->h);
31 unscare_mouse();
32 }
33 else
34 *allegro_errno = ENOMEM;
35 }
36 
37 while (update_dialog(player)) {
38 /* If a menu is active, we yield here, since the dialog
39 * engine is shut down so no user code can be running.
40 */
41 if (active_menu_player)
42 rest(1);
43 }
44 
45 if (_gfx_mode_set_count == screen_count && !(gfx_capabilities&GFX_HW_CURSOR))
46 show_mouse(mouse_screen);
47 
48 ret = shutdown_dialog(player);
49 
50 // Restore display area if necessary
51 if(handle_display_save_restore) {
52 if (bmp) {
53 scare_mouse_area(dialog->x, dialog->y, dialog->w, dialog->h);
54 blit(bmp, gui_bmp, 0, 0, dialog->x, dialog->y, dialog->w, dialog->h);
55 unscare_mouse();
56 destroy_bitmap(bmp);
57 }
58 }
59 
60 return ret;
61}
62 
63 
64/* do_dialog:
65 * new implementation of previous do_dialog, using do_dialog_ex
66*/
67int do_dialog(DIALOG *dialog, int focus_obj)
68{
69 return do_dialog_ex(dialog,focus_obj,false);
70}
71 
72 
73/* popup_dialog:
74 * new implementation of previous popup_dialog, using do_dialog_ex
75*/
76int popup_dialog(DIALOG *dialog, int focus_obj)
77{
78 return do_dialog_ex(dialog,focus_obj,true);
79}

Thread #585681. Printed from Allegro.cc