39987 - Crash in nsMultiMixedConv::SendStop after submitting form [@ nsMultiMixedConv::SendStop]

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Cameron Smith]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.15 i686; en-US; m16) Gecko/20000520
BuildID:    2000052008

When I submit a query (I entered summary: yes to all, and selected SMAC as the
product)

Reproducible: Always
Steps to Reproduce:
1.go to http://fenris.lokigames.com/query.cgi
2.enter yes to all in the summary: field
3.click submit

Comment 1

[Andreas Franke (gone)]

Attachment: stack trace including function symbols, PC/Linux 5/13 build

Comment 2

[Andreas Franke (gone)]

Confirming bug. Reproduced on PC/Linux with several builds, including
2000052008. Here's the stack trace:

#0 nsMultiMixedConv::SendStop ()               from components/libnecko.so
#1 nsMultiMixedConv::OnDataAvailable ()        from components/libnecko.so
#2 nsDocumentOpenInfo::OnDataAvailable ()      from components/liburiloader.so
#3 nsHTTPFinalListener::OnDataAvailable ()     from components/libnecko.so
#4 InterceptStreamListener::OnDataAvailable () from components/libnecko.so
#5 nsHTTPChunkConv::OnDataAvailable ()         from components/libnecko.so
#6 nsHTTPServerListener::OnDataAvailable ()    from components/libnecko.so
#7 nsOnDataAvailableEvent::HandleEvent ()      from components/libnecko.so
#8 nsStreamListenerEvent::HandlePLEvent ()     from components/libnecko.so

This crash has been introduced between 2000041609 and 2000041816 builds.
Changing component to networking and assigning to valeski because of his
change to nsMultiMixedConv in that timeframe, see:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=nsMultiMixedConv.*&filetype=regexp&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=04%2F16%2F2000+08%3A00%3A00&maxdate=04%2F18%2F2000+17%3A00%3A00&cvsroot=%2Fcvsroot

BTW: Jud, if you know about the MultiMixedConverter's stop methods,
maybe you can help understanding the infinite loop problem in bug 38423?
(Well, I just recognize that you are on the cc list there.)

Anyway, resummarizing this bug, adding crash keyword, cc self.

Comment 3

[Judson Valeski]

hmm. my fresh linux pull/build from this morning doesn't crash. works just fine.

I typed "yes to all" in the summary field and hit submit.

Comment 4

[Cameron Smith]

Hmm... I concur.  Today's build, 2000052109 on Linux, doesn't seem to cause the
crash.

Comment 5

[Andreas Franke (gone)]

You're right that "you to all" usually doesn't crash.
I should have said that I did my testing by just typing 
"foobar" into the summary field. "foobar" is still crashing
the 5/21 build.
My guess is that the crash always occurs in the situation
where the answer is "Zarro Boogs found". Note that there has
been entered a bug on 5/20 that matches the "yes to all" query.
I tried "this does not occur in any bug" and it also crashed.
(I don't select any product, but I think this doesn't matter.)
It happened two times (out of many times I tested) today that it
did not crash and the "Zarro Boogs found" page was displayed.
However, in these cases I managed to crash by hitting the back
button from this page. 
Maybe you have to try two times, but I'm sure the bug is still  
alive. It also seems to help run mozilla within gdb. Without gdb,
it is more likely to crash only on the back button.

Comment 6

[Judson Valeski]

nailed it.

Comment 7

[Asa Dotzler [:asa]]

Sorry for the spam.  New QA Contact for Browser General.  Thanks for your help
Joseph (good luck with the new job) and welcome aboard Doron Rosenberg

Comment 8

[Jay Patel [:jay]]

This crash seems to back according to the latest talkback crash data.  I 
couldn't reproduce the test case given here because the link above took me to a 
signon screen for bugzilla for some reason and wouldn't accept my usual login 
info.  But here is some info from talkback:

nsMultiMixedConv::SendStop 92ae4fa8
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/streamconv/converter
s/nsMultiMixedConv.cpp line 397
        Build: 2000070909 CrashDate: 2000-07-09 UptimeMinutes: 1  Total: 1 
        OS: Windows 95  4.0 build 67109975
        URL: http://216.100.231.10:2230/
        Comment: I was viewing http://216.100.231.10:2230/xxx.jpg
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=13877450

