683959 - crash [@ nsFtpState::R_retr() ]

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[IU]

Attachment: stack trace

When clicking the second FTP link for GnuPG 2.0, at http://www.gnupg.org/download/index.en.html, I got this crash.

The first crash happened immediately, the first time I clicked the second link (after clicking the first link and canceling), but subsequent crashes takes many tries before I'm able to get it to crash again.  To get it to crash, I basically clicked the FTP link.  If it does not crash, I cancel the save dialog and keep trying.

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsFtpState::R_retr 	netwerk/protocol/ftp/nsFtpConnectionThread.cpp:1202
1 	xul.dll 	nsFtpState::Process 	netwerk/protocol/ftp/nsFtpConnectionThread.cpp:593
2 	xul.dll 	nsFtpState::OnControlDataAvailable 	netwerk/protocol/ftp/nsFtpConnectionThread.cpp:223
3 	xul.dll 	nsFtpControlConnection::OnInputStreamReady 	netwerk/protocol/ftp/nsFtpControlConnection.cpp:93
4 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:114
5 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631
6 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
7 	xul.dll 	xul.dll@0xbbbac3 	
8 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:201
9 	xul.dll 	_SEH_epilog4 	
10 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:175
11 	xul.dll 	nsThreadManager::GetCurrentThread 	xpcom/threads/nsThreadManager.cpp:218
12 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
13 	xul.dll 	xul.dll@0xbbbac3 	
14 	xul.dll 	nsAppShell::Run 	widget/src/windows/nsAppShell.cpp:261

Comment 1

[IU]

Crash URL: https://crash-stats.mozilla.com/report/index/bp-48790685-1110-4068-aae0-b7b7f2110901

Comment 2

[Hari R]

Unable to reproduce even after clicking tens of times in an x86_64 Linux 3.0 SMP environment.

I do not have a Windows dev env. Can someone who does please try and reproduce this?

Comment 3

[quazgar]

I can confirm this with FF 12.0 on WinXP in a VirtualBox environment (Linux as host system). Tryong to open ftp://ftp.gnupg.org/gcrypt/gnupg/gnupg-2.0.19.tar.bz2.sig crashes Firefox there.

Comment 4

[Josh Matthews [:jdm]]

This might just need a null check for mDataStream, since every other use of it has one.

Comment 5

[quazgar]

(In reply to daniel.hornung from comment #3)
> I can confirm this with FF 12.0 on WinXP in a VirtualBox environment (Linux
> as host system).

I have to retract this comment, since the problem in my case seems to be with VirtualBox: https://www.virtualbox.org/ticket/4478

Comment 6

[IU]

(In reply to daniel.hornung from comment #5)
> (In reply to daniel.hornung from comment #3)
> I have to retract this comment, since the problem in my case seems to be
> with VirtualBox: https://www.virtualbox.org/ticket/4478

You seem to be conflating Firefox with a VM guest, for some reason.  Is it Firefox that crashed or Windows?  If Firefox, you should get the same crash signature in this summary, otherwise you're experiencing a different issue.

Comment 7

[quazgar]

(In reply to IU from comment #6)Is it
> Firefox that crashed or Windows?  If Firefox, you should get the same crash
> signature in this summary, otherwise you're experiencing a different issue.

The VirtualBox environment crashes, I just thought it was improper handling of FF crashing plus improper handling of this of both Win and VB, but it seems it was only/mainly VB in this case, since the error was not limited to FF. So, it's a different issue and not FF's fault.

Comment 8

[Wayne Mery (:wsmwk)]

UI, can you still reproduce this?

quite rare - only 3 version 14 crashes in past month. for example: bp-4cfd052b-ccd5-41f4-bbc7-725d42120723

Comment 9

[IU]

(In reply to Wayne Mery (:wsmwk) from comment #8)
> UI, can you still reproduce this?
> 
> quite rare - only 3 version 14 crashes in past month. for example:
> bp-4cfd052b-ccd5-41f4-bbc7-725d42120723

Yes it is still reproducible: https://crash-stats.mozilla.com/report/index/bp-b563dcdc-a4c1-4761-b70a-683502120807

As noted in comment 0, it takes some trying to reproduce.

That said, if the resolution stated in comment 4 would resolve this bug, why isn't it done?  Certainly can't be a performance concern.

Comment 10

[Josh Matthews [:jdm]]

For anybody who wants to attempt this, first make sure you can reproduce the bug, then try adding null checks to http://hg.mozilla.org/mozilla-central/annotate/c8d94fe7506a/netwerk/protocol/ftp/nsFtpConnectionThread.cpp#l1188 and http://hg.mozilla.org/mozilla-central/annotate/c8d94fe7506a/netwerk/protocol/ftp/nsFtpConnectionThread.cpp#l1146 and see if you that fixes the crash.

Comment 11

[Srinath N [:srinath]]

Hi Josh, 

Per our discussion, I've put together a patch for this issue. Can you please assign the defect to me?

Comment 12

[Srinath N [:srinath]]

Attachment: Patch for bug 683959 - Adding null checks for mDataStream

Patch for bug 683959 - Adding null checks for mDataStream

Comment 13

[Srinath N [:srinath]]

Hi IU,

I tested this using a Nightly on Win 8 and Win XP. I followed the steps as mentioned in the defect. I have not been able to reproduce the crash.

Per jdm's suggestion, I've made changes and have attached the patch to the defect. 

Can you kindly apply the patch and see if you are able to observe the crash?

Thanks in advance!

Comment 14

[Josh Matthews [:jdm]]

You can get builds with the patch applied at https://tbpl.mozilla.org/?tree=Try&rev=a537cf62103f.

Comment 15

[IU]

(In reply to Josh Matthews [:jdm] from comment #14)
> You can get builds with the patch applied at
> https://tbpl.mozilla.org/?tree=Try&rev=a537cf62103f.

I can't run this build because Windows is complaining that I'm missing MSVCR100D.dll. I even installed Microsoft Visual C++ 2010 SP1 Redistributable Package and that did not help.

Is it possible to get a normal non-debug try build?

Comment 16

[Josh Matthews [:jdm]]

It should be on its way at https://tbpl.mozilla.org/?tree=Try&rev=fbdba9064d66.

Comment 17

[IU]

Thanks.  I can confirm that the patch resolves the crashes.

Comment 18

[Jason Duell]

Comment on attachment 681804 [details] [diff] [review]
Patch for bug 683959 - Adding null checks for mDataStream

Thanks for the patch! I've landed it:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/76857b6b13e6

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76857b6b13e6