Confirmed with Sound Recorder 2.13.92 on Ubuntu Dapper.

*** Bug 333393 has been marked as a duplicate of this bug. ***

(I filled a bug on this in ubuntu launchpad https://launchpad.net/distros/ubuntu/+source/gnome-media/+bug/42022/+index)

I looked at the source for grecord, in gsr-window.c the callback for the record action calls fill_record_input():

static void
record_cb (GtkAction *action,
    GSRWindow *window)
{
 GSRWindowPrivate *priv = window->priv;

 if (priv->record) {
  shutdown_pipeline (priv->record);
  if (!make_record_source (window)) exit (1);
  fill_record_input (window);
 }

 if ((priv->record = make_record_pipeline (window))) {
  window->priv->len_secs = 0;
  window->priv->saved = FALSE;

  g_object_set (G_OBJECT (priv->record->sink),
         "location", priv->record_filename,
         NULL);

  gst_element_set_state (priv->record->pipeline, GST_STATE_PLAYING);
 }
}

But fill_record_input() appends to the combo-box:

static void
fill_record_input (GSRWindow *window)
{
 GstElement *e;
 const GList *l;
 int i = 0;

 g_return_if_fail (GST_IS_MIXER (window->priv->mixer));

 for (l = gst_mixer_list_tracks (window->priv->mixer); l != NULL; l = l->next) {
  GstMixerTrack *t = l->data;
  if (t->flags & GST_MIXER_TRACK_INPUT) {
   gtk_combo_box_append_text (GTK_COMBO_BOX (window->priv->input), t->label);
   ++i;
  }
  if (t->flags & GST_MIXER_TRACK_RECORD) {
   gtk_combo_box_set_active (GTK_COMBO_BOX (window->priv->input), i - 1);
  }
 }
}

I just want to confirm the same behaviour here. Is it useful me mentioning I have the same problem?

cc me

From what I understand with my small knowledge, we have two solutions :
- remove the call to fill_record_input() from function record_cb(), but we loose the can't discover new input without restarting gnome-sound-recorder (1 line fix)
- clear the Combobox entries at the beginning of fill_record_input() (no idea)

Correct me if I'm wrong.
Anyway I'll try to contact a gnome-media maintainer.

Created attachment 65375 [details] [review]
Fix the bug for me

The patch.
It works for me.

*** Bug 343462 has been marked as a duplicate of this bug. ***

The patch looks fine, it solves a real problem easy to fix.

grecord seems a little orphan, 6 months without an entry.

May I apply the patch?

(I don't think "commented-on" is the right status for the patch since it generally means it's not sure it's okay)

If the patch is not ok, the status should be 'needs-work'.  IMHO.

Even If I'd tested the patch, which it is, 'commented-on' is the only status that can give some information the patch has been reviewed.  

Now that you say it, it seems useless.

could a maintainer comment on that patch? 

I'm not the maintainer, but some comment nevertheless:

 - clearing and re-filling the store might affect the selection and
   have unexpected side-effects through actions triggered on selection
   change; something to at least double-check before applying this
   patch IMHO

 - minor issue: one shouldn't first cast the model to a list store
   using the cast macros and only then check that the list store is
   non-NULL, since the cast macro will trigger a critical warning
   if the model is actually NULL. Better to do
      if (model) { ... }

I'm sorry, I've had notifications disabled halfway the semester (for obvious reasons) and missed this. Tim's comments are right, the variable should be checked for NULL before casting (so if (model != NULL) { call_function (GTK_LIST_STORE (model)); }

As for the second comment, the code takes that into account, so it should be OK. Please apply with the above small modification.

Created attachment 71151 [details] [review]
patch update

I've commited the "patch update" version for Baptiste, let me know if there is something not correct about it ;)

2006-08-18  Sebastien Bacher  <seb128@debian.org>

	* src/gsr-window.c: (fill_record_input): 
	"Clear combo_box first before add inputs", patch based on the work 
	from Baptiste Mille-Mathias <baptiste.millemathias@gmail.com>
	(Closes: #329922)

This needs to be re-opened.

I am running Ubuntu Gutsy (7.10) Tribe 3 pre-release (gtk 2.11.6, glib 2.13.7, gstreamer 0.10.13).

The code from SVN:
 * works OK for revision {20060817}
 * crashes with revision {20060819} (see steps to reprocuce below)

which corresponds exactly to this patch applied.

It seems Tim's first point from comment #13 actually is valid since it's the reason why the program crashes in this situation:

Steps to reproduce
==================
1. Start gnome-sound-recorder
2. record->stop, then do whatever you want, but not record again
3. press record again - crashes immediately with segfault

Backtrace:

(gdb) bt

+ Trace 151515

By adding custom debugging output, I discovered that the callback record_input_changed_cb which causes the segfault is triggered by line 1275, a.k.a. fill_record_input, which is called from record_cb, a.k.a. the callback called when user clicks the record button; but in this situation, record_input_changed_cb is not called as a result of user selecting a different input, but as a result of programmatic change of the input combobox, exactly as Tim has foreseen.

Ronald, you said that there is a code that should actually check for this condition and ensure this does not happen, could you please tell us which code? Maybe it got lost during subsequent SVN commits.

I am no expert in glib and gtk, but I have poked around a bit with the source code, so there are imho 2 solutions that I am aware of:

1. before fill_record_input (or at its start) disconnect the changed signal of combobox; after filling in of the combo box is done, reconnect it again. (or emulate this using something like "inside_fill_record_input" bool variable and check for it in record_input_changed_cb, and if it's true, record_input_changed_cd would return immediately.

2. remove this refilling. First, it seems very unprobable that a new source would be connected. Second, more correct would be for gstreamer to provide a signal for this purpose (track-added, track-removed), but at least by looking at docs, GstMixer does not provide this. From what I guess that such situation probably can't happen, since GStreamer would probably provide these if it was usable. (Does ALSA support it anyway? If I hotplugged USB microphone, would it be immediately available in mixer?)

Doing 1. correctly is not trivial. e.g. before refilling, we would have to store current input; when refilling, check if the current input from GstMixer is the same as the previous, and if yes, don't emit a signal, etc... And what is more, updating the inputs _after_ recording is started is IMHO not very usable.

I tried 2. (removing the line 1275) That fixes the crash for me.

This makes me think that we should support such a hotplug functionality when GStreamer does it itself.

I prefer 2., at least for now. (patch attached)

What do you think?

strange things I discovered ... in Ubuntu Feisty Fawn (released in April 2007), this bug does not happen; when testing gnome-sound-recorder as in SVN as of 20060819 without the patch applied (= long before Feisty was released), and the latest version without the patch applied, the bug is there - maybe Ubuntu has some Ubuntu-specific fix to handle that?

Created attachment 92757 [details] [review]
Do not update input list when "Record" button hit

Why are they duplicated anyway? Is that for device updates? Does that ever happen?

Boris, before we commit this, the strcmp() is triggered by this code, but is a bug in itself and needs fixing. Your fix may be right for another bug, which is the fact that we're updating, but I still want to see a patch for ths crasher in strcmp() here that actually fixes that issue, regardless of whether we call fill_inputs() unexpectedly or not in the code. Therefore, the patch is not good enough to fix this bug (for me).

OK Ronald, I understand what you mean with fixing the "strcmp" part. I will try to have a look at it.

Created attachment 92765 [details] [review]
Check track name is not NULL before strcmp'ing it

OK, I did the checking for strcmp. I do check both arguments to it for NULL'ness.

And I added 2 FIXME comments about code that gets never called, but probably had some purpose - but I feel not qualified enough to decide if the code actually has some purpose and if yes, how to enable it properly.

With this patch, my previous patch is not needed, but that does not mean that I don't think that the feature of refilling inputs after hitting record button is useful. It might be however useful with a special button "Reload available inputs". Should I implement that? (it should be fairly easy - just add a new button to the UI and add a callback for clicking it that would simply just call fill_record_input)

What I mean - 1. Does GStreamer even support hotplugging new inputs? 2. If yes, does it provide callbacks for notification of this?
If the answers are "1. Yes 2. No", then it would make sense to implement it with a button.
If the answers are "1. Yes 2. Yes", then it would be better to implement it using the callbacks.
And if the answers are "1. No 2. *", then I would suggest applying my previous patch to remove the refilling.

Ronald,

could you please review my last patch (the NULL checking)?
I think it should be quite straightforward.

I hope this will get the chance to get in before Gnome 2.20 release.

Thanks,
Boris

I don't know about the refilling, I asked Tim about 10 times and never got an answer. It wasn't there originally. But code shouldn't be removed just because, it should be fixed. The second patch is ok, you can apply it.

I don't have SVN write access = I can't commit the second patch.

I committed the second patch. I'm leaving this opened as the title of the bug is that entries get duplicated. I guess they still do now :)

2007-09-06  Bastien Nocera  <hadess@hadess.net>

        * src/gsr-window.c: (record_input_changed_cb): Patch from
        Boris Dušek <boris.dusek@gmail.com> to avoid crashing when
        strcmp'ing NULL strings (Helps: #329922)

*** Bug 463891 has been marked as a duplicate of this bug. ***

(In reply to comment #26)
> I committed the second patch. I'm leaving this opened as the title of the bug
> is that entries get duplicated. I guess they still do now :)

The duplication problem got solved with commit in comment #16, I guess. At least during my testing, I did not see any duplication. I needed to reopen this bug since after this commit, a new crasher problem was introduced. My patch that you just commited fixes the introduced crasher. So I think it is right to close this bug now.

In the inprobable event that there are still some problems with this bug, anyone can at anytime reopen it. :-)

Cool, closing then.