nsMultiMixedConv::SendStop 6aeaea43
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/streamconv/converter
s/nsMultiMixedConv.cpp line 397
        Build: 2000071009 CrashDate: 2000-07-11 UptimeMinutes: 0  Total: 179 
        OS: Windows 98  4.10 build 67766222
        URL: http://bugzilla.mozilla.org/show_bug.cgi?id=45125
        Comment: goto bugzilla page using the bottom bar
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=13964533

nsMultiMixedConv::SendStop e03f3f8b
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/streamconv/converter
s/nsMultiMixedConv.cpp line 397
        Build: 2000071009 CrashDate: 2000-07-11 UptimeMinutes: 75  Total: 245 
        OS: Windows NT  4.0 build 1381
        URL: 
        Comment: crash clicking on personal toolbar button
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=14002480

    nsMultiMixedConv::SendStop e03f3f8b
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/streamconv/converter
s/nsMultiMixedConv.cpp line 397
        Build: 2000071009 CrashDate: 2000-07-11 UptimeMinutes: 33  Total: 279 
        OS: Windows NT  4.0 build 1381
        URL: 
        Comment: crash clicking personal toolbar
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=14003625

And the latest stack trace:
Incident ID 13877450 
nsMultiMixedConv::SendStop 
[d:\builds\seamonkey\mozilla\netwerk\streamconv\converters\nsMultiMixedConv.cpp, 
line 397] 
nsMultiMixedConv::OnDataAvailable 
[d:\builds\seamonkey\mozilla\netwerk\streamconv\converters\nsMultiMixedConv.cpp, 
line 162] 
nsDocumentOpenInfo::OnDataAvailable 
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp, line 252] 
nsHTTPFinalListener::OnDataAvailable 
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHTTPResponseListener.cp
p, line 1232] 
nsHTTPServerListener::OnDataAvailable 
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHTTPResponseListener.cp
p, line 557] 
nsOnDataAvailableEvent::HandleEvent 
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsAsyncStreamListener.cpp, line 
407] 
nsStreamListenerEvent::HandlePLEvent 
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsAsyncStreamListener.cpp, line 
106] 
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 588] 
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, 
line 547] 
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 
1045] 
KERNEL32.DLL + 0x3663 (0xbff73663) 
KERNEL32.DLL + 0x22894 (0xbff92894) 
0x00688b48 

Comment 9

[Andreas Franke (gone)]

I tried the old steps-to-reproduce on PC/Linux 2000071208, but I could not
reproduce the crash any more. 

However going to http://216.100.231.10:2230/xxx.jpg crashes immediately.

Comment 10

[Andreas Franke (gone)]

An attempt to download that jpeg failed because it does not seem to have an
end... wget says: Length: unspecified [multipart/x-mixed-replace]

Comment 11

[Judson Valeski]

I cannot get http://216.100.231.10:2230/xxx.jpg to crash on windows or linux
using builds from today. I can't get that image to refresh even in 4.x. How
often does that server push a new image?

I can see where we're crashing here, but I need a test case to solve the
underlying problem.

Comment 12

[Andreas Franke (gone)]

On Linux, you should crash if you do the following:
1) ssh some.remote.host
2) on that host: mozilla http://216.100.231.10:2230/xxx.jpg

If you leave out step 1), mozilla doesn't crash. (Build ID: 2000071408)

Comment 13

[Jay Patel [:jay]]

here's some more talkback data that might help:

nsMultiMixedConv::SendStop e03f3f8b
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/streamconv/converter
s/nsMultiMixedConv.cpp line 397
        Build: 2000071109 CrashDate: 2000-07-12 UptimeMinutes: 63  Total: 63 
        OS: Windows NT  4.0 build 1381
        URL: 
        Comment: Clicked on link for nsbeta2+ bugs for nisheeth's team on 
http://slip/projects/seamonkey/SeamonkeyBugQueries.html and crashed
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=14046869

    nsMultiMixedConv::SendStop cc9028f7
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/streamconv/converter
s/nsMultiMixedConv.cpp line 397
        Build: 2000071209 CrashDate: 2000-07-12 UptimeMinutes: 20  Total: 20 
        OS: Windows NT  4.0 build 1381
        URL: http://216.100.231.10:2230/xxx.jpg
        Comment: just pasted the url in and pressed enter.  crash trying to load
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=14048944

    nsMultiMixedConv::SendStop 35b3e5b1
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/streamconv/converter
s/nsMultiMixedConv.cpp line 397
        Build: 2000071209 CrashDate: 2000-07-14 UptimeMinutes: 674  Total: 674 
        OS: Windows 98  4.10 build 67766446
        URL: http://bugzilla.mozilla.org
        Comment: Hit back button to return to bugs fileed today list from a bug 
