bug#6414: f->output_data.w32->menubar_widget uninitialized?

[Lennart Borgman <lennart.borgman@gmail.com>]

On Mon, Jul 4, 2011 at 01:21, Juanma Barranquero <lekktu@gmail.com> wrote:
> On Mon, Jul 4, 2011 at 01:11, Lennart Borgman <lennart.borgman@gmail.com> wrote:
>
>> This is a complement to them. We agreed to file bug report for code
>> changes we should not forget. This is one of those.
>
> A bug report called " f->output_data.w32->menubar_widget
> uninitialized?" and suggesting to check some system calls return
> values as part of some non-very-specific hypothetical bug is not a
> "code change we should not forget", IMHO.

That there is (was) a bug in the menus is not hypothetical. The reason
I am seeing those bugs and not so many other people is probably that I
am using the menus. (Since I do not use Alt as META I can use them
more easily. And I do.)

> As I've said in the previous
> message, I think you're talking about your suggestion of checking
> every system call. If so, please create a wishlist bug report called
> "All system calls' return values should be checked", or somesuch, so
> at the very least people perusing bug reports can know at a glance
> that it is a wishlist and what it is about. I promise I won't close
> that bug (though I won't likely do much about it, either).

OK.

>> Yes. The problem with race conditions was one of the main reasons for
>> my suggestion to add error checking to all system calls. I thought
>> that you might have overlooked this since you suggested that I should
>> send a clear recipe for how to reproduce the bug. But maybe you did
>> not do that? Please explain if I am misunderstanding you.
>
> Your "problems with race conditions" do only happen to *you*, on
> *your* patched Emacs. So, if you want to help tracking them, you
> shouldn't "suggest" anything (IMO, I hasten to add). Patch your Emacs
> with debug macros to you heart's content, and when you do find
> something wrong, reproduce it with the stock Emacs and file a
> (detailed, step-by-step recipe'd) bug report about it.

I think you misunderstand the logic here. The race condition shows up
because of system messages. In your case you probably see much, much
less of those since (I believe) you use menus much less often.

This has nothing to do with my patched build. In fact I started to
patch Emacs because of those problems. And I have had much less
problems in this area with my patched version.

As I have explained quite a few times the refusal to add things like
error checking puts a lot of burden on me. I therefore do not have
time any more to work with Emacs (except for some libraries I use
myself).

>> Yes, I have a lot of changes in that area. Complicated, unfortunately.
>> I think I have lost control of the details there now. And I never felt
>> I had complete control of it. The code is complex and I am not even
>> sure that we are always doing things in the right thread etc.
>
> "We"? You're talking about code that you've changed in non-trivial ways!

No. I am talking about the core Emacs here.

>> However after fixing the things I suggested I have not seen those
>> crashes any more as far as I remember.
>
> Then, what are we discussing about?

You might want to fix the core Emacs.

>>  "The problem seems to be in x_free_frame_resources. Should not
>>  free_frame_menubar be called before my_destroy_window there?"
>
> It is perhaps a fine suggestion, if you care to explain what is "the problem".

Now I am a bit lost. I thought I had done that change, but I have not.
(Or it has been reverted by some merge, that happens.)

I think the suggestion was just because that is what the MS docs suggest.


However I see another small change that I have made to w32term.c that
might have fixed some crashes (but this is unrelated to the problem we
are discussing) :

*** c:/emacs-lp/bld/emacs/trunk/src/w32term.c	2011-03-19
12:38:38.000000000 +0100

--- c:/emacs-lp/bld/emacs/emacsw32/src/w32term.c	2011-07-04
01:37:24.883981800 +0200

***************

*** 4491,4497 ****

  	  else

  	    {

  	      f = x_window_to_frame (dpyinfo, msg.msg.hwnd);

! 	      f->async_visible = msg.msg.wParam;

  	    }

  #endif



--- 4491,4498 ----

  	  else

  	    {

  	      f = x_window_to_frame (dpyinfo, msg.msg.hwnd);

!               if (f)

!                 f->async_visible = msg.msg.wParam;

  	    }

  #endif

 ***************