User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2a) Gecko/20020917 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2a) Gecko/20020917 When trying to view an HTML file on an FTP server, if the HTML file has a <link rel=stylesheet href=file> and the stylesheet file doesn't exist, mozilla crashes after displaying an error 550 message box. Reproducible: Always Steps to Reproduce: 1.Connect to an FTP server 2.Click on an HTML file with a link to a stylesheet that doesn't exist 3.Click OK when seeing the error message box Actual Results: Mozilla crashes. Expected Results: Mozilla should have displayed the page with no style. Talkback incident ID: TB11088787M
(not knowing a lot about css...) What happens if you load the css stylesheet directly, from URL bar, does it work?
There is no stylesheet to load... the crash occurs when the stylesheet file doesn't exist.
Confirming bug, 2002-09-13-10 trunk Linux. Talkback: TB11101579Y
Created attachment 99622 [details] stacktrace (CVS 2002-9-17) WARNING: CSSLoaderImpl::DidLoadStyle: Load of URL 'ftp://ftp.irisa.fr/html/default.css' failed. Error code: 22, file nsCSSLoader.cpp, line 1262 ###!!! ASSERTION: No way we can not have an mObserver here!: 'mObserver', file nsUnicharStreamLoader.cpp, line 150 (this leads to the crash) ###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../../dist/include/xpcom/nsCOMPtr.h, line 650
trunk build 2002082704 works fine trunk build 2002082722 and 2002092904s pins the throbber forever after failing to load the css trunk build 2002093005 crashes
this (almost certainly) is a regression from bug 119321 (works->throbber spin) and bug 165408 (throbber spin->crash)
ha! i love that assertion.
Created attachment 99918 [details] [diff] [review] patch v.1 why do we go through all of that trouble of checking to see if there was a notificaiton when we can just force the notification by cancelling the transport. This needs some major testing before we think about landing it. I am still kinda thinking out-loud.
Comment on attachment 99918 [details] [diff] [review] patch v.1 cause the data socket might not already be open.
Created attachment 100001 [details] [diff] [review] patch v.2 this ensure that a notification goes out.
So was the problem here that OnStopRequest was getting called twice, then? With this patch, will calling Cancel() on an open FTP channel with data coming through it trigger an OnStopRequest() call to the channel's observer?
> So was the problem here that OnStopRequest was getting called twice, then? right. > With this patch, will calling Cancel() on an open FTP channel with data coming through it trigger an OnStopRequest() call to the channel's observer? yes. I still would like to do alot more testing before landing this patch.
Created attachment 100256 [details] [diff] [review] patch v.3
Comment on attachment 100256 [details] [diff] [review] patch v.3 >Index: nsFTPChannel.cpp >- mStatus = aStatus; >- >+ if (NS_SUCCEEDED(mStatus)) >+ mStatus = aStatus; how can this be right? won't socket level errors appear like successful loads?
A Cancel() on the loadgroup will filter through to the nsFTPChannel setting the mStatus to whatever error code. If a socket level error occurs, it is filtered up through the nsFtpConnectionThread. If mStatus was a failure at the point where we reach OnStartRequest, then we will use the mStatus (ie. if you cancel then get a socket failure, we use the cacel error code). The nsFtpConnectionThread never calls cancel directly on the nsFTPChannel.
Comment on attachment 100256 [details] [diff] [review] patch v.3 sr=darin
hey doug, are you sure that this does the 'right thing' wrt reference counts? i know that at one time, using the same COMPtr for both in and out caused an imbalance in the ref counting... Maybe we're safe here because it isn't the 'last reference'. + + nsCOMPtr<nsIRequestObserver> asyncObserver = do_QueryInterface(mChannel); + NS_NewRequestObserverProxy(getter_AddRefs(asyncObserver), + asyncObserver, + NS_CURRENT_EVENTQ); there is a typo here... 'while' should be which + // The forwarding object was never created while means that we never sent our notifications. Otherwise it looks good. firstname.lastname@example.org after verifying that the reference counting is correct :-) -- rick
Um.. reference counting aside, getter_AddRefs will null out the pointer. And evaluation order of arguments is undefined. Which means that on some compilers the second arg being passed in there will always be null. Please don't do that.
ouch. this pattern is everywhere. why hasn't it bitten us yet?
bz: good catch! we must just be lucky, or perhaps there are real regressions on some platforms, and we just don't know it :-/
Fixed on trunk
VERIFIED: confirmed crash in 1.2a, fixed in 1.2b. Interestingly, Windows would crash on shutdown, not immediately, so it might explain some unverifiable crashers people have been reporting.