209328 - Mailto: links are not recognized and are not working properly

Status: VERIFIED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[Charles Fenwick]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5a) Gecko/20030613
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5a) Gecko/20030613

When putting the cursor over a mailto: link, only a : is displayed in the status
bar.

Clicking on a mailto: link brings up a window reading
 "ALERT 
         is not a registered protocol "


Reproducible: Always

Steps to Reproduce:
1.Find any e-mail link (for example [e-mail])
2.Put cursor over link
3.Click on the link

Actual Results:  
2. : displayed in status bar
3. Alert message popped up

Expected Results:  
2. Displayed full e-mail link
3. Opened a message composition window.

Comment 1

[Jason M. Abels]

This works on the 2003061204 nightly build (the 08 build was pulled).

It also works on the 2003061210 1.4 branch build, so this shouldn't affect 1.4.

I can confirm that this happens on 2003061304 WinXP

Comment 2

[Benjamin Smedberg]

Charles: is this a self-build or a nightly build? do you have mozilla mailnews
installed or not? how did you install (a zipfile or the installer)?

Comment 3

[Mats Palmgren (inactive)]

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

Comment 4

[Charles Fenwick]

Benjamin,

I installed it using the file from
http://ftp.mozilla.org/pub/mozilla/nightly/latest/mozilla-win32-installer-sea.exe

Everything is installed.

Comment 5

[Mats Palmgren (inactive)]

Bug also occurs in Mozilla nightly trunk build 2003-06-12-22 on Linux

Comment 6

[Jason M. Abels]

My build:

This is a build using the mozilla installer (so it's a prefab build). I have
mailnews installed, and it's the default mail program.

Comment 7

[Jason M. Abels]

This was a fresh install, btw.

I also tried creating a new profile, and that had no effect.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

jshin: your checking touched the external handler stuff.  any chance it could be
related?

Comment 9

[Benjamin Smedberg]

From bug 167265?

Comment 10

[Jungshik Shin]

Possibly because it touches nsSmtpService.cpp. I'll figure it out. Thanks for
the alert. 

Comment 11

[Mats Palmgren (inactive)]

It seems it was the removal of the IsEmpty check that caused this,
the diff below fixes it for me:

diff -u -r1.124 nsSmtpService.cpp
--- nsSmtpService.cpp   12 Jun 2003 21:57:38 -0000      1.124
+++ nsSmtpService.cpp   13 Jun 2003 22:53:42 -0000
@@ -324,7 +324,7 @@
           rv = utf8Converter->ConvertURISpecToUTF8(aSpec, aOriginCharset,
utf8Spec);
     }

-    if (NS_SUCCEEDED(rv))
+    if (NS_SUCCEEDED(rv) && !utf8Spec.IsEmpty())
       mailtoUrl->SetSpec(utf8Spec);
     else
       mailtoUrl->SetSpec(aSpec);

Comment 12

[Jungshik Shin]

That's  odd because nsUTF8Converter->ConvertURISpecToUTF8 always fills up
utf8Spec when returning NS_OK (its static predecessor in the previous version of
nsSmtpService.cpp returns NS_OK not filling up utf8Spec when aSpec contains
non-ASCII chars.). I reread my code and that's still my reading. I must be
missing something obvious here... I'm still waiting for my build to complete
(for other bugs, I had begun clobber'n'build before I knew about this bug)

Comment 13

[Jungshik Shin]

I came up with an equivalent but different patch. To find out what's going on, I
had to have a bit more context :-) nsUTF8Con...->ConvertURISpecToUTF8 (that
fills up utf8Spec) is only invoked when aOriginCharset is not null. Therefore,
if-statement should be |if (aOriginCharset && NS_SUCCEEDED(rv)) ...| I prefer
this to |if (NS_SUCCEEDED(rv) && !utf8Spec.IsEmpty())| because
|nsACString.IsEmpty()| must be slower than null check and this way I can also
short-circuit the case when |ConvertURISpecToUTF8| is not called (i.e.
aOriginCharset is null). I'll upload my patch shortly. Thanks, Mats, for your
patch, anyway.

 

