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)
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)
16.28 KB,
patch
|
mconley
:
review+
Bienvenu
:
review+
Bienvenu
:
superreview+
standard8
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
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:
Reporter | ||
Comment 1•12 years ago
|
||
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
Reporter | ||
Comment 2•12 years ago
|
||
Here's a log showing the failures: http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTrunk/1324304903.1324305961.5721.gz
Reporter | ||
Comment 3•12 years ago
|
||
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)
Reporter | ||
Comment 4•12 years ago
|
||
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)
Reporter | ||
Comment 5•12 years ago
|
||
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)
Comment 6•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 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.
Reporter | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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
Assignee | ||
Comment 13•12 years ago
|
||
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...
Reporter | ||
Comment 14•12 years ago
|
||
Still failing on OSX: http://pastebin.mozilla.org/1413818
Comment 15•12 years ago
|
||
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)
Comment 16•12 years ago
|
||
(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.
Assignee | ||
Comment 17•12 years ago
|
||
Are idn-based domains an issue given that they're normally encoded in punycode? In other words, when does the encoding happen?
Comment 18•12 years ago
|
||
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.
Reporter | ||
Comment 19•12 years ago
|
||
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
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → sagarwal
Reporter | ||
Comment 20•12 years ago
|
||
Try builds coming in here: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=6d26500e6925
Comment 21•12 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•12 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.
Assignee | ||
Comment 23•12 years ago
|
||
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•12 years ago
|
Attachment #583628 -
Flags: superreview?(dbienvenu)
Attachment #583628 -
Flags: superreview?
Attachment #583628 -
Flags: review?(dbienvenu)
Comment 24•12 years ago
|
||
I'll try this on the mac.
Comment 25•12 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+
Reporter | ||
Comment 26•12 years ago
|
||
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+
Assignee | ||
Comment 27•12 years ago
|
||
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
Assignee | ||
Comment 28•12 years ago
|
||
Comment on attachment 583628 [details] [diff] [review] patch v5 Bustage fix. one API change though.
Attachment #583628 -
Flags: approval-comm-aurora?
Assignee | ||
Comment 29•12 years ago
|
||
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 30•12 years ago
|
||
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+
Comment 31•12 years ago
|
||
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
Reporter | ||
Comment 32•12 years ago
|
||
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/00a50ae8e19e
status-thunderbird11:
--- → fixed
tracking-thunderbird11:
--- → ?
Updated•12 years ago
|
tracking-thunderbird11:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•