#52 OS4: System freeze when selecting a mui window when dopus5 is running
I get a system freeze when activating a mui window while dopus5 is running (i have dopus set to clone wb), this only happends if i use latest mui 2015R1.
I have already contacted Thore and he thinks it's a dopus5 problem. There is what he say:
QUOTE:
After a look at the DOpus5 source I think I found the reason for the freeze.
On the one hand I did this change to MUI:
2014-11-04 Thore Böckelmann <tboeckel@gmx.de>
* Window.c: integrated support of the 3rd party MuiWheel custom class to let
that darn FreeWheel patch ignore all MUI windows. Unfortunately FreeWheel
feeds too many input events to the system upon using the wheel which then
in turn get misinterpreted by MUI as wheel AND cursor key events and for
example cause all list objects to scroll and change the active item in one
step. This is definitely wrong and unintuitive.
On the other hand this is the code of DOpus' L_GetWindowID() function where you log points to:
ULONG LIBFUNC L_GetWindowID(REG(a0, struct Window *window))
{
WindowID *id;
// Valid window?
if (!window) return WINDOW_UNKNOWN;
// Get ID pointer
if (!(id=(WindowID *)window->UserData) || id<(WindowID *)0x1000 || TypeOfMem(id)==0)
return WINDOW_UNKNOWN;
// Check ID is valid
if (id->magic!=WINDOW_MAGIC || id->window!=window) return WINDOW_UNKNOWN;
// Return ID code
return id->window_id;
}
DOpus5 falsely assumes that it is allowed to read and interpret the UserData field of any window, even of ones it did not open itself. This is definitely a bug in DOpus and must be fixed there. You might argue that the support for FreeWheel is relevant for AmigaOS3 and could be left out for AmigaOS4, but even then DOpus is still buggy. An application can set the UserData field to anything it likes, be it valid pointers, prime numbers, your birthday or a clingon opera. That's why this field is named "UserData" and not "PublicDataForAnybody".
@Mikael
Did you test mui versions as Thore point out ? I mean "Feel free to check r4123 and r4124. r4123 will work while r4124 will cause the freeze.". Is it indeed true ?
ps. i also edited your first post a bit, to put there all information Thore give you.
Thank you for fixing up the post, it looks much better now :)
I didn't test r4124, i can do it tonight.
But i did test r4123 & r4125.
r4123 works, r4125 freezes the system so it seems likely that Thore is right and r4124 will give a freeze.
confirmed, r4124 gives a freeze
So Thore right then. Will try to contact Xenic maybe he can deal with.
@kas1e
Thore may be right that Dopus5 shouldn't be accessing window structures that it doesn't own but I'm wondering what MUI is doing with the window structure Userdata element. I ran a quick test and all the MUI programs I tried had the same value in the UserData element. The UserData is a pointer for programs to use for their windows. Normally the UserData element would be empty (NULL) or contain a pointer to program supplied data. MUI shouldn't be putting anything in UserData unless a program requested it.
When a foreign window is activated on a screen, Intuition changes the text in the screen dragbar. Dopus5 can't update the text in the screen dragbar unless one of of it's windows is the active window, so Dopus5 checks the window structure UserData element to determine if it can update the screen dragbar with the memory & time text. I don't see any other way for Dopus5 to identify it's windows. Unless someone can suggest another way for Dopus5 to identify it's windows, there is nothing I can do.
@xenic
Accessing foreign window structures is not the bad thing. But blindly treating foreign UserData fields as pointers definitely is bad and illegal. As I said in MUI ticket #177 already the UserData field might be set to anything. And even if the value is not a low range number and TypeOfMem() thinks the value might be valid address.
It is true that MUI sets the UserData field of all its window to 0x20000929 to let FreeWheel ignore the window. This is no value that I came up with myself, but this is the one that FreeWheel checks for. Furthermore an application should make no assumptions about the default contents of the UserData field, although it is allowed to store any private data there. In this case the FreeWheel patch of course might behave wrong again. MUI does in no way expect the set value 0x20000929 to stay there forever and does not even check that.
What about keeping a list or hash of all windows opened by DOpus. GetWindowID() would then just search the given window in that list/hash and only if it is found there it can be really sure that the UserData field points to the stuff it expects. Closing the window will remove the window from the list/hash so even if another application opens a window with the same address this will cause no harm.
From my point of view a hash would be the best solution here, because lookups can be done in almost O(1) instead of O(n) for a list. Hence the additional search to make sure you are really dealing with your own window becomes neglectable. This can replace all other checks in GetWindowID().
As I said above already, the UserData field of foreign windows can be anything. And even if it looks like a valid pointer it still can be something completely different. Face it, you are blindly accessing something of which you have no idea what it is. Better make sure that you really deal with your own windows. Then you are in safe waters.
@Thore
I think I've found a way to fix the problem for the OS4 binaries. The WA_WindowName tag for windows is defined in OS3 includes but listed as not implemented. It is implemented in OS4 and I'm adding a name to all Dopus5 windows and then checking for that name before accessing the window's UserData field. I haven't completed the fix yet but it should be done in a day or 2. I don't know if the problem affects OS3 binaries on a classic Amiga or not but my fix won't work for OS3 because of the unimplemented WA_WindowName. As a result the OS3 binaries will crash when run under emulation on OS4 but the OS4 binaries should be fine.
Dopus5 is a multitasking monster with other processes opening windows too. There are 20 seperately launched library modules. I don't think there is a way to track all the windows with a hash in the main program. I don't own a classic Amiga (or an x86 PC for that matter). It's almost sheer luck that we were able to get a functional OS3 Dopus5 with no way to actually test it on OS3 hardware. I think that Dopus5 always worked before because the window UserData field always contained NULL or a valid pointer to memory where DOpus5 would not find it's "magic number" identifier. All the Amiga includes I've seen have the window UserData field defined as a pointer and not an integer value. I think Freewheel should be checking for an address of an integer value and not the actual value in the USerData field. I don't know how OS3 Dopus5 on classic hardware will react to addresses outside of the normal memory range.
The only solution I can think of for OS3 Dopus5 is to check for a value in the USerData field that is out of the normal memory range on OS3 (68k) hardware. Do you know what that upper limit would be?? If not I'll assume 24 bit addressing and check for a value above 0xFFFFFF.
Last edit: xenic 2015-04-01
@kas1e
I've committed an OS4 fix for the MUI conflict with the window UserData element. Could you (or somebody) test OS3 Dopus5 with the latest MUI release on classic hardware? If there is a problem, I'll attempt a fix for OS3 Dopus5 too. If a problem exists on classic Amigas then anyone using the SASC compiled opensource Dopus5 or the original commercial Dopus5 will probably have the same problem. I'm waiting for the next overnight compile to be sure I haven't broken the compiles for other platforms. After that I'll be away from my computer for several days and won't be able to reply to any responses immediately.
@xenic
Where can this fixed version for OS4 be downloaded?
For OS3 you cannot assume 24-bit addressing. Most 68k machines have 32-bit addressing. Blizzard puts its memory at $68000000 for example. I suggest using a token, such as 'opus'. 's' is 115, an odd number that no address will end with. Also, no other program would be likely to put the token 'opus' in there. Should be fairly safe.
@Daniel
All last nightly builds on dopus5.org, which is currently off, as those suckers from where i buy server shut it down some weeks ago because of price change. So i hope in next days setup new one, so nightly builds will be avail again.