Closed Bug 169214 Opened 22 years ago Closed 22 years ago

Viewing an HTML page with a missing CSS file via FTP crashes mozilla [@ nsUnicharStreamLoader::OnStopRequest]

Categories

(Core Graveyard :: Networking: FTP, defect)

x86
Linux
defect
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.2beta

People

(Reporter: fphan, Assigned: dougt)

References

()

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(3 files, 2 obsolete files)

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?
Keywords: crash
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
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: stackwanted
Whiteboard: TB11088787M , TB11101579Y
Attached file 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
Attached file testcase
trunk build 2002082704 works fine
trunk build 2002082722 and 2002092904s pins the throbber forever after failing
to load the css
trunk build 2002093005 crashes
Whiteboard: TB11088787M , TB11101579Y
this (almost certainly) is a regression from bug 119321 (works->throbber spin)
and bug 165408 (throbber spin->crash)
Summary: Viewing an HTML page with a missing CSS file via FTP crashes mozilla → Viewing an HTML page with a missing CSS file via FTP crashes mozilla [@ nsUnicharStreamLoader::OnStopRequest]
ha!  i love that assertion.
Target Milestone: --- → mozilla1.2beta
Attached patch patch v.1 (obsolete) — Splinter Review
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.
Attachment #99918 - Flags: needs-work+
Attached patch patch v.2 (obsolete) — Splinter Review
this ensure that a notification goes out.
Attachment #99918 - Attachment is obsolete: true
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.
Attached patch patch v.3Splinter Review
Attachment #100001 - Attachment is obsolete: true
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
Attachment #100256 - Flags: superreview+
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
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
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsUnicharStreamLoader::OnStopRequest]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: