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)
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)
1.83 KB,
text/plain
|
Details | |
192 bytes,
text/html
|
Details | |
10.53 KB,
patch
|
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 2•22 years ago
|
||
There is no stylesheet to load... the crash occurs when the stylesheet file doesn't exist.
Comment 3•22 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•22 years ago
|
||
(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•22 years ago
|
||
Comment 6•22 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
Whiteboard: TB11088787M , TB11101579Y
Comment 7•22 years ago
|
||
this (almost certainly) is a regression from bug 119321 (works->throbber spin) and bug 165408 (throbber spin->crash)
Updated•22 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 9•22 years ago
|
||
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•22 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•22 years ago
|
||
this ensure that a notification goes out.
Attachment #99918 -
Attachment is obsolete: true
Comment 12•22 years ago
|
||
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•22 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•22 years ago
|
||
Attachment #100001 -
Attachment is obsolete: true
Comment 15•22 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•22 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•22 years ago
|
||
Comment on attachment 100256 [details] [diff] [review] patch v.3 sr=darin
Attachment #100256 -
Flags: superreview+
Comment 18•22 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
Comment 19•22 years ago
|
||
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•22 years ago
|
||
ouch. this pattern is everywhere. why hasn't it bitten us yet?
Comment 21•22 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•22 years ago
|
||
Fixed on trunk
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 23•22 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
Updated•13 years ago
|
Crash Signature: [@ nsUnicharStreamLoader::OnStopRequest]
Updated•2 months ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•