441075 - mailto:?body= gives corrupted body (send link from firefox gets corrupted) in plain text composition

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect, P2)

Description

[km]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008061015 Firefox/3.0
Build Identifier: version 3.0a2pre (2008062118)

If I invoke thunderbird with the mailto:?body= option against a running thunderbird, the compose window comes up with corrupted contents related to the special character "%" encoding.

Reproducible: Always

Steps to Reproduce:
1.thunderbird mailto:?body=http%3A%2F%2FXYZ
2.
3.
Actual Results:  
Compose window comes up with body showing
http://XYZ$2FXYZ 
  where I've put $ in place of a graphic indicating an unprintable character.

If I try and send this message the delivery just hangs. I can however, save it as draft. When I bring the draft back up the corruption is gone and it can be sent.



Expected Results:  
http://XYZ    

in the compose window.

This makes it impossible to use send link from firefox, without saving and recovering the draft.

This is a regression.

Comment 1

[Phil Ringnalda (:philor)]

Huh, I thought this was already filed, since I've known for a while that Send Link from Firefox from a bug page was broken (tacking id%3D441075 onto the end of the URL), but since it regressed between May 13th and May 16th, which points squarely at bug 433687, and nothing's blocking that, I guess it isn't.

The cause really ought to be obvious, since it's only affecting plaintext compose, not HTML, which really narrows down where it has to be happening, but I just don't see it.

Comment 2

[Karsten Düsterloh]

(1) SeaMonkey trunk is also affected.
(2) Backing out bug 433687 fixes it.
(3) But only accidently, it just wallpapers the actual problem.
(4) Which is actually bug 358277.
(5) Which is yours. :-P

The length of members of nsMailtoUrl like m_bodyPart gets computed *including* the escape characters, eg. http://ABCDEF will be escaped as http%3a%2f%2fABCDEF and thus have length 19. Calling nsUnescape on its buffer doesn't correct the length of the nsCString!
This wrong length will now be copied over and over and the superfluous bytes will sneak back in via some UTF16/UTF8 conversion fest...

Comment 3

[Phil Ringnalda (:philor)]

Yeah, I'll be happy to take the blame for that: it has always been broken for all of the parts which weren't mime-decoded and thus Adopt()ed, which didn't matter I'm betting because getter_Copies copied from the unbroken part instead of the broken part, but I'm squarely on the hook for having added something that's actually used and visible to the set of broken things :)

Lest we forget, the IRC consensus seemed to be that we should be parsing into locals, escapedBody et al., and then using MsgUnescapeString to put them more neatly into m_bodyPart et al.

Comment 4

[km]

Any progress on this? It really is pretty hard to forward a link because of this bug.

Comment 5

[Phil Ringnalda (:philor)]

I spent a couple of hours look at and poking it, and looking at draft-duerst-mailto-bis-05, and ad-hoc testing some of the pathological things that need to be in an automated testcase for this bug, and I've got the start of a rash on my left cheek. That's progress, though not exactly in the right direction.

"Fun" question: in the current world, we mime-decode |mailto:(mime-encoded-string)?| but not |mailto:(mime-encoded-string)| (that is, when the URI has a query string, even an empty one, then all to-parts, both from to= and from the part before the query string, are mime-decoded, but when there is no query string, we don't mime-decode). In which case are we right, and why, and ignoring rightness, what's actually useful without being unexpectedly wrong?

Comment 6

[Phil Ringnalda (:philor)]

I must have been tired, to actually submit a comment that's certain to mean I own it now.

And the answer is, we're right to decode to=foo, and right to not decode mailto:foo, and wrong to decode foo but right to decode bar in mailto:foo?to=bar, and useful doesn't usefully matter, since we shouldn't be encouraging people to think that they can use things that contradict the spec and thus shouldn't be expected to work with other consumers (not that we don't, *cough*html-body*cough* but we really shouldn't).

Comment 7

[Phil Ringnalda (:philor)]

Spun off bug 443848, bug 443850, and bug 443851, since I was starting to get the feeling I wouldn't get to declare victory until I could |#include nsSolveWorldHunger.h|

Comment 8

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

Sure, it's ugly and brutal, but if you want elegance you're supposed to get someone else to do it.

Comment 9

[[e-mail]]

Comment on attachment 328282 [details] [diff] [review]
Fix v.1

>+  nsCAutoString escapedInReplyToPart;
>+  nsCAutoString escapedToPart;
>+  nsCAutoString escapedCcPart;
>+  nsCAutoString escapedSubjectPart;
>+  nsCAutoString escapedNewsgroupPart;
>+  nsCAutoString escapedNewsHostPart;
>+  nsCAutoString escapedReferencePart;
>+  nsCAutoString escapedBodyPart;
>+  nsCAutoString escapedBccPart;
>+  nsCAutoString escapedFollowUpToPart;
>+  nsCAutoString escapedFromPart;
>+  nsCAutoString escapedHtmlPart;
>+  nsCAutoString escapedOrganizationPart;
>+  nsCAutoString escapedReplyToPart;
>+  nsCAutoString escapedPriorityPart;
This file might not be external-string-api safe yet, but I think it makes sense to use nsCString to save work down the line.

>+      escapedPath.Cut(startOfSearchPart, numExtraChars);
>+      MsgUnescapeString(escapedPath, 0, m_toPart);
Again, Substring would be more appropriate than Left/Right/Cut/whatever, but I understand that you might not feel like fixing that ;-)

Comment 10

[David :Bienvenu]

Comment on attachment 328282 [details] [diff] [review]
Fix v.1

thx, Phil.

Comment 11

[Phil Ringnalda (:philor)]

Checked in, minus the nsCAutoStrings (which I keep thinking *are* nsCStrings, for no good reason) but with the Cutting and friends intact: you might want me to fix that, but you really don't want me fixing that without closely looking at how I muck it up, so we're better off doing it later and separately.

mailnews/compose/src/nsSmtpUrl.cpp 1.85
mailnews/compose/test/unit/test_mailtoURL.js 1.1