1322054 - Empty "Content-Language:" header line produced if no dictionary is installed

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Jorg K (CEST = GMT+2)]

See bug 1169184 comment #32 and two further comments.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1322054-empty-content-language.patch (v1).

We had complaints from SM that an empty header was written since they haven't ported bug 1169184 yet, see bug 1289493.

We would write an empty header if the comp-field were an empty string, which I think can happen if there is no dictionary installed. Hard to test, since we always build with the en-US dictionary. Anyway, I tested this by hacking contentLanguage = "" in that hunk.

This additional check does no harm. Note that the code in nsMsgCompUtils.cpp wasn't necessary, the header gets copied over in finalHeaders->AddAllHeaders(fields) a few lines up.

The associated test is
https://dxr.mozilla.org/comm-central/rev/79a8ad3930a66444320f89102a45102ccf51186c/mail/test/mozmill/composition/test-drafts.js#203
and that still passes.

Comment 2

[:aceman]

Comment on attachment 8816796 [details] [diff] [review]
1322054-empty-content-language.patch (v1).

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

::: mailnews/compose/src/nsMsgSend.cpp
@@ +2780,5 @@
> +  char* contentLanguage = nullptr;
> +  fields->GetContentLanguage(&contentLanguage);
> +  if (contentLanguage && !*contentLanguage)
> +    contentLanguage = nullptr; // Delete the field.
> +  mCompFields->SetContentLanguage(contentLanguage);

Doesn't SetContentLanguage() do the same at https://dxr.mozilla.org/comm-central/rev/79a8ad3930a66444320f89102a45102ccf51186c/mailnews/compose/src/nsMsgCompFields.cpp#105 (it calls SetAsciiHeader)?

Comment 3

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8816796 [details] [diff] [review]
1322054-empty-content-language.patch (v1).

Just built Seamonkey 2.50a1 and posted to mozilla.support.seamonkey with it. The empty Content-Language field is gone so an f+ and a big thanks from me.

If this works for TB too please push it down to c-b. I can push it to c-r.

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Frank-Rainer Grahl from comment #3)
> If this works for TB too please push it down to c-b. I can push it to c-r.
Haha, before I can push anything, I need to get it past Acemen who is already questioning the solution ;-)


(In reply to :aceman from comment #2)
> Doesn't SetContentLanguage() do the same at ...
Hmm, looks like it. Good catch! Well, perhaps we get away be just removing the other faulty hunk. Right now I need to check Magnus' security bug, but I'll get back to this later tonight.

Comment 5

[Frank-Rainer Grahl (:frg)]

> Haha, before I can push anything, I need to get it past Acemen 
> who is already questioning the solution ;-)

Sure no rush. Not all servers react allergic to it or it would have been discovered earlier. Just came up in the newsgroups today. Just holler if you want me to test a new patch.

FRG

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1322054-empty-content-language.patch (v2).

Holler!!

Comment 7

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8816803 [details] [diff] [review]
1322054-empty-content-language.patch (v2).

Works fine too.

Comment 8

[:aceman]

Comment on attachment 8816803 [details] [diff] [review]
1322054-empty-content-language.patch (v2).

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

So the SetRawHeader() doesn't clear out headers with empty values.

Comment 9

[Jorg K (CEST = GMT+2)]

C-C (TB 53): https://hg.mozilla.org/comm-central/rev/85dd34e18d173d05872889f4b9bb3691b61be6f5
C-A (TB 52): https://hg.mozilla.org/releases/comm-aurora/rev/99f2496ef9788d0aa028fb87f98f2da336db1fe2
C-B (TB 51): https://hg.mozilla.org/releases/comm-beta/rev/62c75cca01eb04c698cec4ee512316f316d3b0c4
(not part of TB 51 beta1 build2)

FRG, please land on C-R if you want.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8816803 [details] [diff] [review]
1322054-empty-content-language.patch (v2).

Landed/approved for all branches. Minor tweak to remove some wrong code that caused problems ;-(

Comment 11

[Rainer Bielefeld]

Although Dictionaries are installed for German, French, en-US:
REPRODUCIBLE with Server-Installation of  unofficial (by FRG) en-US SeaMonkey 2.50a1 (NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0 Build 20161127221846  (Default Classic Theme) on German WIN7 64bit

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Rainer Bielefeld from comment #11)
> Although Dictionaries are installed for German, French, en-US:
Sure. Because SM is missing bug 1169184 / bug 1289493.
In TB it would only happen if no dictionary were installed, or perhaps not at all, I didn't test.

We fixed this as a favour for FRG, so never mind the summary. I didn't want to put:
  Fix in mailnews/ to cater for non-ported bug 1169184, see SM bug 1289493.

Comment 13

[Frank-Rainer Grahl (:frg)]

Thanks.

https://hg.mozilla.org/releases/comm-release/rev/4da63141cb902adf573947586a2e52915ad4ebb1

At least SeaMonkey de ships without a dictionary initially so it might do some good even the bug is ported.