Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Permanent orange: TEST-UNEXPECTED-FAIL | test_mailtoURL.js | test_mime_attachments_size.js | test_attachmentChecker.js | xpcshell/head.js

VERIFIED FIXED in Thunderbird 12.0

Status

Thunderbird
Testing Infrastructure
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: mconley, Assigned: sid0)

Tracking

({dev-doc-needed})

11 Branch
Thunderbird 12.0
dev-doc-needed
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird11 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

We've been seeing the following permanent oranges on comm-central in our XPCShell test runs:

TEST-UNEXPECTED-FAIL | /buildbot/comm-central-linux64-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/compose/test/unit/test_mailtoURL.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /buildbot/comm-central-linux64-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/compose/test/unit/test_mailtoURL.js | ´┐Żencoded subject! == ┬íencoded subject! - See following stack:
TEST-UNEXPECTED-FAIL | /buildbot/comm-central-linux64-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/db/gloda/test/unit/test_mime_attachments_size.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /buildbot/comm-central-linux64-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/db/gloda/test/unit/test_mime_attachments_size.js | false == true - See following stack:
TEST-UNEXPECTED-FAIL | /buildbot/comm-central-linux64-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/db/gloda/test/unit/test_mime_attachments_size.js | 2147500036 - See following stack:
TEST-UNEXPECTED-FAIL | /buildbot/comm-central-linux64-opt-unittest-xpcshell/build/xpcshell/tests/mail/base/test/unit/test_attachmentChecker.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | URIError: malformed URI sequence - See following stack:
As far as we can tell, this looks like a regression from bug 687679 (http://hg.mozilla.org/mozilla-central/rev/d89f3d80d3ec)
Blocks: 687679
Target Milestone: --- → Thunderbird 11.0
Here's a log showing the failures:  http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTrunk/1324304903.1324305961.5721.gz
Created attachment 582870 [details] [diff] [review]
WIP Patch 1

So this patch seems to fix the tests for test_mailtoURL.js...

However, my understanding of what this test does is a bit shallow, and I'm kind of worried that I'm "moving the goalposts" a little.  Does my patch actually fix the problem, or have I just rendered it meaningless?

The tests for test_mime_attachment_size.js are a little trickier.  I'm going to CC protz on this bug to see what his thoughts are on this.
Attachment #582870 - Flags: feedback?(mbanner)
Created attachment 582906 [details] [diff] [review]
WIP Patch 2

Alright, here's a second shot at it.  Again, let me know if I've just moved the goalposts here.
Attachment #582870 - Attachment is obsolete: true
Attachment #582870 - Flags: feedback?(mbanner)
Comment on attachment 582906 [details] [diff] [review]
WIP Patch 2

We still need to figure out the last test failure in test_mime_attachments_size.js, but, assuming these fixes aren't neutering the tests for test_mailtoUrl.js and test_attachmentChecker.js, that means we've got 2 / 3 so far.
Attachment #582906 - Flags: superreview?(dbienvenu)
Attachment #582906 - Flags: review?(mbanner)
Sorry I don't have any ideas for that :-/. Mike, the code you might want to read is in mailnews/mime/src/mimebuf.cpp, esp. mime_LineBuffer; the code that glues the libmime C code and the rest of mailnews together is in mimemoz2.cpp, see the mime_output_fn function.

(If I have any fresh ideas be assured that I'll post them promptly here.)
Comment on attachment 582906 [details] [diff] [review]
WIP Patch 2

Review of attachment 582906 [details] [diff] [review]:
-----------------------------------------------------------------

r=me without the nsIMimeEmitter.idl changes, unless you can prove they are actually necessary.

::: mailnews/compose/public/nsISmtpUrl.idl
@@ +122,5 @@
>     *
>     * These items are in one function as we only ever get them from the one
>     * place and all at the same time.
>     */
> +  void GetMessageContents(out AUTF8String aToPart, out AUTF8String aCcPart,

So I'm not convinced we need to UTF8ify all these parameters, but I don't have objections if bienvenu doesn't.

::: mailnews/mime/public/nsIMimeEmitter.idl
@@ +54,5 @@
>  %{C++
>  #define NS_IMIME_MISC_STATUS_KEY       "@mozilla.org/MimeMiscStatus;1?type="
>  %}
>  
> +[scriptable, uuid(74fd2afb-b329-4959-85b1-f7451415aac9)]

The changes in this file don't seem to be needed to get the two tests to pass.
Attachment #582906 - Flags: review?(mbanner) → review+
(Assignee)

Comment 8

6 years ago
As I said on IRC, I'm not very confident about making all the parameters in nsISmtpUrl AUTF8Strings, given https://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#740 and the fact that references is a mere string at https://mxr.mozilla.org/comm-central/source/mailnews/compose/public/nsIMsgCompFields.idl#63. I think references is merely a list of message ids, and message ids can only be ASCII.

Comment 9

6 years ago
(In reply to Siddharth Agarwal [:sid0] from comment #8)
> As I said on IRC, I'm not very confident about making all the parameters in
> nsISmtpUrl AUTF8Strings, given
> https://mxr.mozilla.org/comm-central/source/mailnews/compose/src/
> nsMsgComposeService.cpp#740 and the fact that references is a mere string at
> https://mxr.mozilla.org/comm-central/source/mailnews/compose/public/
> nsIMsgCompFields.idl#63. I think references is merely a list of message ids,
> and message ids can only be ASCII.

Yes, I tend to agree.
Created attachment 583193 [details] [diff] [review]
WIP Patch v3

I've removed the changes to nsIMimeEmitter, since, as Standard8 noticed, they don't do anything to help us pass the tests.

Which parts of nsIMailToUrl need to be ACString, and which parts need to be AUTF8String?  I wasn't sure, so I've left that for now.
Attachment #582906 - Attachment is obsolete: true
Attachment #582906 - Flags: superreview?(dbienvenu)

Comment 11

6 years ago
Created attachment 583240 [details] [diff] [review]
fix for test failure

the js string changes changed the reported length of the string (it was 173, now it's 168, which seems more correct). That exposed an issue with our calculation of the relationship between the original text length and the attachment size. I haven't had time to figure out why that calculation was so wrong before, but this does get the test working on windows again. I also had to change one of the epsilons to 3 from 2.
Comment on attachment 583240 [details] [diff] [review]
fix for test failure

Hullo,

Huh that sounds right since we're talking about attachment size (counted in bytes, namely 173) vs string length, which now reports the correct 168 character length...

(thanks!).

jonathan
Oh... right. So then the right solution would be to figure out how many bytes that string has in UTF-8 and compare against that. I wonder if that would cause the epsilons to go away...
Still failing on OSX:  http://pastebin.mozilla.org/1413818

Comment 15

6 years ago
Created attachment 583352 [details] [diff] [review]
fix mac case

this should fix it for the mac test case - attachment size was 169, for funky funk, for some reason, which was 4 away from 173, not 3. I'm not sure why the attachment size would be different on the mac, though line endings are the obvious suspect.
Attachment #583240 - Attachment is obsolete: true
Attachment #583352 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) from comment #10)
> Which parts of nsIMailToUrl need to be ACString, and which parts need to be
> AUTF8String?  I wasn't sure, so I've left that for now.

My instinct says 

- Definitely the To/Cc/Bcc/Subject parts
-- We should be allowing non-ascii based names in the To fields etc, and even if that isn't in these fields, we'll be allowing idn based domain names at some stage.
- Uncertain about the rest
-- I'd need David to chime in here.
Are idn-based domains an issue given that they're normally encoded in punycode? In other words, when does the encoding happen?
Well if the Subject we're getting is decoded at this stage, if it is a message we've saved or something, I suspect that those fields will be decoded as well.
Created attachment 583474 [details] [diff] [review]
Patch v4 (by sid0)

Sid just sent me this patch as a more precise solution to the test_mailtoURL.js and test_attachmentChecker.js test failures.

I'll be taking both this and bienvenu's patch and pushing them to try shortly.
Attachment #583193 - Attachment is obsolete: true
Assignee: nobody → sagarwal
Try builds coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=6d26500e6925

Comment 21

6 years ago
(In reply to Mark Banner (:standard8) from comment #16)
> (In reply to Mike Conley (:mconley) from comment #10)
> > Which parts of nsIMailToUrl need to be ACString, and which parts need to be
> > AUTF8String?  I wasn't sure, so I've left that for now.
> 
> My instinct says 
> 
> - Definitely the To/Cc/Bcc/Subject parts
> -- We should be allowing non-ascii based names in the To fields etc, and
> even if that isn't in these fields, we'll be allowing idn based domain names
> at some stage.
> - Uncertain about the rest
> -- I'd need David to chime in here.

I agree, yes for the To/Cc/Bcc/Subject parts.

priorityPart is treated as a utf8 string in the code, so that should change as well (though I don't know if it really would ever be). I think news host part should be as well, though that seems less likely to happen than the other fields.

Comment 22

6 years ago
(In reply to David :Bienvenu from comment #15)
> Created attachment 583352 [details] [diff] [review]
> fix mac case
> 
> this should fix it for the mac test case - attachment size was 169, for
> funky funk, for some reason, which was 4 away from 173, not 3. I'm not sure
> why the attachment size would be different on the mac, though line endings
> are the obvious suspect.

On windows, the attachment size is 170 for the funky funk test, which certainly implies a line ending difference.
Created attachment 583628 [details] [diff] [review]
patch v5

I think this should fix everything up.

I've also changed the GetMessageContents in the idl to getMessageContents, since this is an API change anyway.

Tested on Windows. Could someone test this on Mac/Linux?
Attachment #583352 - Attachment is obsolete: true
Attachment #583474 - Attachment is obsolete: true
Attachment #583352 - Flags: review?(mconley)
Attachment #583628 - Flags: superreview?
Attachment #583628 - Flags: review?(mconley)
(Assignee)

Updated

6 years ago
Attachment #583628 - Flags: superreview?(dbienvenu)
Attachment #583628 - Flags: superreview?
Attachment #583628 - Flags: review?(dbienvenu)

Comment 24

6 years ago
I'll try this on the mac.

Comment 25

6 years ago
Comment on attachment 583628 [details] [diff] [review]
patch v5

that's funny/sad the idl for GetMessageContents was wrong

tests pass on Mac.
Attachment #583628 - Flags: superreview?(dbienvenu)
Attachment #583628 - Flags: superreview+
Attachment #583628 - Flags: review?(dbienvenu)
Attachment #583628 - Flags: review+
Comment on attachment 583628 [details] [diff] [review]
patch v5

Review of attachment 583628 [details] [diff] [review]:
-----------------------------------------------------------------

Just one tiny style nit, but other than that, this looks reasonable (as far as I understand it).

Thanks for your work,

-Mike

::: mailnews/db/gloda/test/unit/test_mime_attachments_size.js
@@ +243,5 @@
>  
>    let totalSize = htmlText.length;
>  
>    for each (let [i, att] in Iterator(aMimeMsg.allUserAttachments)) {
> +    dump("*** Attachment now is "+att.name+" "+att.size+" "+"\n");

While you're here, spaces around the +'s would be nice.
Attachment #583628 - Flags: review?(mconley) → review+
https://hg.mozilla.org/comm-central/rev/323b7de4b399
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 11.0 → Thunderbird 12.0
Comment on attachment 583628 [details] [diff] [review]
patch v5

Bustage fix. one API change though.
Attachment #583628 - Flags: approval-comm-aurora?
The API change needs to be documented and addon compat checks run for it. Assuming this gets approval for Aurora, this needs to be done for Thunderbird 11.
Keywords: dev-doc-needed
Comment on attachment 583628 [details] [diff] [review]
patch v5

Trunk is green, so that's good enough for me ;-)
Attachment #583628 - Flags: approval-comm-aurora? → approval-comm-aurora+
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1324598894.1324601594.17748.gz
OS X 10.5 comm-central-trunk debug test xpcshell on 2011/12/22 16:08:14

V.Fixed
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Hardware: x86 → All
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/00a50ae8e19e
status-thunderbird11: --- → fixed
tracking-thunderbird11: --- → ?
tracking-thunderbird11: ? → ---
You need to log in before you can comment on or make changes to this bug.