Closed Bug 1462481 Opened 6 years ago Closed 6 years ago

HTML Sanitizer class cleanup


(MailNews Core :: MIME, enhancement)

Not set


(thunderbird_esr52 fixed, thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)

Thunderbird 62.0
Tracking Status
thunderbird_esr52 --- fixed
thunderbird_esr60 --- fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- fixed


(Reporter: BenB, Assigned: BenB)




(1 file, 3 obsolete files)

Attached patch efail-sanitizer-cleanup-1.diff (obsolete) — Splinter Review
The attached patch is a fallout from my work in security bug 1419417. None of the changes in this patch are important for security, they are just cleaning up the code from old debug code etc.

The purpose of this ticket is only to get that patch in. The purpose is expressively *not* to make a general cleanup, or fix more than I already did there.

Please note that libmime has a different code style than the rest of Thunderbird, and I do not intent to change that in this in this bug. If you want wider style changes, they should happen in all of libmime at once. Like we sometimes have spellcheck or warning fix pr whitespace change patches across a whole module.

The purpose of this bug is only to get this patch in. It should be a massive improvement over status quo. We can make more changes later.
Comment on attachment 8976717 [details] [diff] [review]

Carrying over positive review from Magnus from bug 1419417.
Attachment #8976717 - Flags: review+
Attached patch efail-sanitizer-cleanup-2.diff (obsolete) — Splinter Review
This is against 52.

Will need update for trunk.
Should also remove the AllowStyles and pref change, as that depends on another mozilla-central change.
Attached patch efail-sanitizer-cleanup-3.diff (obsolete) — Splinter Review
Remove changing the name and effect of the pref.
This removes the functional changes of the patch.

The patch mostly removes debug code and fixes a few comments.
Attachment #8976718 - Attachment is obsolete: true
Comment on attachment 8976725 [details] [diff] [review]

To review, it's easiest to compare with patch v1, which was already r+.
Attachment #8976725 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8976725 [details] [diff] [review]

Thanks again for the clean-up. It's a good idea to move this to this separate bug.

Please rebase the patch against trunk. Right now I get:
pplying efail-sanitizer-cleanup-3.diff
patching file mailnews/mime/src/mimemoz2.cpp
Hunk #1 FAILED at 2154
1 out of 1 hunks FAILED -- saving rejects to file mailnews/mime/src/mimemoz2.cpp.rej
patching file mailnews/mime/src/mimethsa.cpp
Hunk #1 FAILED at 1
1 out of 3 hunks FAILED -- saving rejects to file mailnews/mime/src/mimethsa.cpp.rej
patching file mailnews/mime/src/mimethsa.h
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file mailnews/mime/src/mimethsa.h.rej
patch failed, unable to continue (try -v)
Attachment #8976725 - Flags: review+
Comment on attachment 8976725 [details] [diff] [review]

Review of attachment 8976725 [details] [diff] [review]:

Looks good. Need it to apply to trunk though
Attachment #8976725 - Flags: review?(mkmelin+mozilla) → review+
Rebased to trunk and added commit message. Also removed superfluous parenthesis in |x = y ? z : t| assignment.
Attachment #8976717 - Attachment is obsolete: true
Attachment #8976725 - Attachment is obsolete: true
Attachment #8979053 - Flags: review+
Pushed by
clean up MIME's HTML sanitizer class. r=mkmelin,jorgk
fix white-space issues in mimethsa.cpp. rs=white-space-only
Closed: 6 years ago
Resolution: --- → FIXED
I took the liberty to fix white-space issues while we where here:
- No space between function names and opening parenthesis
- no space used for casts.

Some MIME files have already been cleaned up that way, for example the worst offender here:
Target Milestone: Thunderbird 61.0 → Thunderbird 62.0
Attachment #8979053 - Flags: approval-comm-esr60+
Attachment #8979053 - Flags: approval-comm-beta+
In bug 1419417 Comment 144, Jörg K wrote:

> I promise a very quick review in bug 1462895 and immediate landing if there are no test failures.

Given that our tests in many cases hardcode the exact output, and we're going to change the output here, this is pretty much guaranteed to "break" the tests, with a false positive.
Follow-up: fix "Context-Type", fix typo in comment and remove comment referencing future code. r=me
You need to log in before you can comment on or make changes to this bug.