Opened 10 years ago

Closed 6 years ago

#4147 closed bug (fixed)

Magnify app crashes

Reported by: atmartens Owned by: jscipione
Priority: normal Milestone: R1
Component: Kits/Interface Kit Version: R1/pre-alpha1
Keywords: Cc: stimut@…
Blocked By: Blocking: #6610
Has a Patch: no Platform: All

Description

I was playing with the magnifying application and it crashed on me after a right-click.

Here is a backtrace:

#0  0x0020b204 in TMagnify::MouseDown ()
#1  0x0036c2d0 in BWindow::DispatchMessage () from /boot/system/lib/libbe.so
#2  0x00368ad3 in BWindow::task_looper () from /boot/system/lib/libbe.so
#3  0x002ba904 in BLooper::_task0_ () from /boot/system/lib/libbe.so
#4  0x0062f08d in thread_entry () from /boot/system/lib/libroot.so
#5  0x78074fec in ?? ()

Attachments (2)

magnify-crash.png (252.5 KB) - added by atmartens 10 years ago.
Menu.cpp.diff (449 bytes) - added by stimut 9 years ago.
Updated fix

Download all attachments as: .zip

Change History (15)

Changed 10 years ago by atmartens

Attachment: magnify-crash.png added

comment:1 Changed 10 years ago by anevilyak

Component: - GeneralApplications/Magnify

comment:2 Changed 10 years ago by atmartens

The only problem is I can't seem to reproduce the bug. I'll keep at it. :-)

comment:3 Changed 9 years ago by stimut

I was able to reproduce it: select a separator from the pop-up menu.

This is caused by the BPopupMenu::Go() function returning non-NULL when an item is selected even if the item has no BMessage associated with it (i.e. the item does nothing - such as for a separator).

I have created a simple patch so that it now returns NULL when the item selected has no message associated with it.

comment:4 Changed 9 years ago by stimut

Cc: stimut@… added

comment:5 Changed 9 years ago by stippi

Nice work! You definitely identified the problem and correct location for the fix, only your fix is incorrect. BMenu should not care whether the BMenuItem has a BMessage or not, but I don't suppose it should return BSeparatorMenuItem instances. This should actually be checked against the BeOS implementation. I still have a ZETA installation which I can test on, but in general, I think one should return NULL in case dynamic_casting to BSeparatorMenuItem is successful.

comment:6 in reply to:  5 Changed 9 years ago by jackburton

Replying to stippi:

Nice work! You definitely identified the problem and correct location for the fix, only your fix is incorrect. BMenu should not care whether the BMenuItem has a BMessage or not, but I don't suppose it should return BSeparatorMenuItem instances. This should actually be checked against the BeOS implementation. I still have a ZETA installation which I can test on, but in general, I think one should return NULL in case dynamic_casting to BSeparatorMenuItem is successful.

I seem to recall something like this having been fixed in the past: IOW: this chould be a regression.

comment:7 Changed 9 years ago by jackburton

Check hrev17026.

comment:8 Changed 9 years ago by stippi

Ah, that makes sense. The correct fix is thus "if (item->Frame().Contains(where) && item->IsEnabled())".

comment:9 in reply to:  8 Changed 9 years ago by stimut

Component: Applications/MagnifyKits/Interface Kit

Replying to stippi:

Ah, that makes sense. The correct fix is thus "if (item->Frame().Contains(where) && item->IsEnabled())".

Yes, I agree. I have updated the patch in case someone wants to commit the changes.

Changed 9 years ago by stimut

Attachment: Menu.cpp.diff added

Updated fix

comment:10 Changed 9 years ago by stippi

Resolution: fixed
Status: newclosed

Thanks a lot! Applied in hrev35248.

comment:11 Changed 6 years ago by jscipione

Blocking: 6610 added
Resolution: fixed
Status: closedreopened

This fix for this bug was not right because it caused #6610

comment:12 Changed 6 years ago by jscipione

Owner: changed from axeld to jscipione
Status: reopenedin-progress

comment:13 Changed 6 years ago by jscipione

Resolution: fixed
Status: in-progressclosed

Fixed in hrev45977

Note: See TracTickets for help on using tickets.