Status

Core Graveyard
Widget: OS/2
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: Rich Walsh, Assigned: Rich Walsh)

Tracking

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 393451 [details] [diff] [review]
nsSound rewrite

The existing code for nsSound is suspected of causing instability.  It's also a slovenly mess.  The attached rewrite addresses these issues:

- arguments passed to the secondary thread no longer live on the primary thread's stack;  the event semaphore that was supposed to "protect" them has been eliminated (who wrote that stuff?)

- explicit file i/o has been eliminated;  if a file is to be played, it is converted to an nsURL that's passed to the existing nsStreamLoader code

- the MMOS/2 dlls are loaded once & remain loaded for the remainder of the session;  only the 3 entry points that are actually needed are loaded

- allocation of high memory is now handled properly;  previously, it would fail on systems where hi-mem was absent or disabled

- the code has been reorganized & reformatted in a consistent style;  dead code, unneeded headers, excessive debugging code, etc, have been eliminated

An easy way to test this code with an actual sound is to point the preference "accessibility.typeaheadfind.soundURL" at a file on your system.

Important!  While testing this, I got sporadic crashes in a few different Thebes modules as well as the gcc CRT's free() code.  This could be instability in the current version of that code.  However, it reminded me of the problems I had while developing libsydneyaudio that came from passing high-memory buffers to MMOS/2.  If anyone experiences these issues AFTER PLAYING A SOUND FILE, then the use of hi-mem should be eliminated (if no sound has been played, then this is NOT the cause).
Attachment #393451 - Flags: review?(mozilla)
(Assignee)

Comment 1

9 years ago
Created attachment 393453 [details]
the new nsSound.cpp (FYI only)

this is the final product - it may be easier to get the "big picture" from this rather than the diff since so much has been eliminated/rearranged/reformatted

Comment 2

9 years ago
You are trying to change code that is the result of extensive exchange between Lars Erdmann an me over more than a month to get it working and crash free (we were seeing spurious crashes like yours with a lot of versions before). Only part of this exchange is reflected in bug 253955.

That said, I'm in favor of removing extra debug code (especially the stuff in playSound() that since the code landed in the tree hasn't shown me any errors) and the #if0/#endif block. What I don't like is that you are moving the static functions to the bottom and what you did to most of the "else if" blocks. The latter also goes against the coding style that was discussed again over the last weeks in mozilla.dev.platform and can be found (at the bottom of) <https://developer.mozilla.org/En/Developer_Guide/Coding_Style>.

I have to think about the rest.
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> You are trying to change code that is the result of extensive exchange
> [...] to get it working and crash free

The crashes you saw in PM dlls suggest problems with MMOS2 - that part of the code hasn't been changed at all.  The crashes I saw in moz thebes modules more nearly resemble MMOS2's problems with high memory - and there's no magic in the existing code or any replacement code that can avoid this.

If we really want to enhance stability, we should:
 - eliminate the use of hi-mem for the sound file buffer.  With so many other things moved to hi-mem, there's plenty of room in low-mem these days for the typical 64-128k needed.
 - avoid loading the MMOS2 dlls until they're actually needed.  Since few users set a typeaheadfind or newmail sound, these dlls will seldom if ever be loaded during a session.

> What I don't like is that you are moving the static functions to the
> bottom

Why should subsidiary functions that don't become meaningful until the "meat" of the class has been examined be at the top rather than in-line or at the bottom?  To save someone the 30 seconds needed to create 3 prototypes?

> and what you did to most of the "else if" blocks.

I wasn't aware that this had become the official style - I'll change them.
(Assignee)

Comment 4

9 years ago
Created attachment 398973 [details] [diff] [review]
nsSound rewrite v2

This version defers loading the MMOS2 dlls until the first time a sound file is actually going to be played.  It retains the use of high memory because I haven't had a single problem with it in weeks.

A major addition to this version is a full implementation of Mozilla event sounds.  A companion REXX script lets users add selected Mozilla events to the WPS Sound object where they can be associated with sounds in the same way as system events.  Once the novelty has worn off, the script can also be used to delete some or all of the events.
Attachment #393451 - Attachment is obsolete: true
Attachment #393453 - Attachment is obsolete: true
Attachment #393451 - Flags: review?(mozilla)
(Assignee)

Comment 5

9 years ago
Created attachment 398977 [details]
add/delete Mozilla events to/from the WPS Sound object

