Return value of esd_open_sound not checked correctly

RESOLVED FIXED

Status

()

Core
Widget: Gtk
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
esd_open_sound returns a file descriptor, we test for (!fd) when we should test for (fd < 0).

Note: this is a spin off of my patch from bug 469635.
(Assignee)

Comment 1

9 years ago
Created attachment 363833 [details] [diff] [review]
patch v1

Also removes confusing/useless comments.
Assignee: nobody → kinetik
Attachment #363833 - Flags: superreview?(roc)
Attachment #363833 - Flags: review?(roc)
Attachment #363833 - Flags: superreview?(roc)
Attachment #363833 - Flags: superreview+
Attachment #363833 - Flags: review?(roc)
Attachment #363833 - Flags: review+
(Assignee)

Comment 2

9 years ago
Pushed 5ac18226837e.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 339885
Depends on: 504670
Depends on: 576365
Comment on attachment 363833 [details] [diff] [review]
patch v1

>     if (!elib) {
>-        /* we don't need to do esd_open_sound if we are only going to play files
>-           but we will if we want to do things like streams, etc */
>-        EsdOpenSoundType EsdOpenSound;
>-
>         elib = PR_LoadLibrary("libesd.so.0");
>         if (elib) {
>-            EsdOpenSound = (EsdOpenSoundType) PR_FindFunctionSymbol(elib, "esd_open_sound");
>+            EsdOpenSoundType EsdOpenSound =
>+                (EsdOpenSoundType) PR_FindFunctionSymbol(elib, "esd_open_sound");
>             if (!EsdOpenSound) {
>                 PR_UnloadLibrary(elib);
>                 elib = nsnull;
>             } else {
>                 esdref = (*EsdOpenSound)("localhost");
>-                if (!esdref) {
>+                if (esdref < 0) {

I consider this patch/fix broken. It makes sense at first glance but it doesn't fit
reality AFAICS.

1. The error return code of esd_open_sound() is defined as -1 in any case (no need to fiddle with that)
2. The comment which was removed /* we don't need to do esd_open_sound if we are only going to play files */
   seems to be perfectly true. The usecase of notification in TB, SM and FF is playing WAV files.
   This definitely works w/o esd running and therefore the !esdref makes more sense than esdref < 0
   The new implementation unloads libesd just because no esd is running which is done under the
   assumption that libesd cannot output anything w/o esd running what is not true.
reopening (or do we need a new bug?)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 458313 [details] [diff] [review]
patch reverting the change
Created new bug 579877 to allow proper tracking.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
Depends on: 579877
You need to log in before you can comment on or make changes to this bug.