Closed Bug 1322054 Opened 3 years ago Closed 3 years ago

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

Categories

(MailNews Core :: MIME, defect)

defect
Not set

Tracking

(thunderbird50 wontfix, thunderbird51 fixed, thunderbird52 fixed, thunderbird53 fixed, seamonkey2.46 unaffected, seamonkey2.47 fixed, seamonkey2.48 fixed, seamonkey2.49esr fixed, seamonkey2.50 fixed)

RESOLVED FIXED
Thunderbird 53.0
Tracking Status
thunderbird50 --- wontfix
thunderbird51 --- fixed
thunderbird52 --- fixed
thunderbird53 --- fixed
seamonkey2.46 --- unaffected
seamonkey2.47 --- fixed
seamonkey2.48 --- fixed
seamonkey2.49esr --- fixed
seamonkey2.50 --- fixed

People

(Reporter: jorgk, Assigned: jorgk)

References

Details

Attachments

(1 file, 1 obsolete file)

See bug 1169184 comment #32 and two further comments.
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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8816796 - Flags: review?(acelists)
Attachment #8816796 - Flags: feedback?(frgrahl)
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 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.
Attachment #8816796 - Flags: feedback?(frgrahl) → feedback+
(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.
> 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
Holler!!
Attachment #8816796 - Attachment is obsolete: true
Attachment #8816796 - Flags: review?(acelists)
Attachment #8816803 - Flags: feedback?(frgrahl)
Comment on attachment 8816803 [details] [diff] [review]
1322054-empty-content-language.patch (v2).

Works fine too.
Attachment #8816803 - Flags: feedback?(frgrahl) → feedback+
Attachment #8816803 - Flags: review?(acelists)
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.
Attachment #8816803 - Flags: review?(acelists) → review+
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 ;-(
Attachment #8816803 - Flags: approval-comm-release+
Attachment #8816803 - Flags: approval-comm-beta+
Attachment #8816803 - Flags: approval-comm-aurora+
Summary: Empty Content-Language: header produced when no dictionary is installed → Empty "Content-Language: header" produced when no dictionary is installed
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
Summary: Empty "Content-Language: header" produced when no dictionary is installed → Empty "Content-Language:" header line produced if no dictionary is installed
(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.
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.
Duplicate of this bug: 1322048
You need to log in before you can comment on or make changes to this bug.