Last Comment Bug 169214 - Viewing an HTML page with a missing CSS file via FTP crashes mozilla [@ nsUnicharStreamLoader::OnStopRequest]
: Viewing an HTML page with a missing CSS file via FTP crashes mozilla [@ nsUni...
Status: VERIFIED FIXED
: crash, regression, testcase
Product: Core
Classification: Components
Component: Networking: FTP (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: mozilla1.2beta
Assigned To: Doug Turner (:dougt)
: benc
Mentors:
ftp://ftp.irisa.fr/pub/gnu/gnugo/inde...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-09-17 10:56 PDT by Frederic Phan
Modified: 2002-12-02 14:19 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stacktrace (1.83 KB, text/plain)
2002-09-17 20:15 PDT, Andrew Schultz
no flags Details
testcase (192 bytes, text/html)
2002-09-17 20:22 PDT, Andrew Schultz
no flags Details
patch v.1 (7.64 KB, patch)
2002-09-19 19:17 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
patch v.2 (8.21 KB, patch)
2002-09-20 11:42 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
patch v.3 (10.53 KB, patch)
2002-09-23 11:35 PDT, Doug Turner (:dougt)
darin.moz: superreview+
Details | Diff | Splinter Review

Description Frederic Phan 2002-09-17 10:56:46 PDT
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
Comment 1 benc 2002-09-17 13:45:46 PDT
(not knowing a lot about css...)

What happens if you load the css stylesheet directly, from URL bar, does it work?
Comment 2 Frederic Phan 2002-09-17 13:55:26 PDT
There is no stylesheet to load... the crash occurs when the stylesheet file
doesn't exist.
Comment 3 Mats Palmgren (vacation) 2002-09-17 14:39:06 PDT
Confirming bug, 2002-09-13-10 trunk Linux.   Talkback: TB11101579Y
Comment 4 Andrew Schultz 2002-09-17 20:15:05 PDT
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
Comment 5 Andrew Schultz 2002-09-17 20:22:00 PDT
Created attachment 99623 [details]
testcase
Comment 6 Andrew Schultz 2002-09-17 20:38:30 PDT
trunk build 2002082704 works fine
trunk build 2002082722 and 2002092904s pins the throbber forever after failing
to load the css
trunk build 2002093005 crashes
Comment 7 Andrew Schultz 2002-09-17 21:05:57 PDT
this (almost certainly) is a regression from bug 119321 (works->throbber spin)
and bug 165408 (throbber spin->crash)
Comment 8 Doug Turner (:dougt) 2002-09-18 17:25:36 PDT
ha!  i love that assertion.
Comment 9 Doug Turner (:dougt) 2002-09-19 19:17:18 PDT
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 10 Doug Turner (:dougt) 2002-09-20 11:09:56 PDT
Comment on attachment 99918 [details] [diff] [review]
patch v.1

cause the data socket might not already be open.
Comment 11 Doug Turner (:dougt) 2002-09-20 11:42:00 PDT
Created attachment 100001 [details] [diff] [review]
patch v.2

this ensure that a notification goes out.
Comment 12 Boris Zbarsky [:bz] 2002-09-20 17:20:07 PDT
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?
Comment 13 Doug Turner (:dougt) 2002-09-20 17:24:55 PDT
> 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.
Comment 14 Doug Turner (:dougt) 2002-09-23 11:35:29 PDT
Created attachment 100256 [details] [diff] [review]
patch v.3
Comment 15 Darin Fisher 2002-09-25 08:45:02 PDT
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?
Comment 16 Doug Turner (:dougt) 2002-09-30 10:23:17 PDT
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 17 Darin Fisher 2002-09-30 11:10:37 PDT
Comment on attachment 100256 [details] [diff] [review]
patch v.3

sr=darin
Comment 18 rpotts (gone) 2002-09-30 12:53:38 PDT
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.  r=rpotts@netscape.com after verifying that the
reference counting is correct :-)

-- rick
Comment 19 Boris Zbarsky [:bz] 2002-09-30 13:20:01 PDT
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.
Comment 20 Doug Turner (:dougt) 2002-09-30 13:26:06 PDT
ouch.  this pattern is everywhere.  why hasn't it bitten us yet?
Comment 21 Darin Fisher 2002-09-30 13:38:51 PDT
bz: good catch!  we must just be lucky, or perhaps there are real regressions on
some platforms, and we just don't know it :-/
Comment 22 Doug Turner (:dougt) 2002-09-30 15:09:32 PDT
Fixed on trunk
Comment 23 benc 2002-12-02 14:19:04 PST
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.

Note You need to log in before you can comment on or make changes to this bug.