Opened 8 years ago

Closed 10 months ago

Last modified 10 months ago

#3424 closed defect (fixed)

wxMSW & wxMac: "bug in wxMenu::Remove logic"

Reported by: cwalther Owned by:
Priority: low Milestone:
Component: GUI-all Version:
Keywords: wxMenu Cc: cwalther
Blocked By: Blocking:
Patch: no

Description

In both wxMSW 2.6.3 and wxMac 2.6.3 (and presumably
also in the current CVS, since the relevant code
doesn't seem to have changed), the code

wxMenu *menu = new wxMenu();
wxMenuItem *item = new wxMenuItem(menu, wxID_ANY,

wxT("item"));

menu->Remove(item);

(which is a simplified version of some real-life code I
have here) triggers an assertion failure in
wxMenu::DoRemove(wxMenuItem *item):

...\src\msw\menu.cpp(625): assert "wxAssertFailure"

failed: bug in wxMenu::Remove logic

resp.

.../src/mac/carbon/menu.cpp(312): assert

"wxAssertFailure" failed: bug in wxMenu::Remove logic

On the line preceding this assertion
(src/msw/menu.cpp:624, src/mac/carbon/menu.cpp:311),
there is a comment "DoRemove() (unlike Remove) can only
be called for existing item!", which is obviously
wrong, since wxMenuBase::Remove(wxMenuItem *item), from
which this function is called, does *not* check whether
the item is actually contained in the menu.

I'm not sure what the expected behavior is when trying
to remove a menu item that is not in the menu (the
documentation doesn't specify anything about it), but
getting an error message that says "bug in remove
logic" probably isn't it. I can imagine two solutions,
of which I'd prefer the latter (though I don't know how
this is handled on other platforms):

  • That assertion message should be changed to something

more descriptive, and the documentation should mention
that the user has to check for this condition before
calling Remove().

  • The assertion (including the comment above it) should

just be replaced by a silent "if (node == NULL)
return;", and the documentation should mention that
it's OK to call Remove() even when one isn't sure
whether the item is actually in the menu.

Change History (13)

comment:1 Changed 6 years ago by wojdyr

  • Component set to GUI-all
  • Keywords wxMenu added

comment:2 Changed 21 months ago by oneeyeman

This bug still stands.
The assertion still says: "bug in wxMenu::Remove() logic".

AFAICS, the message is not very informative and it at least should be changed as suggested by the OP.

It happens in src\msw\menu.cpp(780).

Tested on Windows.

comment:3 Changed 21 months ago by oneeyeman

Can we change the message to read "removing non-existing item?"

comment:4 follow-up: Changed 20 months ago by vadz

  • Priority changed from normal to low
  • Status changed from new to confirmed

What we should probably do, is to remove the item from m_items in wxMenuBase::Remove() itself before calling wxMenuBase::DoRemove(). Then the assert would indeed become "removing non existing item" and the derived classes DoRemove() would be able to assume that the item does exist. And DoRemove() could be probably made pure virtual too.

comment:5 in reply to: ↑ 4 Changed 20 months ago by oneeyeman

Vadim,
Replying to vadz:

What we should probably do, is to remove the item from m_items in wxMenuBase::Remove() itself before calling wxMenuBase::DoRemove(). Then the assert would indeed become "removing non existing item" and the derived classes DoRemove() would be able to assume that the item does exist. And DoRemove() could be probably made pure virtual too.

I tried to check what is happening here.
Unfortunately you suggestion will not work. The thing is that the code OP provided does not _add_ menu item to the menu, and, hence, does not add it to the m_items list. Which means that the item _is_ non-existent.

Modify menu sample as:

#if wxUSE_STATUSBAR

CreateStatusBar();

#endif wxUSE_STATUSBAR
+ wxMenu *menu = new wxMenu();
+ wxMenuItem *my_item = new wxMenuItem( menu, wxID_ANY, "item" );
+ menu->Remove( my_item );

create the menubar
wxMenu *fileMenu = new wxMenu;

and when entering "menu-Remove()" function, m_items is empty.

Or you are talking independently of this particular patch?

comment:6 Changed 10 months ago by oneeyeman

Vadim,
Any other idea?

comment:7 follow-up: Changed 10 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [75250]) Fix assert message when removing non-existent items from wxMenu.

The assert complained about a bug in wxMenu code itself, but actually could be
triggered by simply trying to remove a non-existent item from the menu.

Fix this by checking for the item existence, and also removing it from the
internal menu data structure, in wxMenuBase::Remove() and only dealing with
the item itself in DoRemove().

Closes #3424.

comment:8 in reply to: ↑ 7 Changed 10 months ago by wojdyr

Replying to VZ:

Fix this by checking for the item existence, and also removing it from the
internal menu data structure, in wxMenuBase::Remove() and only dealing with
the item itself in DoRemove().

Note that DoRemove() is also called from DoDelete() and DoDestroy()

comment:9 Changed 10 months ago by vadz

I think this is fine, at least I don't see any problems when it's called from there. Am I missing something?

comment:10 Changed 10 months ago by wojdyr

m_items.Erase(node); has been moved from DoRemove() to Remove(), so now Erase() is not called from when Delete() and Destroy().

comment:11 Changed 10 months ago by vadz

Ah, yes, of course, thanks for pointing this out!

comment:12 Changed 10 months ago by VZ

(In [75290]) Fix menu item destruction broken by r75250.

The menu items were not removed from the menu any longer when they were
deleted or destroyed after the changes in this revision.

Fix this by calling the public Remove(), which does the right thing, instead
of the private DoRemove(), which only does part of the job of removing the
item, in DoDelete() and DoDestroy().

See #3424.

comment:13 Changed 10 months ago by VZ

(In [75297]) Yet another fix after wxMenu::Remove() refactoring.

wxMenu::Remove() was still broken in wxMSW after r75250, even with the fix in
r75290, as wxMSW code relied on the item still being present in
wxMenu::m_items.

Delay removing it from there until after DoRemove() call to fix this.

See #3424.

Note: See TracTickets for help on using tickets.