Open Bug 1297653 Opened 5 years ago Updated 3 years ago

html attachment with CSS breaks email rendering

Categories

(Thunderbird :: Message Reader UI, defect)

45 Branch
defect
Not set
normal

Tracking

(Not tracked)

People

(Reporter: layus, Unassigned)

References

Details

Attachments

(4 files, 5 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160806094113

Steps to reproduce:

Attach an html document with css to the email


Actual results:

The email does not render correctly. The html attachment contains custom styles that leak into the email, making the text difficult to read.


Expected results:

The html attachment should be rendered separately, isolating the styles from the email itself.
Attachment #8784304 - Attachment mime type: message/rfc822 → text/plain
(In reply to Guillaume Maudoux [:layus] from comment #0)
> Expected results:
> The html attachment should be rendered separately, isolating the styles from
> the email itself.
Sadly, that's not going to happen in the short/medium term.
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → 1287640
Workaround: Don't view attachments inline:
Uncheck: View > Display Attachments Inline.
Summary: html attachment style breaks email rendering → html attachment with CSS breaks email rendering
> Workaround: Don't view attachments inline:
Hmm... well, yes ;-).

Do you think it would be easily patchable ?
(In reply to Jorg K (GMT+2, PTO during summer) from comment #1)
> Sadly, that's not going to happen in the short/medium term.

(In reply to Guillaume Maudoux [:layus] from comment #3)
> Do you think it would be easily patchable ?
Not at all since you'd have to open up how the different message parts are rendered. Maybe you need an iframe for each individual part. I don't think it's going to happen unless you do it yourself. Hey, it's open source, you hate it, you fix it ;-)
Attached patch sandbox-html-attachments.diff (obsolete) — Splinter Review
Et voilà! This fixes my issue, and does not break thunderbird.
Not too sure what testsuite to run, nor how to run it.

Must be something like `./mach test XXX`, right ?
Flags: needinfo?(jorgk)
Attachment #8785228 - Flags: review?(jorgk)
(In reply to Guillaume Maudoux [:layus] from comment #5)
> Must be something like `./mach test XXX`, right ?

Hi, thanks for the patch. On a change like this, you'd normally run the entire test suite via our try server:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central
You need Level 1 access rights to push to the try server.

I'll check the patch locally and then send it to the try server. Watch this space ;-)
Flags: needinfo?(jorgk)
Comment on attachment 8785228 [details] [diff] [review]
sandbox-html-attachments.diff

Richard, this is about displaying HTML attachments in a "sandbox" iframe, so their CSS doesn't carry over into the rest of the message display. A good idea really.

Can you please review the CSS for me, I'll do the MIME part. I'm not sure why you'd want |height: 30em;|
Well, I suppose that would give you a nice box with scrollbar (I haven't tried it yet, I need to wait for local build to finish).

Oh, I just noticed,  Guillaume, you need to change the CSS for the other platforms as well, so currently I can't even try/review it on Windows.
Attachment #8785228 - Flags: review?(jorgk) → feedback?(richard.marti)
There is only CSS code for Linux in the patch. I'll try it this evening how it looks without code and then with the code copied to win and osx.
(In reply to Richard Marti (:Paenglab) from comment #9)
> ... and then with the code copied to win and osx.
Yes, but the patch author needs to do this. We can do the parts he can't do due to missing access rights.
(In reply to Jorg K (GMT+2, PTO during summer) from comment #10)
> (In reply to Richard Marti (:Paenglab) from comment #9)
> > ... and then with the code copied to win and osx.
> Yes, but the patch author needs to do this. We can do the parts he can't do
> due to missing access rights.

That's right. But with this I can already check how it looks.
I have try access for firefox, I assume it extends also to thunderbird (comm-central), right ?
Should I trigger all the tests or only a subset ?
> I'm not sure why you'd want |height: 30em;|
> Well, I suppose that would give you a nice box with scrollbar

Well, an iframe does not adapt to its content, so I had to provide a sane hardcoded value. 30em seems good as it is not too high related to average emails size, and adapts to the font size.
I stopped when I got something good enough. Improvements are welcome.
(In reply to Guillaume Maudoux [:layus] from comment #12)
> I have try access for firefox, I assume it extends also to thunderbird
> (comm-central), right ?
Yes.

> Should I trigger all the tests or only a subset ?
Good question. I depends on what you want to do and what you are working on.
Linux is normally fastest. Typically most errors already show on a Linux 64 opt build.

I use a variety of combinations, for example:
hg qnew -m "try: -b o -p linux64,macosx64 -u xpcshell" try
-b o = build optimised, you can also do |-b d| or |-b do|. You can add platforms, for example win32. The -u takes xpcshell, mozmill or all. And the push would be:
hg push -f -r tip cc-try && hg qpop && hg qdelete try
with |cc-try| appropriately aliased.

So for the first try here I used: |try: -b o -p linux64 -u all|

Note that the Linux and Mac compilers are very fussy, so at times it's good just to request a build on Mac. Just have a look at what people use:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central
Keep in mind that server resource cost money, even if you don't pay for them.
Attached patch sandbox-html-attachments.patch (obsolete) — Splinter Review
Added the Windows and Mac parts.
Guillaume, please put a proper commit comment "Bug xxx - ..." and if you don't mind, use extension .patch.
Attachment #8785228 - Attachment is obsolete: true
Attachment #8785228 - Flags: feedback?(richard.marti)
Attached patch sandbox-html-attachments.patch (obsolete) — Splinter Review
Update to keep consistent with the if condition. I cannot obsolete your patch, so I submit a new one.
I am trying to push to try.
Comment on attachment 8785275 [details] [diff] [review]
sandbox-html-attachments.patch

Works OK on Windows at first glance ;-)

What happens if the content of the attachment is not 30em high? Then you'll get an iframe which is partly empty. Could that be improved? Otherwise I really like the idea and the initiative!
Could not test yet. Is the border around the iframe needed? We have already a horizontal separator with title for the attachments.
> What happens if the content of the attachment is not 30em high?
> Then you'll get an iframe which is partly empty. Could that be improved?

iframe's are not flexible at all. You must hardcode their size, or get hardcoded defaults.
Javacript can help to dynamically set the size of an iframe based on it's content, but I will not implement that (is javascript even supported here ?).

> Is the border around the iframe needed?
> We have already a horizontal separator with title for the attachments.

I added it because it makes the scroll bar less weird, and it hints the user that their may be other inline attachments when the document is very small (<30em).
A few remarks:
1) Yes, some tools get the try link wrong.
2) No need to submit to Linux *and* Mac for a first round.
3) Initial patch and initial try:
   https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=148cff11d62e
   showed heaps of errors.
   MIME is a very sensitive area, so if you change something, heaps of things can break.
4) Please don't use Mozreview. I'm not familiar with it and I don't want to spend the
   time to get acquainted. So if you don't mind, please just attach the patch.
try: -b o -p linux64,macosx64 -u xpshell,mozmill isn't right.
Needs to be -u all, or otherwise xpcshell
I always S/MIME sign my messages and this moves also my own HTML message text into an iframe. This shouldn't happen and looks weird when this is the only text, especially when my text is taller than 300px and I have two scrollbars. Only real attachments should be in a iframe.
@Paenglab: Excuse my ignorance, but what is S/MIME signing, and what tool do you use to apply it ?
Also, how can you tell real attachments from the normal message? MIME lib does not seem to contain information about that.

@Jorg K: Right, I cancelled the builds. That being said, I already fixed some of the issues in the new version of the patch.

Generally speaking, I must admit that MIME is a sensitive area, and the code is *very* difficult to understand, and overly complicated. The email is just one chunck of html with sections and such. I would never have expected that :-).

Now, I am willing to continue working on this, but I could really use some hints on how MIME works internally, and if the tactic I have used has any chance to fix my issue without breaking too many expectations from other modules.
(In reply to Guillaume Maudoux [:layus] from comment #26)
> @Paenglab: Excuse my ignorance, but what is S/MIME signing, and what tool do
> you use to apply it ?
Thunderbird can do signing with a certificate using S/MIME. See: Account settings, Security. You need to get a certificate from somewhere, Comodo and others hand out free ones.

> Also, how can you tell real attachments from the normal message? MIME lib
> does not seem to contain information about that.
Well, they would be different MIME parts. Try looking at an e-mail with View > Message Body As > All Body Parts (need to set preference mailnews.display.show_all_body_parts_menu).

> The email is just one chunk of html with sections and such.
No. The e-mail is one chunk of data with multiple parts. There can be a HTML part which serves as main message display, and there can be HTML and other attachments. mimethtm.cpp does HTML parts, but it's all held together in mimemult.cpp (multi part) and mimemalt/rel.cpp (multipart alternative/related).

> Now, I am willing to continue working on this, but I could really use some
> hints on how MIME works internally, ...
I'm afraid nobody knows. As you can see from the header of the files, this stuff is originally from IBM. The person who worked on this has left, that's why we don't touch MIME if we can avoid it. There are plans to replace the whole C++ library with a re-implementation in JS (JSMime), but sadly they got delayed (https://groups.google.com/d/msg/mozilla.dev.apps.thunderbird/EB6fqF3Hngk/q8NEGfI4BgAJ).

Having said that, myself and Magnus have (reluctantly) accepted some patches here and there. For some fun, read bug 1251783 comment #20.

If this bug is important to you, we will most likely be able to review a patch that doesn't break tests (or you need to fix the tests that break), but please don't expect much support while you're working on it.

I personally like the HTML attachment sitting nicely in an iframe ;-)
Attached file Font Test.eml
This is a HTML only message which shows the iframe when it shouldn't.
An other message which shows the content in a iframe. This time a MIME/multi-part.
@Paenglab: Thank you for the example emails.

@Jorg K: Thank you for the explanations.
You made it clear that Thunderbird is short on manpower. Minor issues like this with huge impact are too time-consuming. I will continue working on it, but a a slow pace. This should not be an issue, right?

My biggest fear is that I will have to patch the tests themselves to make them work with the fix. And there are many failing tests... I will see what I can do.
(In reply to Guillaume Maudoux [:layus] from comment #31)
> @Jorg K: Thank you for the explanations.
> You made it clear that Thunderbird is short on manpower.
Very much so. The team of unpaid volunteers is stressed and overworked. Please take a look at:
https://treeherder.mozilla.org/#/jobs?repo=comm-central
All the stuff in orange and red demands attention. Since we build on the Mozilla platform, we are in a state of "continuous integration" and are constantly running behind a team of hundreds of paid Mozilla developers whose changes we have to adapt too. A good example is today, where I woke up to some major new bustage (the orange/red) Xpcshell test failures.

> Minor issues like
> this with huge impact are too time-consuming. I will continue working on it,
> but a a slow pace. This should not be an issue, right?
Just get it done before we trash libmime in a few years down the track.

> My biggest fear is that I will have to patch the tests themselves to make
> them work with the fix.
Changes to tests can be of different nature. Of course we can't accept changes where a test is changed/disabled since it now fails due to the required function not working any more. If, on the other hand, there were a test that assumes a certain structure of a message and that has changed, then of course the test needs to be adapted. Looking at the test failures, surely test-charset-edit.js for example shouldn't be affected by your changes since I bet there it doesn't use a HTML attachment. I think if you make sure only *attachments* are sand-boxed, there will be very few tests failing.
I am trying to run tests, but on my current branch most test will fail even without my patch applied.

> $ ./mozilla/mach xpcshell-test mailnews
> ...
> Summary
> =======
> 
> Ran 677 tests (661 parents, 16 subtests)
> Expected results: 381
> Unexpected results: 270 (FAIL: 270)
> Skipped: 26

Is there some stable revision, or am I missing something important ?
A typical error with my current revision (changeset:   19932:7ca0a0644ba1) looks like

> ./mozilla/mach xpcshell-test mailnews/mime/test/unit/test_mimeStreaming.js
> Elapsed: 1.58s; From _tests: Kept 28549 existing; Added/updated 0; Removed 0 files and 0 directories.
>  0:00.43 LOG: MainThread INFO Running tests sequentially.
>  0:00.43 SUITE_START: MainThread 1
>  0:00.44 LOG: Thread-1 INFO profile dir is /tmp/firefox/xpcshellprofile
>  0:00.44 TEST_START: Thread-1 mailnews/mime/test/unit/test_mimeStreaming.js
>  0:00.44 LOG: Thread-1 INFO mailnews/mime/test/unit/test_mimeStreaming.js | full command: ['/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/dist/bin/xpcshell', '-g', '/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/dist/bin', '-a', '/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/dist/bin', '-r', '/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/dist/bin/components/httpd.manifest', '-m', '-s', '-e', 'const _HEAD_JS_PATH = "/home/gmaudoux/projets/comm-central/mozilla/testing/xpcshell/head.js";', '-e', 'const _MOZINFO_JS_PATH = "/tmp/firefox/xpcshellprofile/mozinfo.json";', '-e', 'const _TESTING_MODULES_DIR = "/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/_tests/modules/";', '-f', '/home/gmaudoux/projets/comm-central/mozilla/testing/xpcshell/head.js', '-p', '/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/temp/xpc-plugins-JAOpFe', '-e', 'const _SERVER_ADDR = "localhost"', '-e', 'const _HEAD_FILES = ["/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/mailnews/mime/test/unit/head_mime.js"];', '-e', 'const _TAIL_FILES = ["/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/mailnews/mime/test/unit/tail_mime.js"];', '-e', 'const _JSDEBUGGER_PORT = 0;', '-e', 'const _TEST_FILE = ["/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/mailnews/mime/test/unit/test_mimeStreaming.js"];', '-e', 'const _TEST_NAME = "mailnews/mime/test/unit/test_mimeStreaming.js"', '-e', '_execute_test(); quit(0);']
>  0:00.44 LOG: Thread-1 INFO mailnews/mime/test/unit/test_mimeStreaming.js | current directory: '/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/mailnews/mime/test/unit'
>  0:00.44 LOG: Thread-1 INFO mailnews/mime/test/unit/test_mimeStreaming.js | environment: ['LD_LIBRARY_PATH=/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/dist/bin', 'XPCOM_DEBUG_BREAK=stack-and-abort', 'MOZ_CRASHREPORTER=1', 'XPCSHELL_TEST_TEMP_DIR=/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/temp/xpc-other-IbJeSz', 'MOZ_DISABLE_NONLOCAL_CONNECTIONS=1', 'XPCSHELL_TEST_PROFILE_DIR=/tmp/firefox/xpcshellprofile', 'MOZ_CRASHREPORTER_NO_REPORT=1']
>  0:00.55 PROCESS_OUTPUT: Thread-1 (pid:8769) Full command: ['/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/dist/bin/xpcshell', '-g', '/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/dist/bin', '-a', '/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/dist/bin', '-r', '/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/dist/bin/components/httpd.manifest', '-m', '-s', '-e', 'const _HEAD_JS_PATH = "/home/gmaudoux/projets/comm-central/mozilla/testing/xpcshell/head.js";', '-e', 'const _MOZINFO_JS_PATH = "/tmp/firefox/xpcshellprofile/mozinfo.json";', '-e', 'const _TESTING_MODULES_DIR = "/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/_tests/modules/";', '-f', '/home/gmaudoux/projets/comm-central/mozilla/testing/xpcshell/head.js', '-p', '/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/temp/xpc-plugins-JAOpFe', '-e', 'const _SERVER_ADDR = "localhost"', '-e', 'const _HEAD_FILES = ["/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/mailnews/mime/test/unit/head_mime.js"];', '-e', 'const _TAIL_FILES = ["/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/mailnews/mime/test/unit/tail_mime.js"];', '-e', 'const _JSDEBUGGER_PORT = 0;', '-e', 'const _TEST_FILE = ["/home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/mailnews/mime/test/unit/test_mimeStreaming.js"];', '-e', 'const _TEST_NAME = "mailnews/mime/test/unit/test_mimeStreaming.js"', '-e', '_execute_test(); quit(0);']
> (pid:8769) "[8769] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /home/gmaudoux/projets/comm-central/mozilla/toolkit/crashreporter/nsExceptionHandler.cpp, line 2723"
>  0:00.57 PROCESS_OUTPUT: Thread-1 (pid:8769) "ExceptionHandler::GenerateDump cloned child 8787"
>  0:00.58 PROCESS_OUTPUT: Thread-1 (pid:8769) "ExceptionHandler::SendContinueSignalToChild sent continue signal to child"
>  0:00.58 PROCESS_OUTPUT: Thread-1 (pid:8769) "ExceptionHandler::WaitForContinueSignal waiting for continue signal..."
>  0:00.76 TEST_END: Thread-1 FAIL, expected PASS
> xpcshell return code: -11
> PROCESS-CRASH | mailnews/mime/test/unit/test_mimeStreaming.js | application crashed [unknown top frame]
> Crash dump filename: /home/gmaudoux/projets/comm-central/obj-x86_64-pc-linux-gnu/temp/xpc-other-IbJeSz/1f3e57ee-2c25-a714-5c218958-4c191b3d.dmp
> MINIDUMP_STACKWALK not set, can't process dump.
>  0:00.76 LOG: MainThread INFO INFO | Result summary:
>  0:00.76 LOG: MainThread INFO INFO | Passed: 0
>  0:00.76 LOG: MainThread INFO INFO | Failed: 1
>  0:00.76 LOG: MainThread INFO INFO | Todo: 0
>  0:00.76 LOG: MainThread INFO INFO | Retried: 0
>  0:00.76 SUITE_END: MainThread 
> Summary
> =======
> 
> Ran 1 tests
> Expected results: 0
> Unexpected results: 1 (FAIL: 1)
> 
> Unexpected Results
> ==================
> 
> FAIL mailnews/mime/test/unit/test_mimeStreaming.js
>
(In reply to Guillaume Maudoux [:layus] from comment #33)
> Is there some stable revision, or am I missing something important ?
I don't know that you're doing. At the current tip almost all tests will pass unless affected by current bustage (bug 1300059): https://treeherder.mozilla.org/#/jobs?repo=comm-central

Click on one of the "X1"s on Linux to see what's currently failing, about six and we're close to fixing that, too.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 31052
Let's see whether we can revive the patch here and fix it.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
The old patch still applied, but didn't compile any more. This one compiles (BMO hangs when attaching it, so I'll try in the next comment).

The major flaw is that all HTML parts are now placed into an Iframe, as Richard pointed out in comment #29 and comment #30. Only inline attachment should go into the Iframe.

Most likely due to this deficiency we have all the test failures mentioned in comment #33 and earlier.

So the main issue that needs to be solved here is to only do the Iframe-wrapping for inline attachments. That should be doable.
Assignee: nobody → jorgk
Attachment #8785275 - Attachment is obsolete: true
Attachment #8785280 - Attachment is obsolete: true
Attachment #8785311 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
That patch no longer applies, since I removed writing the <div class="moz-text-html" ...> in bug 1463133. We could put it back, but only for subordinate parts, not the main body.
@Jörg:
The idea of putting each MIME part into its own <iframe> is very old. I had tried doing something like that in bug 9942 (note the 4-digit bug number!), with a patch 14 years ago. heh! The bug there was that we want headers and body to scroll together, not a fixed header section. We never wanted the fixed header, it was needed for security.

The problem why this didn't land is that we want a single scroll bar for the entire message body, including all inline attachments. But <iframe>s can't size to their content, they get their size from above, from the container. That meant that all MIME parts get their own scroll bar.

<iframe seamless> was supposed to be the solution, but was never implemented. If we can find a solution for that, we could put the header, the body, and each attachment into their own <iframe>s, and then scroll all together. I actually have an idea how to do that.

But this is a big frontend change, and is highly likely to cause some regressions, so there's no way we can do that for 52.8.1 in the next 2 days. If you really want it, we could consider it for 52.9.
Rebased, but not working at all.

(In reply to Ben Bucksch (:BenB) from comment #41)
> That meant that all MIME parts get their own scroll bar.
I think that's acceptable if only done for the subordinate parts, not the body. That would already help, since then these parts don't interfere any more.
Attachment #8953780 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) (bustage-fix and urgent reviews only) from comment #42)
> Rebased, but not working at all.
Yes, not working at all. The HTML is placed outside the <iframe>. That's the same problem we tried to address in bug 1462895 which already exposed a logic error in the eof() processing for the child object relative to the parent object :-( So here the same logic error is exposed again.
Hey Jörg,

thanks for your initiative. I am happy that you like that idea of putting MIME parts into <iframe>s.

Don't waste your time on refreshing this patch. I don't think this would work at all. You'd get a horrible UI. If the patch would work, you'd see the horrible UI, and scrape it.

The approach taken with the patch here is wrong. The right fix isn't in libmime, but in the frontend. You might just need minor modifications to libmime to support it. The frontend should create the iframes, and then load each MIME part in there as a URL. That avoids all that HTML-munging and -escaping, and also gives the frontend control over the presentation, as it should have.

If you want to run with this general idea of iframes, I think the best start is my old patch from bug 9942. It only separates body and headers, not each MIME part, but it starts from the right angle. You go from there. You'll also see why I abandonned it. But meanwhile I saw some ideas how to fix the scroll bars, so there's hope.
I just came here while triaging bugs. The reporter was happy to play with it and I picked up the pieces and refreshed them twice so far.

The HTML-munging and -escaping is indeed ugly :-( - On the other hand, I don't think that individual scrollbars for inline HTML attachments would be too terrible.
Assignee: jorgk → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.