Closed Bug 209328 Opened 21 years ago Closed 21 years ago

Mailto: links are not recognized and are not working properly

Categories

(MailNews Core :: Networking: SMTP, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.5alpha

People

(Reporter: charles.fenwick, Assigned: jshin1987)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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 webmasters@mozilla.org)
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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Mailto: links are not recognized and are not working properly → Mailto: links are not recognized and are not working properly
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)?
*** Bug 209327 has been marked as a duplicate of this bug. ***
Benjamin,

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

Everything is installed.
Bug also occurs in Mozilla nightly trunk build 2003-06-12-22 on Linux
Keywords: regression
OS: Windows 98 → All
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.

This was a fresh install, btw.

I also tried creating a new profile, and that had no effect.
jshin: your checking touched the external handler stuff.  any chance it could be
related?
From bug 167265?
Possibly because it touches nsSmtpService.cpp. I'll figure it out. Thanks for
the alert. 
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);
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)
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.

 
Attached patch a patch (obsolete) — Splinter Review
It took a bit more work than I thought. url-escaping routine doesn't work the
way I thought it does.
assign to myself. Change the product to mailnews. I couldn't set the component,
though. 
Assignee: general → jshin
Component: Browser-General → Networking: SMTP
Product: Browser → MailNews
Hardware: PC → All
Target Milestone: --- → mozilla1.5alpha
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 ;-) )
Attachment #125614 - Flags: superreview?(alecf)
Attachment #125614 - Flags: review?(nhotta)
Blocks: 207974
   if (....) {
     ... return ...;
   }
-  if (IsASCII(unescapedSpec) || IsUTF8(unescapedSpec)) {
+  else if (IsASCII(unescapedSpec) || IsUTF8(unescapedSpec)) {

  I'll remove 'else'.
*** Bug 209554 has been marked as a duplicate of this bug. ***
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?
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.

Status: NEW → ASSIGNED
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
Attachment #125614 - Flags: superreview?(alecf) → superreview+
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);
 
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...
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.
*** Bug 209983 has been marked as a duplicate of this bug. ***
Attached patch a new patch Splinter Review
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.
Attachment #125614 - Attachment is obsolete: true
Attachment #125614 - Flags: review?(nhotta)
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.
Attachment #126276 - Flags: superreview?(alecf)
Attachment #126276 - Flags: review?(nhotta)
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)
Attachment #126276 - Flags: review?(nhotta) → review+
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.
Attachment #126276 - Flags: superreview?(alecf) → superreview+
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.
The patch with NS_STATIC_CAST worked fine on a Tru64 UNIX.
Er... how about just .get() instead of a cast?
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.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Verified fix
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: