248706 - Failing Type ahead find crashes Mozilla

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Peter Weilbacher]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8a2) Gecko/20040625
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8a2) Gecko/20040625

As reported by a few people in the npm.os2 newsgroup a failing "Type ahead
find"-operation sometimes crashes the browser in libc05.dll, the SYS3175
register screen is said to hint to a null-pointer problem.

Reproducible: Sometimes
Steps to Reproduce:




It actually happened to me now, too. I ran through IDebug, and found that the
resulting file pointer of the fopen call to write the WAV file was not checked,
so sometimes a NULL was given to fwrite. This simple patch should get rid of the
problem (see below). This may result in the sound not being played everytime,
though...

--- widget/src/os2/nsSound.cpp.orig     2004-06-26 01:37:18.000000000 +0100
+++ widget/src/os2/nsSound.cpp  2004-06-26 01:53:44.000000000 +0100
@@ -133,6 +133,12 @@
   nsCAutoString soundFilename;
   (void) soundTmp->GetNativePath(soundFilename);
   FILE *fp = fopen(soundFilename.get(), "wb+");
+  if (fp==NULL) {
+#ifdef DEBUG
+    printf("Could not open WAV file for binary writing.\n");
+#endif
+    return NS_ERROR_FAILURE;
+  }
   fwrite(data, dataLen, 1, fp);
   fclose(fp);
   HOBJECT hobject = WinQueryObject(soundFilename.get());

Comment 1

[Mike Kaply [:mkaply]]

What a stupid bug I introduced. argh.

Comment 2

[Mike Kaply [:mkaply]]

Attachment: add null check for fopen

Here's a an actual diff for what you did.

Nice job finding this.

and in case I haven't said so, Peter, you are doing an awesome job so far on
this stuff.

Comment 3

[jhp (no longer active)]

Comment on attachment 151747 [details] [diff] [review]
add null check for fopen

Replace the "#ifdef DEBUG" lines with a one-line "NS_WARNING".	Otherwise, it
looks good.

Comment 4

[Peter Weilbacher]

Thanks, Mike. I very slowly find my way around the code, and without
understanding much of it I waste a lot of time. Perhaps it will get better...

Next time I will send a "cvs diff -u" patch, just looked up how to do it.

Unfortunately the quick fix didn't help all the people in the newsgroup, it
seems to break other things. Will try to investigate that later.

I just copied the #ifdef DEBUG from a few lines further up. Do those need to be
changed as well?

Comment 5

[Mike Kaply [:mkaply]]

You did fine with the #ifdef DEBUG stuff.

Technically those other #ifdef DEBUGs in the files should be NS_WARNINGs as well.

NS_WARNING basically encapsulates the #ifdef DEBUG and printf into one statement.

I wonder if the solution to the typeahead find crash is to return NS_OK instead
of NS_ERROR_FAILURE in this case, and just do a beep perhaps?

Comment 6

[Peter Weilbacher]

The new crash in the newsgroup has the location
> TYPAHEAD.DLL 0001:00004670
which according to the map file is in method FindItNow in nsTypeAheadFind.cpp. I
don't see how that can have anything to do with this fix.

I have created a new patch from the 1.7 branch, that returns NS_OK and Beep()s
in the two cases that it fails to play the error sound. That should be a good
enough workaround until someone figures out how to do this properly using MMOS2
routines.

Comment 7

[Peter Weilbacher]

Attachment: new patch with NS_WARNING and Beep()

Comment 8

[Mike Kaply [:mkaply]]

Were you able to recreate the TYPAHEAD crash at all?

Comment 9

[Peter Weilbacher]

No, and it happened only once for Wolfi, I do not feel compelled to do anything
about it currently.

Instead I am looking at the MMOS2 functions to properly implement this, but they
are just not working the way they are documented.

Comment 10

[Peter Weilbacher]

I have tried for a week now to get some MMOS2 based stuff running and consulted
people in comp.os.os2.programmers.misc. But I just can't get the code to do what
I think it should. So for now the patch here is the best bet for 1.7.x and 1.8a2.

Comment 11

[Mike Kaply [:mkaply]]

Comment on attachment 151787 [details] [diff] [review]
new patch with NS_WARNING and Beep()

sr=mkaply

Stupid question - should we use WinAlarm with ERROR or is Beep enough?

Comment 12

[Peter Weilbacher]

Beep() uses WA_WARNING, WA_ERROR gives a deeper beep tone (on my machine and
according to the PM Programmer's Guide). As we now return NS_OK I guess WARNING
makes more sense than ERROR, and Beep() is already there...

Comment 13

[Mike Kaply [:mkaply]]

Fix checked in to trunk, 1.7, aviary.

Comment 14

[Peter Weilbacher]

I should have checked the method parameters before calling this a patch for all
branches. Trunk is OK, but Aviary and 1.7 branches still have string and
stringLen instead of data and dataLen. The mini-diffs follow.

Comment 15

[Peter Weilbacher]

Attachment: mini-diff to aviary branch

Comment 16

[Peter Weilbacher]

Attachment: mini-diff to 1.7 branch

Identical to Aviary diff (apart from version info)