This is an interactive REXX script to add or delete entries for Mozilla events to mmpm.ini.  Once an event has been added, it can be accessed from the WPS Sound object and associated with a sound.  Only events the user selects are added, and any or all can be deleted at any time.  The script also make provisions for the possibility that its entries might conflict with those from other apps.
(Assignee)

Comment 6

9 years ago
Created attachment 406881 [details]
MozSounds.cmd REXX script - v2

This revision makes a backup of mmpm.ini the first time it's run.
Attachment #398977 - Attachment is obsolete: true
Attachment #406881 - Flags: review?(mozilla)
(Assignee)

Comment 7

9 years ago
Comment on attachment 398973 [details] [diff] [review]
nsSound rewrite v2

This patch has now been used by many people for over a month with no identifiable problems.  It would be nice to get this into v3.6 since it adds a minor enhancement to an otherwise feature-poor release.
Attachment #398973 - Flags: review?(mozilla)
(Assignee)

Comment 8

9 years ago
Ooops...  While I wasn't looking, they replaced nsSound with nsSystemSoundService.  This patch remains valid for 3.6, and IMHO, should be reviewed and committed.  I'll write a new version for the trunk.

Updated

9 years ago
Attachment #398973 - Flags: review?(mozilla) → review+

Comment 9

9 years ago
Comment on attachment 406881 [details]
MozSounds.cmd REXX script - v2

If you want this checked in, you should make it part of the patch.
Attachment #406881 - Flags: review?(mozilla) → review+
(Assignee)

Comment 10

9 years ago
Created attachment 406899 [details] [diff] [review]
nsSound rewrite v3 including MozSounds.cmd [checkin comment 12]

Beside adding the script to the patch, I also added a line to Makefile.in to copy the file to dist/bin.  It works but I don't know if the line is in the right place.  Please correct it if I got it wrong.
Attachment #398973 - Attachment is obsolete: true
Attachment #406881 - Attachment is obsolete: true
Attachment #406899 - Flags: review?(mozilla)

Comment 11

9 years ago
I think it should use $(INSTALL) instead of cp (which was just because the README file gets renamed while copied) but should also happen with other apps, i.e. outside the ifdef/endif block. Let me see how that can be best done.

I'll check in the patch without the Makefile change for now.

Comment 12

9 years ago
Comment on attachment 406899 [details] [diff] [review]
nsSound rewrite v3 including MozSounds.cmd [checkin comment 12]

Pushed (without Makefile change):

http://hg.mozilla.org/mozilla-central/rev/282c9ba9e8d0
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3ac6c2063f63
Attachment #406899 - Attachment description: nsSound rewrite v3 including MozSounds.cmd → nsSound rewrite v3 including MozSounds.cmd [checkin comment 12]
Attachment #406899 - Flags: review?(mozilla) → review+

Comment 13

9 years ago
Created attachment 407992 [details] [diff] [review]
makefile change [checkin comment 16]

This is a trick that I found in modules/plugin/default/os2/Makefile.in. It should work for all apps.
Attachment #407992 - Flags: review?(wuno)

Comment 14

9 years ago
Comment on attachment 407992 [details] [diff] [review]
makefile change [checkin comment 16]

We should add MozSounds.cmd also to browser/installer/packager-manifest.in, probably in a section starting with ;[OS2 specific files]
Can we do it here or in another bug?
Attachment #407992 - Flags: review?(wuno) → review+

Comment 15

9 years ago
Comment on attachment 407992 [details] [diff] [review]
makefile change [checkin comment 16]

>[OS/2] Bug 509330 - copy the MozSounds.cmd file into the program dir
>
>

>+	$(INSTALL) MozSounds.cmd $(DIST)/bin/
change to: $(INSTALL) $(srcdir)/MozSounds.cmd $(DIST)/bin/
otherwise MozSounds.cmd won't get found

Comment 16

9 years ago
Comment on attachment 407992 [details] [diff] [review]
makefile change [checkin comment 16]

Fixed on checkin as requested:
http://hg.mozilla.org/mozilla-central/rev/81edc26bc8a0
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dfce9796b38f
Attachment #407992 - Attachment description: makefile change → makefile change [checkin comment 16]

Updated

9 years ago
Blocks: 526630

Comment 17

9 years ago
I split off the packaging issue into bug 526630 because we'll need approval for 1.9.2 (even though it's a trivial patch).
Status: NEW → RESOLVED
Last Resolved: 9 years ago
status1.9.2: --- → final-fixed
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.