343371 - page with ftp:// URLs and javascript crashes firefox [@ nsFtpState::StopProcessing]

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[moof]

The specified page on archive.org, presumably due to the javascript trying to do something with fetching the nonexistent URLs, causes firefox to crash. This happens not only on Bon Echo, but on Deer Park 1.5.0.4 as well, regardless of OS.

Comment 1

[Reed Loden [:reed]]

WFM on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060701 BonEcho/2.0a3 ID:2006070113

You give no specifics on what builds you are using, which means your description is completely useless.

Comment 2

[Adam Guthrie]

If it's reproducible it'd be nice to have a Talkback ID for the crash. http://kb.mozillazine.org/Talkback

Comment 3

[moof]

Talkback incident ID TB20496473Y.
However, the crash occurs on NetBSD FireFox 1.5.0.4, WinXP FireFox 1.5.0.4, and WinXP Bon Echo 2.0a2 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a2) Gecko/20060512 BonEcho/2.0a2

Comment 4

[Adam Guthrie]

Incident ID: 20496473
Stack Signature	nsFtpState::StopProcessing 75945868
Product ID	Firefox15
Build ID	2006050817
Trigger Time	2006-07-01 20:22:17.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	firefox.exe + (000903d2)
URL visited	http://web.archive.org/web/20041011002607/http://vax.sevensages.org/swdist/bsd/bdc/VAX-netboot-HOWTO-3.html
User Comments	Just loading the page. See https://bugzilla.mozilla.org/show_bug.cgi?id=343371
Since Last Crash	589 sec
Total Uptime	589 sec
Trigger Reason	Access violation
Source File, Line No.	c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp, line 2441
Stack Trace 	
nsFtpState::StopProcessing  [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp, line 2441]
nsFtpState::Process  [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp, line 751]

Comment 5

[moof]

And the Bon Echo 2.0a3 crash: TB20801560H (still line 2441, as in comment #4)

Comment 6

[Adam Guthrie]

This works for me on current trunk. (It looks like there's been some major reworking in that part of the tree.)

mChannel is null for some reason when we try to use it, but I guess it was non-null when we used it earlier in this function to give the alert you're seeing.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp&rev=1.296.8.1&mark=2402,2441#2400

Comment 7

[moof]

Aaaaaand 2.0b1: TB21254682M

Comment 8

[Marek Stępień [:marcoos, inactive]]

Reproducible on Linux as well.

http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=TB22268577Y

Just click here to crash:
http://web.archive.org/web/20000824075351/http://portfolio.art.pl/mozilla

Comment 9

[Martijn Wargers (dead)]

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

Comment 10

[Martijn Wargers (dead)]

Attachment: patch

So is this acceptable to fix the crash on branches?

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

OK, I only crash if I wait for a while while the dialog is up until I press OK.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

some debug output:
FTP 0x8a2ea80: Alerting '550 Failed to change directory.
'
FTP 0x8a2ea80: Alerting '550 Failed to change directory.
'
(the same nsFtpState!)

Here I click OK in one dialog

FTP 0x8a2ea80: Calling OnStatus
FTP 0x8a2ea80: Releasing chan
(output from other ftpstates trimmed)

And then I click OK in the second dialog, and here's the crash:
WARNING: NS_ENSURE_TRUE(observer) failed, file ../../../../mozilla/netwerk/base/src/nsRequestObserverProxy.cpp, line 296
FTP 0x8a2ea80: Calling OnStatus
###!!! ASSERTION: You can't dereference a NULL nsRefPtr with operator->().: 'mRawPtr != 0', file ../../../../dist/include/xpcom/nsAutoPtr.h, line 1063

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

the stack trace is quite awesome here. The second dialog is caused by this: While the dialog is up, we process events. That causes a timer to fire, which causes GC. That destroys a DOM node, which calls imgRequestProxy::Cancel, and here we call the FTP channel's cancel method again, which calls nsFtpState::Cancel, which reenters StopProcessing.

Cool, isn't it?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 236784 [details] [diff] [review]
patch

that means that this patch is wrong because it still shows two dialogs where we should show only one.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

not an issue on trunk because StopProcessing ensures early that it can't be reentered

I'm working on another version of the branch patch

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

So this patch does two things:
- make this function set mKeepRunning to false at the beginning (like trunk)
- moves showing the dialog to the end of the function, after updating the member variables

I felt it was safer to do both of those.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 236873 [details] [diff] [review]
patch v2

this is a branch-only patch, so it can't land on trunk, but it makes the branch look a bit more like trunk

it only moves code around a bit, it should be pretty low risk, and fixes a crash.

Comment 18

[Mike Schroepfer]

Comment on attachment 236873 [details] [diff] [review]
patch v2

a=schrep for drivers for crash fix.

Comment 19

[Adam Guthrie]

Maybe we could get a unit test in netwerk/ using Martijn's testcase in attachment 236642 [details].

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Hm... there are a few problems with making this a unit test: the testcase as written depends on having a DOM and also on having UI; and it depends on having an FTP server available. that's all not so nice for a unit test...

fixed on MOZILLA_1_8_BRANCH:
Checking in nsFtpConnectionThread.cpp;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v  <--  nsFtpConnectionThread.cpp
new revision: 1.296.8.2; previous revision: 1.296.8.1
done

Comment 21

[Adam Guthrie]

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

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 236873 [details] [diff] [review]
patch v2

since this fixes a crash, I think it would be good on the 1.8.0 branch as well. It's been on the 1.8 branch for 9 days now and no regressions have been reported. As described in comment 17, the risk should be pretty low.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 236873 [details] [diff] [review]
patch v2

approved for 1.8.0 branch, a=dveditz for drivers

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in nsFtpConnectionThread.cpp;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v  <--  nsFtpConnectionThread.cpp
new revision: 1.296.8.1.2.2; previous revision: 1.296.8.1.2.1
done

Comment 25

[Adam Guthrie]

Making a testcase for this is much more viable now that we have the mochitest framework. The only additional thing we'd need is an FTP server running on the machine that runs the tests, I think.

Comment 26

[Carsten Book [:Tomcat]]

Verified fixed for 1.8.0.9 and 1.8.1.1. No crash on URL with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.1pre) Gecko/20061129 BonEcho/2.0.0.1pre
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.9pre) Gecko/20061130 Firefox/1.5.0.9pre also on Vista and on Fedora FC 6