99057 - M094 N620 crashes [@ GetExtensionsAndDescriptionFromMimetypesFile][@ GetTypeAndDescriptionFromMimetypesFile]

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Talkback contains occasional reports of crashes at the following stack (taken
from report #34278060):

GetExtensionsAndDescriptionFromMimetypesFile()
LookUpExtensionsAndDescription()
nsOSHelperAppService::GetFromMIMEType()
nsExternalHelperAppService::DoContent()
nsDocumentOpenInfo::DispatchContent()
nsDocumentOpenInfo::OnStartRequest()
nsHttpChannel::ProcessNormal()
nsHttpChannel::ProcessResponse()
nsHttpChannel::OnStartRequest()
nsOnStartRequestEvent::HandleEvent()
nsARequestObserverEvent::HandlePLEvent()
PL_HandleEvent()
PL_ProcessPendingEvents()
nsEventQueueImpl::ProcessPendingEvents()
event_processor_callback()
our_gdk_io_invoke()
libglib-1.2.so.0 + 0xf3c0 (0x403593c0)
libglib-1.2.so.0 + 0x10c56 (0x4035ac56)
libglib-1.2.so.0 + 0x11283 (0x4035b283)
libglib-1.2.so.0 + 0x1144c (0x4035b44c)
libgtk-1.2.so.0 + 0x9392c (0x4027592c)
nsAppShell::Run()
nsAppShellService::Run()
main1()
main() 

These have been reported in builds 2001-08-17-15 (2 reports), 2001-09-05-08 (2
reports), 2001-09-06-08 (6 reports), and 2001-09-07-08 (5 reports).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

User comments:

     (35156273)	Comments: Trying to download a playlist from live365.com
     (35135939)	Comments: Download with default decision.
     (35130618)	URL: earthlink.net
     (35112630)	Comments: Clicked a link.  Most likely it was to a downloadable file rather
than something displayable.
     (35109092)	Comments: Trying to download
     (34989377)	URL: ftp.mozilla.org/
     (34989377)	Comments: default download of 05-September
mozilla-i686-pc-linux-gnu-sea.tar.gzDownload by clicking.  Download by
shift-click works
     (34989259)	Comments: Attempting default download by clicking name of .doc file.See
mozilla bug 98355

Comment 2

[Boris Zbarsky [:bzbarsky]]

Looking into this, especially bug 98355 (since there I can get the
mailcap/mime.types files involved).

Comment 3

[Boris Zbarsky [:bzbarsky]]

So.... if the last thing in the talkback trace is

GetExtensionsAndDescriptionFromMimetypesFile()

Is there any chance that we crash in something called from that function?  Like
Substring() or something along those lines?

Or are we definitely crashing in GetExtensionsAndDescriptionFromMimetypesFile()?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The crash could be within an inlined function within it.  I could look at the
disassembly later and try to see what's crashing...

Comment 5

[Boris Zbarsky [:bzbarsky]]

If you get to that, that would be much appreciated.  I've asked all the people
whom I've found who report this crash to send me their mime.types files.... I'm
also going to go over all the code I wrote that's called from that function with
a fine-toothed comb and look for (mis|un)-initialized iterators.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The crash is in the following sequence of instructions:

408619d7 8b0a             mov     ecx,[edx]               <==== CRASH HERE
408619d9 898db4fdffff     mov     [ebp+0xfffffdb4],ecx
408619df 8d85e0fdffff     lea     eax,[ebp+0xfffffde0]
408619e5 50               push    eax
408619e6 8b8dd8fdffff     mov     ecx,[ebp+0xfffffdd8]
408619ec 51               push    ecx
408619ed 52               push    edx
408619ee 8b8db4fdffff     mov     ecx,[ebp+0xfffffdb4]

EDX is null.  Well, for completeness:

EAX: 00000000        EBX: 4086b500        ECX: bfffeac0        EDX: 00000000
ESI: bfffea10        EDI: bfffeac0        ESP: bfffe908        EBP: bfffeb60
EIP: 408619d7        cf pf af zf sf of IF df nt RF vm     IOPL: 0
CS: 0023    DS: 002b    SS: 002b    ES: 002b    FS: 0000    GS: 0000

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

and liburiloader.so is from addresses 0x40851000 to 0x40869684.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the crash is because mimeTypes is null here:

    rv = mimeTypes->ReadLine(buf, &more);

I'll attach a partially annotated disassembly of the entire function (from
objdump on the 2001-09-07-12-trunk build, from which talkback #35156273 was filed.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: partially annotated disassembly of entire function

Comment 10

[Boris Zbarsky [:bzbarsky]]

David, thank you.  Here's what's going on:

PRBool more = PR_TRUE;
rv = CreateInputStream(aFilename, getter_AddRefs(mimeFile),
                       getter_AddRefs(mimeTypes), buf, &netscapeFormat);

if (NS_FAILED(rv)) {
   return rv;
}

Unfortunately, there is the case when the file only contains one line (empty
file, eg).  In that case, CreateInputStream() will _not_ set mimeTypes and
mimeFile to anything but will return NS_OK. Furthermore, "more" will be true, so
at the bottom of the loop we don't break in the |if (!more)| and try to read
another line from mimeTypes.

So.  For a correct solution to this, CreateInputStream() should addref the
streams any time it's returning NS_OK.   I can't believe I missed that the first
time.  For a bonus, it should return the correct value of |more| so that an
extra call to ReadLine() can be avoided.

Patch coming up.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix crash (and add error-checking on filenames passed to GetFileTokenForPath)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Taking

Comment 14

[gabriel]

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

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you also want to make the do-while loop into a while loop with condition
|NS_SUCCEEDED(rv) && more|?  And do you really need the empty-check at the
beginning of GetFileTokenForPatch (in addition to the assertion, which never
hurts if it's valid, which it seems to be) if you fixed the caller?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Did you also want to make the do-while loop into a while loop with condition
> |NS_SUCCEEDED(rv) && more|?

No...  ReadLine() can return PR_FALSE for more while at the same time returning
data in the string.  more = PR_FALSE just means that further calls will return
no data.  So my first time through the loop I can have more = PR_FALSE and have
some data to process (a single entry in the file).

> And do you really need the empty-check at the beginning of
> GetFileTokenForPatch

That I do not.  Removed it.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 49348 [details] [diff] [review]
Patch to fix crash (and add error-checking on filenames passed to GetFileTokenForPath)

ok, r=dbaron

Comment 18

[Scott Collins]

Use |NS_ASSERTION| for situations that you really don't expect to happen, and
are not prepared for when they do.  Use error handling to catch situations that
you do expect, and can recover from.  Supplement with |NS_WARNING| (and note it
has a different sytax that |NS_ASSERTION|) or other instrumentation to note the
frequency of the expected-but-bad occurance in cases where you want more
information.  I think you need to change your assertion into a warning, or else
delete it entirely; the condition is expected and handled.  After that, sr=scc. 

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: reinstate error return, change assertion to warning.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 49703 [details] [diff] [review]
reinstate error return, change assertion to warning.

r=dbaron on this, although I think scc might also have been happy with
a patch incorporating the changes noted in the comments since
the most recent patch.

Comment 21

[Boris Zbarsky [:bzbarsky]]

checked in.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Reopening because I'd like to see this land on the 0.9.4 branch as well, since
it was one of the top crashes for Linux.

Comment 23

[Suresh Duddi (gone)]

nsbranch+ ing.

Comment 24

[chris hofmann]

yes, lets get this on the branch.  thanks

Comment 25

[Boris Zbarsky [:bzbarsky]]

reassign to dbaron to land on the branch if this gets pdt+

Comment 26

[Jaime Rodriguez, Jr.]

Pls get baron's r= into Patch Staus, and you get PDT+ per chofmann's email.

Comment 27

[Jay Patel [:jay]]

Adding M094 to summary, since this is a topcrasher for Mozilla 0.9.4.  There is
another crash with M094 with a similar stack signature: 

GetTypeAndDescriptionFromMimetypesFile

But the user comments talk more about reading and opening mail, so I logged a
new bug for that crash: bug 100875.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in on branch 2001-09-20 20:17 PDT.

Giving back to bzbarsky...

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

...and marking fixed.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 31

[benc]

To reproduce this, then I need a empty mime.types file (like "touch mime.types"?)

Comment 32

[Boris Zbarsky [:bzbarsky]]

Yes.  'touch ~/mime.types' and then point the private mime type file pref to the
empty file (see the 0.9.4 release notes for the pref name).

Comment 33

[benc]

STEPS:

quick question - I guess we aren't creating or touching these files when we
install or create a new profile...?

1- create empty mime.types file (see above)
2- launch Mozilla 0.9.4. Try to download daily build link on mozilla.org -> CRASH.

Okay, I'll be grabbing a post-fix build and testing tomorrow...

Comment 34

[Boris Zbarsky [:bzbarsky]]

No, profile creation and installation does not touch these files.

Comment 35

[greer]

Adding N620 to the summary for talkback. All of the incidents were in builds
before the 9/17 checkin, though. Good fix. Marking Verified.

Comment 36

[benc]

bz: should create a bug for installer so they can use your very excellent
contribution?

greer: thanks!

I'll defer my daily build test and move onto other pressing testing problemss...

reopen if i see trouble...

Comment 37

[Boris Zbarsky [:bzbarsky]]

Ben, the files involved are the system MIME registry.  We should not be
modifying them unless we're prepared to do so correctly...  The relevant bugs
are all already filed.  :)

Comment 38

[Jay Patel [:jay]]

Adding [@ GetTypeAndDescriptionFromMimetypesFile] to summary for
tracking...since bug 100875 was marked a dup.