Closed Bug 711980 Opened 12 years ago Closed 12 years ago

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

Categories

(Thunderbird :: Testing Infrastructure, defect)

11 Branch
defect
Not set
normal

Tracking

(thunderbird11 fixed)

VERIFIED FIXED
Thunderbird 12.0
Tracking Status
thunderbird11 --- fixed

People

(Reporter: mconley, Assigned: rain1)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 6 obsolete files)

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
Attached patch WIP Patch 1 (obsolete) — Splinter Review
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)
Attached patch WIP Patch 2 (obsolete) — Splinter Review
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+
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.
(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.
Attached patch WIP Patch v3 (obsolete) — Splinter Review
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)
Attached patch fix for test failure (obsolete) — Splinter Review
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...
Attached patch fix mac case (obsolete) — Splinter Review
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.
Attached patch Patch v4 (by sid0) (obsolete) — Splinter Review
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
(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.
(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.
Attached patch patch v5Splinter Review
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)
Attachment #583628 - Flags: superreview?(dbienvenu)
Attachment #583628 - Flags: superreview?
Attachment #583628 - Flags: review?(dbienvenu)
I'll try this on the mac.
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
Closed: 12 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
You need to log in before you can comment on or make changes to this bug.