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)
MailNews Core
Networking: SMTP
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.5alpha
People
(Reporter: charles.fenwick, Assigned: jshin1987)
References
()
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
3.02 KB,
patch
|
nhottanscp
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
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.
Comment 1•21 years ago
|
||
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
Comment 2•21 years ago
|
||
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•21 years ago
|
||
*** Bug 209327 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 4•21 years ago
|
||
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•21 years ago
|
||
Bug also occurs in Mozilla nightly trunk build 2003-06-12-22 on Linux
Keywords: regression
OS: Windows 98 → All
Comment 6•21 years ago
|
||
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•21 years ago
|
||
This was a fresh install, btw. I also tried creating a new profile, and that had no effect.
Comment 8•21 years ago
|
||
jshin: your checking touched the external handler stuff. any chance it could be related?
Comment 9•21 years ago
|
||
From bug 167265?
Assignee | ||
Comment 10•21 years ago
|
||
Possibly because it touches nsSmtpService.cpp. I'll figure it out. Thanks for the alert.
Comment 11•21 years ago
|
||
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);
Assignee | ||
Comment 12•21 years ago
|
||
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)
Assignee | ||
Comment 13•21 years ago
|
||
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.
Assignee | ||
Comment 14•21 years ago
|
||
It took a bit more work than I thought. url-escaping routine doesn't work the way I thought it does.
Assignee | ||
Comment 15•21 years ago
|
||
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
Assignee | ||
Comment 16•21 years ago
|
||
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)
Assignee | ||
Comment 17•21 years ago
|
||
if (....) { ... return ...; } - if (IsASCII(unescapedSpec) || IsUTF8(unescapedSpec)) { + else if (IsASCII(unescapedSpec) || IsUTF8(unescapedSpec)) { I'll remove 'else'.
Comment 18•21 years ago
|
||
*** Bug 209554 has been marked as a duplicate of this bug. ***
Comment 19•21 years ago
|
||
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?
Assignee | ||
Comment 20•21 years ago
|
||
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 21•21 years ago
|
||
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+
Assignee | ||
Comment 22•21 years ago
|
||
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•21 years ago
|
||
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...
Reporter | ||
Comment 24•21 years ago
|
||
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•21 years ago
|
||
*** Bug 209983 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 26•21 years ago
|
||
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
Assignee | ||
Updated•21 years ago
|
Attachment #125614 -
Flags: review?(nhotta)
Assignee | ||
Comment 27•21 years ago
|
||
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 28•21 years ago
|
||
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 29•21 years ago
|
||
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+
Assignee | ||
Comment 30•21 years ago
|
||
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•21 years ago
|
||
The patch with NS_STATIC_CAST worked fine on a Tru64 UNIX.
Comment 32•21 years ago
|
||
Er... how about just .get() instead of a cast?
Assignee | ||
Comment 33•21 years ago
|
||
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
Updated•20 years ago
|
Product: MailNews → Core
Updated•15 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•