Comment 14

[Jungshik Shin]

Attachment: a patch

It took a bit more work than I thought. url-escaping routine doesn't work the
way I thought it does.

Comment 15

[Jungshik Shin]

assign to myself. Change the product to mailnews. I couldn't set the component,
though. 

Comment 16

[Jungshik Shin]

Comment on attachment 125614 [details] [diff] [review]
a patch

asking r for Naoki (beccause  that part was originally written by him) and Alec
(because this is a 'fallout' of bug 162765). Sorry for tabs (the file is
littered with tabs and I didn't touch them ;-) )

Comment 17

[Jungshik Shin]

   if (....) {
     ... return ...;
   }
-  if (IsASCII(unescapedSpec) || IsUTF8(unescapedSpec)) {
+  else if (IsASCII(unescapedSpec) || IsUTF8(unescapedSpec)) {

  I'll remove 'else'.

Comment 18

[Hermann Schwab]

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

Comment 19

[nhottanscp]

Jungshik, I need some info about the patch.
I think that part of the code unescapes the URI and convert to UTF-8 because the
mail compose code expects the URI as UTF-8. How is that related to the problem
of this bug? 
I don't know much about the code in nsUTF8ConverterService.cpp. Is that a function?

Comment 20

[Jungshik Shin]

Naoki, thanks for looking at it. Here's the explanation :-)

nsUTF8ConverterService::ConvertURISpecToUTF8() is very similar to what you used
to have in nsStmpService.cpp(|EnsureUTF8Spec|) before my patch to bug 162765
went in. However, it's different a little from your code and that difference 
led to this bug. 

In your old code (the name of the function was |EnsureUTF8Spec|), out parameter
is left unfilled if in parameter is considered as in UTF-8. Then, in
nsSmtpService, you examined whether utf8Spec is empty or not (in additon to
checking whether it returns successfully) and only when both conditions are met,
is utf8Spec used. 

In my new code, out parameter is always filled out so that I removed the check
for utf8Spec.IsEmpty(). This is fine as long as out parameter is always filled
up in ConvertURISpecToUTF8, which I thought was the case.  

The problem arose in an unexpected place in
nsUTF8ConverterService::ConvertURISpecToUTF8(). In what follows, I thought
NS_UnescapeURL always filled up unescapedSpec, but that turned out wrong unless
I turn on the flag esc_AlwaysCopy. Back in the caller (nsSmtpService.cpp),
|utf8Spec| is empty, which results in this bug.

+  // NS_UnescapeURL does not fill up unescapedSpec unless there's at least 
+  // one character to unescape.
+  PRBool written = NS_UnescapeURL(PromiseFlatCString(aSpec).get(), aSpec.Length(), 
+                                  esc_OnlyNonASCII, unescapedSpec);

I could have just turned that flag on when calling NS_UnescapeURL, but I
realized that it would lead to an unnecessary copying for the most common cases
(pure ASCII cases). So I decided to leave that out and check the return value
('written'), instead. Well, all of this might be too much 'micro-management' and
I may as well just pass esc_AlwaysCopy.

Comment 21

[Alec Flett]

Comment on attachment 125614 [details] [diff] [review]
a patch

-
-    if (NS_SUCCEEDED(rv))
-      mailtoUrl->SetSpec(utf8Spec);
-    else
-      mailtoUrl->SetSpec(aSpec);
+

if you're not going to use "aSpec" do you need to convert it still? (maybe you
do, I just want to make sure)

sr=alecf

Comment 22

[Jungshik Shin]

Thank you sr, Alec.

I'm sorry because of the embedded tabs, the patch was not so easy to read. Below
I made it easier to read, which  is also for Naoki. 

What's going on is 
1. copy aSpec -> utf8Spec before doing anything (at the cost of
   a redundant 'copying', I got rid of 'if-else' statement. I don't 
   know whether that saves us anything,but it seems that the code is
   simpler this way than what I wrote in comment #13. It might not 
    be...)
2. if aOriginCharset is set, call the converter and store the result in
   utf8Spec 
3. Finally, SetSpec is called with utf8Spec which is a simple copy 
   of aSpec if aOriginCharset is empty while it's the value returned 
   by the converter if aOriginCharset is set. 

 if (NS_SUCCEEDED(rv))
 {
-    nsCAutoString utf8Spec;
+    nsCAutoString utf8Spec(aSpec);
     if (aOriginCharset)
     {
        nsCOMPtr<nsIUTF8ConverterService> 
@@ -323,11 +323,7 @@
        if (NS_SUCCEEDED(rv))
           rv = utf8Converter->ConvertURISpecToUTF8(aSpec, aOriginCharset,
utf8Spec);
     }
-
-    if (NS_SUCCEEDED(rv))
-      mailtoUrl->SetSpec(utf8Spec);
-    else
-      mailtoUrl->SetSpec(aSpec);
+    mailtoUrl->SetSpec(utf8Spec);
 

Comment 23

[Mats Palmgren (inactive)]

Let's say |utf8Converter->ConvertURISpecToUTF8| fails.  Can you really trust
the value in |utf8Spec| after that?  I don't know, but maybe there is a point
in using the original value in that if-else...

Comment 24

[Charles Fenwick]

Functionality restored with Scot MacGregor's checkin at 22:34 last night.
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1055914380&maxdate=1055914680&who=scott%25scott-macgregor.org

See bug 162765 (comments 79 and onward) for details.

Comment 25

[Matthias Versen [:Matti]]

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

Comment 26

[Jungshik Shin]

Attachment: a new patch

Mat, you're right that I have to check the return value of
ConvertURISpecToUTF8(). Now I'm going with what I wrote in comment #13.

Comment 27

[Jungshik Shin]

Comment on attachment 126276 [details] [diff] [review]
a new patch 

Sorry to bother you again :-) Can I get r/sr?

BTW, at the beginning of this patch is a one-line fix for Tru64 cxx. None of
tinderboxes (Linux, Win32, BeOS, MacOS classic and X, Solaris, Irix, HP/UX,
AIX, FreeBSD) has this problem, but Tru64 Cxx  does need an explicit casting as
included in this patch, which shouldn't hurt other platforms. This may have to
be dealt in a separate bug, but it'd be nice if I can get away with sneaking
that in here :-)

Thanks.

Comment 28

[nhottanscp]

Comment on attachment 126276 [details] [diff] [review]
a new patch 

r=nhotta

please add a comment to nsSmtpService.cpp change
since that is a little complicated (e.g. checking aOriginCharset twice)

Comment 29

[Alec Flett]

Comment on attachment 126276 [details] [diff] [review]
a new patch 

could this have been an NS_STATIC_CAST?

sr=alecf with or without it as appropriate.

Comment 30

[Jungshik Shin]

Thank you for r/sr, Naoki and Alec. I'll add the comment as Naoki suggested. As
for NS_STATIC_CAST, I've asked Shanmu of Digital (who reported the problem to
me) to check if it works fine. It should, but I just wanna make sure. As soon as
I get the confirmation from Shanmu, I'll land the patch with NS_STATIC_CAST.

Comment 31

[Shanmu]

The patch with NS_STATIC_CAST worked fine on a Tru64 UNIX.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Er... how about just .get() instead of a cast?

Comment 33

[Jungshik Shin]

fix was checked  in a couple of hours ago. I was waiting for all
tinderboxes(including ports) to turn green after my check-in.

As for casting vs get(), I don't know why I didn't hit upon that idea even after
reading nsAutoPtr.h.  Let's deal with it in bug 210297.

Comment 34

[Jason M. Abels]

Verified fix