I was looking a
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=14187095

Comment 14

[Doron Rosenberg (IBM)]

*spam* - setting default qa contact [e-mail]

Comment 15

[Andreas Franke (gone)]

Maybe it has something to do with the display having a low bit depth?
Anyway, in a 7/21 debug build I hit the following assertions before the crash:

###!!! ASSERTION: we should be pushing raw data: '!mProcessingHeaders', file
../../../../mozilla/netwerk/streamconv/converters/nsMultiMixedConv.cpp, line 133

###!!! ASSERTION: implies we're processing: 'mPartChannel', file
../../../../mozilla/netwerk/streamconv/converters/nsMultiMixedConv.cpp, line 134

###!!! ASSERTION: our channel went away :-(: 'mPartChannel', file
../../../../mozilla/netwerk/streamconv/converters/nsMultiMixedConv.cpp, line 412

###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().:
'mRawPtr != 0', file ../../dist/include/nsCOMPtr.h, line 649

Comment 16

[Andreas Franke (gone)]

Note the following explanation for the "our channel went away :-(" assertion:

409 // if we hit this assert, it's likely that the data producer isn't sticking
410 // headers after a token to delineate a new part. This is required. If
411 // the server's not sending those headers, the server's broken.

It seems to suggest that in case of broken servers mPartChannel may be null.
The actual crash is caused by mPartChannel->GetLoadGroup (line 396 in SendStop).
Adding a null check there seems to avoid the crash for me:

