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

VERIFIED FIXED in mozilla1.2beta

Status

()

Core
Networking: FTP
--
critical
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Frederic Phan, Assigned: dougt)

Tracking

({crash, regression, testcase})

Trunk
mozilla1.2beta
x86
Linux
crash, regression, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
(not knowing a lot about css...)

What happens if you load the css stylesheet directly, from URL bar, does it work?
Keywords: crash
(Reporter)

Comment 2

15 years ago
There is no stylesheet to load... the crash occurs when the stylesheet file
doesn't exist.

Comment 3

15 years ago
Confirming bug, 2002-09-13-10 trunk Linux.   Talkback: TB11101579Y
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: stackwanted
Whiteboard: TB11088787M , TB11101579Y

Comment 4

15 years ago
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

15 years ago
Created attachment 99623 [details]
testcase

Comment 6

15 years ago
trunk build 2002082704 works fine
trunk build 2002082722 and 2002092904s pins the throbber forever after failing
to load the css
trunk build 2002093005 crashes
Keywords: stackwanted → regression, testcase
Whiteboard: TB11088787M , TB11101579Y

Comment 7

15 years ago
this (almost certainly) is a regression from bug 119321 (works->throbber spin)
and bug 165408 (throbber spin->crash)

Updated

15 years ago
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]
(Assignee)

Comment 8

15 years ago
ha!  i love that assertion.
Target Milestone: --- → mozilla1.2beta
(Assignee)

Comment 9

15 years ago
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.
(Assignee)

Comment 10

15 years ago
Comment on attachment 99918 [details] [diff] [review]
patch v.1

cause the data socket might not already be open.
Attachment #99918 - Flags: needs-work+
(Assignee)

Comment 11

15 years ago
Created attachment 100001 [details] [diff] [review]
patch v.2

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?
(Assignee)

Comment 13

15 years ago
> 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.
(Assignee)

Comment 14

15 years ago
Created attachment 100256 [details] [diff] [review]
patch v.3
Attachment #100001 - Attachment is obsolete: true

Comment 15

15 years ago
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?
(Assignee)

Comment 16

15 years ago
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

15 years ago
Comment on attachment 100256 [details] [diff] [review]
patch v.3

sr=darin
Attachment #100256 - Flags: superreview+

Comment 18

15 years ago
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.
(Assignee)

Comment 20

15 years ago
ouch.  this pattern is everywhere.  why hasn't it bitten us yet?

Comment 21

15 years ago
bz: good catch!  we must just be lucky, or perhaps there are real regressions on
some platforms, and we just don't know it :-/
(Assignee)

Comment 22

15 years ago
Fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 23

15 years ago
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]
You need to log in before you can comment on or make changes to this bug.