There is a complain from Valgrind about invalid memory access in snd_device_name_hint(-1, ..) // ...

There is a complain from Valgrind about invalid memory access in snd_device_name_hint(-1, ..) // -1 means all cards.
It looks like the problem is actually because we use -1 to loop through all the soundcards.
And this patch will loop through the soundcard manually, which should resolve the Valgrind problem.

Bug=96207
Test=media_unittests with Valgrind

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101240

[Alexander Potapenko, 2011-09-13 17:03:03]

http://codereview.chromium.org/7888011/diff/1/media/audio/linux/audio_manager...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/7888011/diff/1/media/audio/linux/audio_manager...
media/audio/linux/audio_manager_linux.cc:72: // Don't use
snd_device_name_hint(-1,..) since there is a memory leak inside
There is not a memory leak, but an access violation inside that ALSA API. Please
fix the comment.

[Raymond Toy (Google), 2011-09-13 17:07:40]

http://codereview.chromium.org/7888011/diff/1/media/audio/linux/audio_manager...
File media/audio/linux/audio_manager_linux.cc (left):

http://codereview.chromium.org/7888011/diff/1/media/audio/linux/audio_manager...
media/audio/linux/audio_manager_linux.cc:84:
wrapper_->DeviceNameFreeHint(hints);
Won't there be a real leak now since you don't free hints anymore?

[wjia(left Chromium), 2011-09-14 03:37:17]

Nice work! I also tried your patch with suppression removed and it succeeds
valgrind bot. See
http://build.chromium.org/p/tryserver.chromium/builders/linux_valgrind/builds...

LGTM.

[xians, 2011-09-14 08:12:00]

Fix the comment; Wait for a owner stamp to commit.

http://codereview.chromium.org/7888011/diff/1/media/audio/linux/audio_manager...
File media/audio/linux/audio_manager_linux.cc (left):

http://codereview.chromium.org/7888011/diff/1/media/audio/linux/audio_manager...
media/audio/linux/audio_manager_linux.cc:84:
wrapper_->DeviceNameFreeHint(hints);
On 2011/09/13 17:07:40, rtoy wrote:
> Won't there be a real leak now since you don't free hints anymore?

In case DeviceNameHint fails, hints should be always NULL, so it is all right
not to call DeviceNameFreeHint. Of course, it is also safe to call it since ALSA
takes care of the case when hints==NULL.

http://codereview.chromium.org/7888011/diff/1/media/audio/linux/audio_manager...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/7888011/diff/1/media/audio/linux/audio_manager...
media/audio/linux/audio_manager_linux.cc:72: // Don't use
snd_device_name_hint(-1,..) since there is a memory leak inside
On 2011/09/13 17:03:04, Alexander Potapenko wrote:
> There is not a memory leak, but an access violation inside that ALSA API.
Please
> fix the comment.

Done.

[_com_google_glider, 2011-09-14 09:18:37]

Your patch indeed fixes the errors reported by AddressSanitizer in
media_unittests and content_unittests. Thanks for that!
Please add the "BUG=95797,96207" to the description in order to automatically
update the issues when you commit the CL.

[scherkus (not reviewing), 2011-09-14 16:32:31]

http://codereview.chromium.org/7888011/diff/5/media/audio/linux/audio_manager...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/7888011/diff/5/media/audio/linux/audio_manager...
media/audio/linux/audio_manager_linux.cc:85: LOG(ERROR) << "Unable to get device
hints: " << wrapper_->StrError(error);
do we still need  to free the hint here?

it looks like before you called DeviceNameFreeHint() all the time

[scherkus (not reviewing), 2011-09-14 16:33:07]

also do you plan on removing the suppression?

[xians, 2011-09-14 16:36:54]

Andrew, could you please guide me how to remove the suppression?

http://codereview.chromium.org/7888011/diff/5/media/audio/linux/audio_manager...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/7888011/diff/5/media/audio/linux/audio_manager...
media/audio/linux/audio_manager_linux.cc:85: LOG(ERROR) << "Unable to get device
hints: " << wrapper_->StrError(error);
No, don't need to do it. Since when DevicenameHint return -1, hints will be
always NULL, so we don't need to free it in this case. 
Previously we always do it because we know ALSA will handle the case even though
hints is NULL.

On 2011/09/14 16:32:31, scherkus wrote:
> do we still need  to free the hint here?
> 
> it looks like before you called DeviceNameFreeHint() all the time

[wjia(left Chromium), 2011-09-14 18:11:23]

It seems that build bots and try bots don't have sound card. When
snd_device_name_hint is called, it's suspected that some string functions such
as sprintf/strcpy are called with NULL pointer as source. By checking card
availability before calling snd_device_name_hint, this problem can be avoided.

[scherkus (not reviewing), 2011-09-14 18:45:50]

I'm confused -- are you going to update the source code to check for devices
somehow first, or is this patch ready to go?

If you don't need to update the source code... then LGTM :)

[wjia(left Chromium), 2011-09-14 18:56:44]

On Wed, Sep 14, 2011 at 11:45 AM, <scherkus@chromium.org> wrote:

> I'm confused -- are you going to update the source code to check for
> devices
> somehow first, or is this patch ready to go?
>
> I think this patch is ready to go (it does check devices first). What I
said is about the possible reason for the problem that this patch fixed (or
workaround).


> If you don't need to update the source code... then LGTM :)
>
>
>
http://codereview.chromium.**org/7888011/<http://codereview.chromium.org/7888...
>

[commit-bot: I haz the power, 2011-09-14 20:30:29]

Try job failure for 7888011-3002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-09-15 03:59:37]

Change committed as 101240