Last Comment Bug 711980 - Permanent orange: TEST-UNEXPECTED-FAIL | test_mailtoURL.js | test_mime_attachments_size.js | test_attachmentChecker.js | xpcshell/head.js
: Permanent orange: TEST-UNEXPECTED-FAIL | test_mailtoURL.js | test_mime_attach...
Status: VERIFIED FIXED
: dev-doc-needed
Product: Thunderbird
Classification: Client Software
Component: Testing Infrastructure (show other bugs)
: 11 Branch
: All All
: -- normal (vote)
: Thunderbird 12.0
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
Mentors:
Depends on:
Blocks: 687679
  Show dependency treegraph
 
Reported: 2011-12-19 07:18 PST by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2012-01-12 07:02 PST (History)
5 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
WIP Patch 1 (1.07 KB, patch)
2011-12-19 10:15 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
WIP Patch 2 (5.70 KB, patch)
2011-12-19 11:50 PST, Mike Conley (:mconley) - (Needinfo me!)
standard8: review+
Details | Diff | Splinter Review
WIP Patch v3 (3.48 KB, patch)
2011-12-20 09:06 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
fix for test failure (1.56 KB, patch)
2011-12-20 12:02 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
fix mac case (1.04 KB, patch)
2011-12-20 17:21 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
Patch v4 (by sid0) (6.24 KB, patch)
2011-12-21 06:16 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
patch v5 (16.28 KB, patch)
2011-12-21 14:19 PST, Siddharth Agarwal [:sid0] (inactive)
mconley: review+
mozilla: review+
mozilla: superreview+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (Needinfo me!) 2011-12-19 07:18:31 PST
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:
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2011-12-19 07:23:42 PST
As far as we can tell, this looks like a regression from bug 687679 (http://hg.mozilla.org/mozilla-central/rev/d89f3d80d3ec)
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2011-12-19 07:25:21 PST
Here's a log showing the failures:  http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTrunk/1324304903.1324305961.5721.gz
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2011-12-19 10:15:53 PST
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.
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2011-12-19 11:50:53 PST
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.
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2011-12-19 13:40:46 PST
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.
Comment 6 Jonathan Protzenko [:protz] 2011-12-19 14:54:05 PST
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 7 Mark Banner (:standard8) (afk until 26th July) 2011-12-19 15:30:48 PST
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.
Comment 8 Siddharth Agarwal [:sid0] (inactive) 2011-12-20 00:48:45 PST
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 David :Bienvenu 2011-12-20 08:47:18 PST
(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.
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2011-12-20 09:06:02 PST
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.
Comment 11 David :Bienvenu 2011-12-20 12:02:12 PST
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 12 Jonathan Protzenko [:protz] 2011-12-20 12:10:48 PST
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
Comment 13 Siddharth Agarwal [:sid0] (inactive) 2011-12-20 12:14:00 PST
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...
Comment 14 Mike Conley (:mconley) - (Needinfo me!) 2011-12-20 12:15:36 PST
Still failing on OSX:  http://pastebin.mozilla.org/1413818
Comment 15 David :Bienvenu 2011-12-20 17:21:30 PST
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.
Comment 16 Mark Banner (:standard8) (afk until 26th July) 2011-12-21 01:00:34 PST
(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.
Comment 17 Siddharth Agarwal [:sid0] (inactive) 2011-12-21 01:02:35 PST
Are idn-based domains an issue given that they're normally encoded in punycode? In other words, when does the encoding happen?
Comment 18 Mark Banner (:standard8) (afk until 26th July) 2011-12-21 01:05:17 PST
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.
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2011-12-21 06:16:36 PST
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.
Comment 20 Mike Conley (:mconley) - (Needinfo me!) 2011-12-21 06:22:36 PST
Try builds coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=6d26500e6925
Comment 21 David :Bienvenu 2011-12-21 06:56:00 PST
(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 David :Bienvenu 2011-12-21 07:23:09 PST
(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.
Comment 23 Siddharth Agarwal [:sid0] (inactive) 2011-12-21 14:19:15 PST
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?
Comment 24 David :Bienvenu 2011-12-21 14:41:52 PST
I'll try this on the mac.
Comment 25 David :Bienvenu 2011-12-21 15:22:11 PST
Comment on attachment 583628 [details] [diff] [review]
patch v5

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

tests pass on Mac.
Comment 26 Mike Conley (:mconley) - (Needinfo me!) 2011-12-22 06:07:29 PST
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.
Comment 27 Siddharth Agarwal [:sid0] (inactive) 2011-12-22 06:16:10 PST
https://hg.mozilla.org/comm-central/rev/323b7de4b399
Comment 28 Siddharth Agarwal [:sid0] (inactive) 2011-12-22 06:17:06 PST
Comment on attachment 583628 [details] [diff] [review]
patch v5

Bustage fix. one API change though.
Comment 29 Siddharth Agarwal [:sid0] (inactive) 2011-12-22 06:17:56 PST
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.
Comment 30 Mark Banner (:standard8) (afk until 26th July) 2011-12-22 10:54:21 PST
Comment on attachment 583628 [details] [diff] [review]
patch v5

Trunk is green, so that's good enough for me ;-)
Comment 31 Serge Gautherie (:sgautherie) 2011-12-22 18:58:48 PST
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
Comment 32 Mike Conley (:mconley) - (Needinfo me!) 2011-12-29 07:53:44 PST
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/00a50ae8e19e

Note You need to log in before you can comment on or make changes to this bug.