> cvs diff nsMultiMixedConv.cpp
Index: nsMultiMixedConv.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/streamconv/converters/nsMultiMixedConv.cpp,v
retrieving revision 1.39
diff -r1.39 nsMultiMixedConv.cpp
395,398c395,399
< 
<     (void) mPartChannel->GetLoadGroup(getter_AddRefs(loadGroup));
<     if (loadGroup) {
<         loadGroup->RemoveChannel(mPartChannel, mContext, NS_OK, nsnull);
---
>     if (mPartChannel) {
>         (void) mPartChannel->GetLoadGroup(getter_AddRefs(loadGroup));
>         if (loadGroup) {
>             loadGroup->RemoveChannel(mPartChannel, mContext, NS_OK, nsnull);
>         }
400d400
<

Jud, is this the right thing to do? Should a fix for this go in for beta2?
I wonder if PDT would reject even a null check workaround for a "topcrash"...

Comment 17

[Judson Valeski]

removing patch keyword. assertions are there to point out that you're about to 
crash (because there's a non-recoverable error), the null check will bury the 
problem.

Comment 18

[Jay Patel [:jay]]

has this been fixed?  it has disappeared from the talkback topcrash list today.

Comment 19

[Judson Valeski]

I checked in some boundary searching changes a couple of days ago. shouldn't 
have been realted, but maybe that helped the situation???

Comment 20

[Andreas Franke (gone)]

On PC/Linux 2000072821, http://216.100.231.10:2230/xxx.jpg still crashes for me.

Comment 21

[Gagan]

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

Comment 22

[Judson Valeski]

need reproducible test case.

Comment 23

[Andreas Franke (gone)]

Does it mean that you have tried to load http://216.100.231.10:2230/xxx.jpg on a
Linux machine with an 8bit depth display, and you did not crash??? Otherwise
please try this.

Comment 24

[Judson Valeski]

how do I set my bit depth to 8bit?

Comment 25

[Andreas Franke (gone)]

In your XF86Config file (somewhere under /etc) should be something like this:

Section "Screen"
    [...]
    Subsection "Display"
        Depth       8

You might have the XF86Setup program that allows you to configure this file,
even while X is running.
If you edit it manually, a brute-force way is this:
- put 8 for your Depth at one such place 
- comment out all other "Screen" and "Display" sections
- restart X
If it actually comes up, run xdpyinfo in a shell to verify. 

Comment 26

[Judson Valeski]

verified. I'm already running w/ an 8-bit display depth.

Comment 27

[leger]

so is this fixed now?

Comment 28

[Andreas Franke (gone)]

If nothing has been done to stop the crash, I'd be very surprised if this is
fixed. Unfortunately, I'm away from the usual equipment till about 8/27, so I
can't test.
Valeski, I take it your last comment means that you don't crash when running
mozilla on your local machine with an 8bit display. Can you try
"ssh some.remote.machine" or something similar and running mozilla there?
It may well be the combination of "remote display" and "8bit depth" that
triggers the crash.

Comment 29

[Judson Valeski]

Can't do anything w/ this until we have an in house reproducible test case.

Comment 30

[Judson Valeski]

I'm attatching a patch that does a few things:
1. some added comments for more clarification in this labrynth.
2. I void out a return value from SendData() as it wasn't, and doesn't need to 
be used.
3. I've changed the SendStop() argument list to pass in the status and status 
string used to pass into the OnStopRequest() callback that SendStop() winds up 
making. This clears up another bug I found while looking at this in which the 
appropriate error info wasn't being passed all the way down to the OnStopReq() 
in the failure case.
4. I've removed two assertions about the mPartChannel being required. These 
assertions can go away because of #5.
5. SendData and SendStop, now protect against a null mPartChannel. I gave up 
trying to determine what the case(s) which could throw the state into a quandry. 
Servers can do too many things to account for in the amount of time I can spend 
on this problem.
6. I consolodated some code in the :OnStopRequest() method that was able to be 
generalized.

SendStop() will graciously fail if a mPartChannel doesn't exist. This is because 
I'm betting that the errant SendStop() that was causing this crash was an edge 
case which immediately followed a previous SendStop() (which is illegal, but was 
*possible*), which meant things were finished up anyway.

SendData() will return an error if the mPartChannel doesn't exist. This 
indicates a parsing error as a result of the server not sending a format we can 
handle. If SendData() doesn't have a mPartChannel, something went horribly 
wrong.

I don't like touching this code at all (it's incredibly fragile), so I would 
greatly appreciate it if people able to reproduce this, applied the patch and 
tested it.

My simple tests of bugzilla (a heavy multipart/mixed user) went fine on windows.

note: I was never able to reproduce this on any platform.

Comment 31

[Judson Valeski]

Attachment: patch to prevent crash

Comment 32

[Judson Valeski]

some sites to test:
http://home.netscape.com/assist/net_sites/mozilla/index.html
http://bugzilla.mozilla.org/
http://fenris.lokigames.com/query.cgi

There are many "live web cams" on the net that use this functionality as well, 
testing those would be a good idea.

Comment 33

[Brendan Eich [:brendan]]

/me waits for rpotts to stamp approval.

/be

Comment 34

[Judson Valeski]

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

Comment 35

[Judson Valeski]

just marked a 4xp bug a dup of this, updating keywords.

Comment 36

[Judson Valeski]

updated patch (disregard previous patch and description, this one take 
precedence)

this patch...
- removes old cruft that has not been needed for months (some includes, the 
IOService (which led to the removal of Init() as that was no longer being used), 
mPartCount, mEndingNewLine, the static Create() function).
- adds more descriptive comments.
- the allocation of "buffer" was resulting in a free of "null" if buffer wasn't 
allocated successfully (that's what ERR_OUT does).
- I've added a check for the first OnDataAvailable() callback. There are servers 
out there that send the first block of data *without* a beginning boundary 
token. This was throwing all the parsing off and leading to crashes. Now if we 
catch this case, we generously insert the boundry token ourselves so the parsing 
thinks it was there all along.
- I replaced some "if (failure) free buffer and return rv" blocks w/ the ERR_OUT 
 macro for cleanliness.
- we were ignoring a failed rv from SendData().
- SendStop() now takes status info as args in order to propegate the appropriate 
status. The impetus for this was that SendStop() is now used properly in the 
OnStopRequest() callback.
- ::OnStopRequest()... We now SendData and SendStop if there's data left over 
when we receive our OnStopRequest() callback. If OnStopReq() is called w/ an 
error, we propegate an OnStart and OnStop combination out to the final listener.
- SendStart, SendData, and SendStop, now all protect against missing 
mPartChannels.

Comment 37

[Judson Valeski]

Attachment: comprehensive patch, see previous comments for description.

Comment 38

[Judson Valeski]

After beating our brains out pouring over this. sr=rpotts.

Comment 39

[Tom Everingham]

verified: 06/27 branch and trunk, linux rh6