Bug 28586 covers bad connections and such, but not responses from FTP servers. These errors should be handled in the same manner.

Why does this _block_ bug 28586? Surely it would either depend on it or be merely related to it.

Because bug 28586 is the meta bug

->me. adam doesn't mind.

actually reassigning now. oh, that bodes well.

Been trying to make it more right before starting the patch thing, but perhaps it's better to air this thing out a bit in its current state. This one is a slight departure from the html error pages already checked in by Adam. A few of us discussed taking this path, expecting it to solve some problems (i.e. the urlbar incorrectly showing the address of the chrome:// error page). The html error page is loaded by the docshell in response to an error attempting to load a URI. This patch instead streams content that happens to be an error page back to whoever requested the ftp load. Likely you only want to do this if the loader requests it. I suspect I don't have that quite right. I'm keying off the LOAD_INITIAL_DOCUMENT_URI flag. Also this isn't prevent the browser urlbar from incorrectly showing the url of the error document. I couldn't sneak the new content in without notifying everyone because the only way I can think of to send parameters to the error document was by appending them to the error document url, as Adam had already done. And for that to work the document script has to know about the new url. Another bad thing; I had to add docshell to necko's Makefile's REQUIRES list. That's only because I wanted some load flags when I djinned up the new channel to stream back the error page. But it's still an unfortunate dependency. Anybody feeling love or hate?

I don't mind at all if we go the necko route if it's doable. It would certainly solve some of the issues that cause problems in the docshell side of things. Darin mentioned a good way to do this by setting a flag on the nsIRequest which says to return an error page text if need be (e.g. for documents but not other things). However as nsIRequest is frozen, we might have to consider an nsIRequest2 to define this extra flag and then implement it on ftp and http. So docshell would still be in charge of determining whether it wants error codes and/or error text, but the backend supplies it. The ftp impl will need this flag too since the request could be for an inline image where it would be inappropriate to return error text.

adam: it is ok to define additional loadflags on nsIRequest or nsIChannel. undefined flags must be zero by clients that don't know about them and must be ignored by request/channel implementations that don't know about them.

*** Bug 202527 has been marked as a duplicate of this bug. ***

interface nsIFTPChannel : nsIChannel { + void displayError(in long responseCode, in wstring text); can we put that on nsIFTPEventSink instead? that way, this can be handled by docshell like the other errors.

shouldn't server generated errors be handled the as they are with HTTP? i.e., just make the channel return the error page as if it were the requested page. we'd of course need a way for callers to discern the difference. in HTTP land that's done with nsIHttpChannel::responseSucceeded. but, we need something different here. i've always thought that a success status code (other than NS_OK) could be used to indicate this condition.

more localizable text in necko :( longer-term, I'd like to eliminate stringbundles from necko as far as possible...

ok, i didn't realize that we were talking about content that included localized text. i thought we were only talking about server generated text. i agree that it'd be nice to eliminate use of string bundles in necko.

ah... I don't think FTP can generate an error page with just server-supplied text... though, maybe.

Works on current trunk and FF 1.5.

not for me, ftp://ftp.mozilla.org/foo still shows a dialog.

Sorry about that; I should have been more careful.

Dan M's patch (attachment 119239 [details] [diff] [review]) is localizing per ftp response code. + switch (aResponseCode) { + case 530: + error.Assign(NS_LITERAL_STRING("noAnonymous")); + break; + case 550: + error.Assign(NS_LITERAL_STRING("noDirectory")); + break; + default: + error.Assign(NS_LITERAL_STRING("serverError")); + } This would be not only user-friendly but also robust. However, why only 530 and 550? If we are to cover all the FTP errors, that will require a lot of l10n work. Is this the way to go?

Oops, I forgot to change uuid of nsIFTPChannel...

Comment on attachment 614727 [details] [diff] [review] Patch v2 Part1 Suppress generic server-error dialog +++ b/netwerk/protocol/ftp/nsFtpConnectionThread.cpp + // Save the last response message so as to display it later. + mChannel->SetErrorMessageFromServer(mResponseMsg); this doesn't have to be inside the if - no harm in always storing the error message on the channel, right? + prompter->Alert(nsnull, + NS_ConvertASCIItoUTF16(mResponseMsg).get()); honestly, I feel there is no need to do this. if this is for document or iframe, docshell will show the error page. otherwise, no need to bother the user with a dialog. + CopyASCIItoUTF16(externalMessage, formatStrs[1]); don't we have a method somewhere for html-escaping the string? That avoids the spoof issue you mention below.

Comment on attachment 614304 [details] [diff] [review] Patch v1 Part2 Suppress "unsupported server" dialog + // Suppress error dialog when this request is document. + if (mChannel->HasLoadFlag(nsIChannel::LOAD_DOCUMENT_URI)) + return FTP_ERROR; again, I don't feel we need the alert() in necko anymore. that should let you remove some strings from necko.properties, too.

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #24) > this doesn't have to be inside the if - no harm in always storing the error > message on the channel, right? You are right. > honestly, I feel there is no need to do this. if this is for document or > iframe, docshell will show the error page. otherwise, no need to bother the > user with a dialog. OK. > don't we have a method somewhere for html-escaping the string? That avoids > the spoof issue you mention below. Basically the spoof seems not to happen. DOM |element.textContent| automatically converts "<" to "&amp;" on substitution. However, according to <about:neterror>, its script intentionally do HTML-*unescape* on SSL error. > // Rather than textContent, we need to treat description as HTML ... > var re = /<a id="cert_domain_link" title="([^"]+)">/; > var result = re.exec(desc); ... > // Now create the link itself > var anchorEl = document.createElement(" In other words, the SSL error's description is trusted. That makes me feel slightly uneasy. But, well, there's no big problem as long as developers are careful enough, I think. I removed the xxx comment. http://mxr.mozilla.org/mozilla-central/source/docshell/resources/content/netError.xhtml#195 http://mxr.mozilla.org/mozilla-central/source/security/manager/locales/en-US/chrome/pipnss/pipnss.properties#320

Please don't use unsupportedFTPServer2 when deleting entries at one place and generating new entries elsewhere. Also, the entity keys don't really match the actual strings, and I'm not sure the strings are as good as they can get. Maybe ask one of our writers for help, or UX?

(In reply to Axel Hecht [:Pike] from comment #28) > Also, the entity keys don't really match the actual strings, and I'm not > sure the strings are as good as they can get. Maybe ask one of our writers > for help, or UX? Exactly whom are you talking about? And would you suggest proper phrases instead?

I'm not sure I have constructive comments just yet, and I don't think I know the error conditions of ftp well enough to find some. Same for the UX/writers that we have, probably. a few more concrete notes: <!ENTITY unsupportedFTPServer2.title "FTP connection aborted"> <!ENTITY unsupportedFTPServer2.longDesc "<p><ul><li>Maybe this type of FTP server is not commonly used.</li></ul></p>"> The phrases should be consistent. You probably know best which one is right, or generic enough. Similarly for the other string, the server said "cannot change directory" and the message talks about file names. Maybe leave the long desc empty, if there's nothing helpful to be said about ftp errors.

Comment on attachment 644657 [details] [diff] [review] Patch v3 Part1 Suppress generic server-error dialog True, this does need ui-review from someone

Comment on attachment 644663 [details] [diff] [review] Patch v3 Part2 Suppress "unsupported server" dialog - nsCOMPtr<nsIStringBundle> bundle; - nsresult rv = bundleService->CreateBundle(NECKO_MSGS_URL, - getter_AddRefs(bundle)); can maybe remove some #includes? r=biesi but do get ui-review on the strings and listen to Axel on l10n details