TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/composition/test-charset-edit.js

RESOLVED FIXED in Thunderbird 51.0

Status

Thunderbird
General
RESOLVED FIXED
10 months ago
5 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

unspecified
Thunderbird 51.0

Thunderbird Tracking Flags

(thunderbird_esr4551+ fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

10 months ago
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/composition/test-charset-edit.js | test-charset-edit.js::test_wrong_reply_charset
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/composition/test-charset-edit.js | test-charset-edit.js::test_no_mojibake 

First seen Wed Aug 24, 2016, 16:01:00
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=44a362f04d8d444b7e84ff772a28fed383a58ab0

Most likely caused by bug 1191841.
(Assignee)

Comment 1

10 months ago
mozmake SOLO_TEST=composition/test-drafts.js mozmill-one

Failures are:

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\composition\test-charset-edit.js | test-charset-edit.js::test_wrong_reply_charset
  EXCEPTION: a != b: 'UTF-8' != 'windows-1252'.
    at: test-folder-display-helpers.js line 2858
       assert_true test-folder-display-helpers.js:2858 11
       assert_equals test-folder-display-helpers.js:2845 3
       test_wrong_reply_charset test-charset-edit.js:113 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestFile frame.js:534 3
       runTestFile frame.js:713 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\composition\test-charset-edit.js | test-charset-edit.js::test_no_mojibake
  EXCEPTION: a != b: 'x-mac-croatian' != 'utf-7'.
    at: test-folder-display-helpers.js line 2858
       assert_true test-folder-display-helpers.js:2858 11
       assert_equals test-folder-display-helpers.js:2845 3
       test_no_mojibake test-charset-edit.js:146 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestFile frame.js:534 3
       runTestFile frame.js:713 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3

We also note
[332] WARNING: NS_ENSURE_SUCCESS(childCV->SetForceCharacterSet(msgCharSet), NS_ERROR_FAILURE) failed with result 0x80070057: file c:/mozilla-source/comm-central/mailnews/compose/src/nsMsgCompose.cpp, line 1618

This was introduced in bug 1191841. Local debugging shows that the charset that fails to be set is x-mac-croatian.
(Assignee)

Comment 2

10 months ago
Created attachment 8784533 [details] [diff] [review]
Proposed solution (v1a).

Not a nice patch. But M-C broke us and we need to react.

Clean-up bug 1297118 already filed.
Attachment #8784533 - Flags: review?(mkmelin+mozilla)

Comment 3

10 months ago
Comment on attachment 8784533 [details] [diff] [review]
Proposed solution (v1a).

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

Isn't this just hiding the error? I don't think we should hide it if it's real.
(Assignee)

Comment 4

10 months ago
No, the test that fails deliberately sets an invalid charset to check that this gets corrected somehow. With the changed behaviour of M-C this is not happening, since SetForceCharacterSet() now complains about bad input. So we are busted.

As the checkin comment says:
Ignore error returned by SetForceCharacterSet() to maintain *status quo*.

The whole thing should be reviewed in bug 1297118.
(Assignee)

Comment 5

10 months ago
Perhaps I should say it more clearly.

Before bug 1191841, SetForceCharacterSet() would accept any input and the HTML5 parser would crash later.
They changed that, and now it returns an error instead of crashing later.

Before we checked for an error that never came even with bad input. Ignoring that error now restores the exact previous state (status quo), but without the crash.

So I'd land this now to fix the test failure and the fact that TB actually now doesn't work and review the whole area later.

I hope the purists can wait for bug 1297118 ;-)
(Assignee)

Comment 6

10 months ago
(In reply to Jorg K (GMT+2, PTO during summer) from comment #5)
> ... and the fact that TB actually now doesn't work ...
Right now, you can't reply to a message with a bad character set since nsMsgCompose::InitEditor() returns with a bad status. I'm not sure what the final solution will look like, but not setting a bad charset, that is, ignoring the error for now, will get the default and that's what we want. That's the status quo.
(Assignee)

Comment 7

10 months ago
Created attachment 8784572 [details]
UTF-7  test.eml

Right now, you can't answer this e-mail. With the patch, which restores the previous state, you can. Once again, this is meant as a bustage fix for the bustage caused by bug 1191841.
FWIW, I intend to remove the x-mac-* encodings that aren't in the Encoding Standard in due course. Intent to Remove coming up after I've dealt with more pressing issues.

Comment 9

10 months ago
Comment on attachment 8784533 [details] [diff] [review]
Proposed solution (v1a).

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

Ok, yeah I guess this makes sense then. r=mkmelin
Attachment #8784533 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 10

10 months ago
Aleth, can you land this with your fix for the build oranges so we get a "completely" green tree, ha, ha.
Flags: needinfo?(aleth)
Keywords: checkin-needed

Comment 11

10 months ago
https://hg.mozilla.org/comm-central/rev/d1ab8c5568261fa49914c23022f76806a73b6767
Bug 1297761 - Ignore error returned by SetForceCharacterSet() to maintain status quo. r=mkmelin

Updated

10 months ago
Blocks: 1191841
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Flags: needinfo?(aleth)
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0

Comment 12

10 months ago
7 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* comm-central: 7

Platform breakdown:
* windowsxp: 2
* windows7-32: 2
* linux64: 2
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1297761&startday=2016-08-22&endday=2016-08-28&tree=all

Comment 13

7 months ago
Comment on attachment 8784533 [details] [diff] [review]
Proposed solution (v1a).

[Approval Request Comment]
If we land the patch for bug 1191841 in m-esr45 THUNDERBIRD_45_VERBRANCH then presumably this issue will appear in TB 45, so we should land this too.
Attachment #8784533 - Flags: approval-comm-esr45?
(Assignee)

Comment 14

7 months ago
Right!

Updated

5 months ago
Attachment #8784533 - Flags: approval-comm-esr45? → approval-comm-esr45+

Comment 15

5 months ago
https://hg.mozilla.org/releases/comm-esr45/rev/af893d6214a0
status-thunderbird_esr45: --- → fixed
tracking-thunderbird_esr45: --- → 51+
You need to log in before you can comment on or make changes to this bug.