157531 - FTP errors should also use error page instead of dialog

Status: RESOLVED WORKSFORME

(Core :: DOM: Navigation, defect)

Description

[Aaron Kaluszka]

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

Comment 1

[Hixie (not reading bugmail)]

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

Comment 2

[Adam Lock]

Because bug 28586 is the meta bug

Comment 3

[Dan M]

->me. adam doesn't mind.

Comment 4

[Dan M]

actually reassigning now. oh, that bodes well.

Comment 5

[Dan M]

Attachment: preliminary implementation, more a discussion thing than a checkin thing

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?

Comment 6

[Adam Lock]

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.

Comment 7

[Darin Fisher]

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.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

 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.

Comment 10

[Darin Fisher]

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.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 12

[Darin Fisher]

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.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 14

[Eli Friedman]

Works on current trunk and FF 1.5.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 16

[Eli Friedman]

Sorry about that; I should have been more careful.

Comment 18

[O. Atsushi (Torisugari)]

Attachment: Patch v1 Part1 Suppress generic server-error dialog

Comment 19

[O. Atsushi (Torisugari)]

Attachment: Patch v1 Part2 Suppress "unsupported server" dialog

Comment 20

[O. Atsushi (Torisugari)]

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?

Comment 21

[O. Atsushi (Torisugari)]

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

Comment 22

[O. Atsushi (Torisugari)]

Attachment: Patch v2 Part1 Suppress generic server-error dialog

Comment 23

[O. Atsushi (Torisugari)]

Attachment: Screenshot v1

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

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 25

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 26

[O. Atsushi (Torisugari)]

Attachment: Patch v3 Part1 Suppress generic server-error dialog

(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

Comment 27

[O. Atsushi (Torisugari)]

Attachment: Patch v3 Part2 Suppress "unsupported server" dialog

Comment 28

[Axel Hecht [:Pike]]

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?

Comment 29

[O. Atsushi (Torisugari)]

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

Comment 30

[Axel Hecht [:Pike]]

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 31

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 644657 [details] [diff] [review]
Patch v3 Part1 Suppress generic server-error dialog

True, this does need ui-review from someone

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 33

[Valentin Gosu [:valentin] (he/him)]

FTP code has been removed.