[PATCH] user32: Don't depend on HMENU being valid for popup menus

[Andrew Eikum]

On Wed, May 07, 2014 at 03:59:07PM -0500, Ken Thomases wrote:
> On May 7, 2014, at 9:57 AM, Andrew Eikum wrote:
> > Unless I'm mistaken, these issues existed before this patch as well.
> > DestroyMenu has always closed the handle, and additionally destroyed
> > the struct. Now we preserve the struct if it's referenced by another
> > menu.
> 
> Well, first of all, the situation before this patch has problems
> because of that, which is what you're trying to solve.
> 
> Also, the specific issue of having the struct and going from that to
> the handle, which might not be valid, and then implicitly expecting to
> be able to get back to the struct is new.
> 

I don't think it is new. Before the patch, we still relied on the
HMENU being valid, it's just stored in a different place now. I don't
think I've introduced any dependencies on the handle being valid that
weren't already there, I've only removed them. Yes, there are some
remaining, but those can be addressed in future patches as
applications need it.

The only new problem I can think of is where before we would have just
passed an invalid HMENU, now the struct may have been destroyed and we
can no longer get the HMENU. But this issue is precisely why I had to
add the struct refcounting to this large patch instead of breaking it
out, so this should not actually be a problem.

> > There is some precedent for Windows returning an invalid HMENU. See
> > the second GetMenuItemInfoA call in test_subpopup_locked_by_menu which
> > returns the handle of the submenu that was just destroyed.
> 
> Yes.  I'm just not sure if that's what happens in these particular
> circumstances.  And I'm not so terribly concerned about the handle
> being passed out to other code being invalid, although it would be
> good to know we're doing the right thing.  It's mostly about places
> where the code has the menu pointer, obtains the menu handle from it,
> passes that to other code in the menu module, and then tries to get
> the menu pointer again.
> 

You're right that those should be cleaned up, but this patch is
already huge at over 700 changed lines. Future patches could address
those issues as needed. This patch shouldn't make them any worse than
they already are.

Andrew