Closed Bug 1352701 Opened 4 years ago Closed 4 years ago

Write test for display of embedded cid: images when message is stored in local folder

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 55.0

People

(Reporter: jorgk-bmo, Assigned: aceman)

References

Details

Attachments

(1 file, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1351966 +++
Now with bug 1351966 fixed, this is the critical bug since HTML display without images is no good.

In this version 55.0a1 (2017-03-29) (64-bit) I see embedded images:
M-C: 272ce6c2572164f5f6a9fba2a980ba9ccf
In this version 55.0a1 (2017-03-31) (64-bit) I don't:
M-C: 8df9fabf2587b7020889755acb9e75b664

So
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=272ce6c2572164f5f6a9fba2a980ba9ccf&tochange=8df9fabf2587b7020889755acb9e75b664

I need to narrow it down some more.
Good: 6ea713ccc9abea93126423fefb855d0e05
Bad:  60d7a0496a3673450ddbc37ec387525148
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6ea713ccc9abea93126423fefb855d0e05&tochange=60d7a0496a3673450ddbc37ec387525148

Looks like bug 1347817 which also discovered the problem fixed in bug 1351966.

Needs more debugging.
Aceman, with bug 1351966 fixed, the C-C treeherder is green again. That means, we have no test to detect this failure here. I find that rather disconcerting. 

Can you please write a test for me. Simply display a message with an embedded cid: image and check that the image is actually displayed. Maybe check test-general-content-policy.js or test-blocked-content.js to see how "is displayed" is tested.
Flags: needinfo?(acelists)
I forgot to say, test message is comm-central/mail/test/mozmill/composition/content-utf8-rel-only.eml
> Looks like bug 1347817 which also discovered the problem fixed in bug 1351966.

Can you bisect which of the bug 1347817 changesets is relevant here?  That will give us some idea which code to be looking at...
Flags: needinfo?(jorgk)
I will in a minute. Since I've worked on "content policy" a few times, I have a pre-canned debug patch which I've just applied. Stand by for the results, then I do the bisecting, OK? Thanks as always for your support!
Flags: needinfo?(jorgk)
The debug didn't yield much. We I get:

=== nsMsgContentPolicy::ShouldLoad: URI=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/April%202017?number=1|
=== nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 1/0/0

So the entire message is loaded, but no ShouldLoad() for the image, which is a message part.

I noticed that embedded images in news messages still work, where I get:

=== nsMsgContentPolicy::ShouldLoad: URI=|news://news.mozilla.org:119/mailman.1403.1480173815.16819.test-multimedia%40lists.mozilla.org?group=mozilla.test.multimedia&key=3453|
=== nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 1/0/0

for the message and later:

=== nsMsgContentPolicy::ShouldLoad: URI=|news://news.mozilla.org:119/mailman.1403.1480173815.16819.test-multimedia%40lists.mozilla.org?group=mozilla.test.multimedia&key=3453&part=1.2&filename=tweeting.jpg|
=== nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
=== nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|news://news.mozilla.org:119/mailman.1403.1480173815.16819.test-multimedia%40lists.mozilla.org?group=mozilla.test.multimedia&key=3453|

So there's my ShouldLoad() for the part, where the requests comes from the entire message with a code principal.

I've checked IMAP as well and there the images also show, same/similar/analogue debug as above.

So it's only mailbox:// that is broken.

OK, now for the bisecting.
Summary: Embedded cid: images not displayed → Embedded cid: images not displayed when message is stored in local folder
Sorry, no quick results, of the six changesets
https://hg.mozilla.org/mozilla-central/rev/ea39a16e4c97
https://hg.mozilla.org/mozilla-central/rev/a303ac6e7f6b
https://hg.mozilla.org/mozilla-central/rev/918682c3ff30
https://hg.mozilla.org/mozilla-central/rev/7012be88341d
https://hg.mozilla.org/mozilla-central/rev/89b3b96c32fe (comment only)
https://hg.mozilla.org/mozilla-central/rev/235e1d29916f (test only, but also some code changes)
I've backed out the last three and I'm still building. I'm wondering how independent these changes are, so I hope the thing won't fall over in a screaming heap by backing out parts of it.

It would be (not) funny to back it all out just to discover that another bug was responsible, but in the regression range that looks like the only candidate.
OK, after backing out the last three, the images still didn't show. Backing out the third one, 918682c3ff30, brings my image back, and the debug is:

=== nsMsgContentPolicy::ShouldLoad: URI=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/April%202017?number=1|
=== nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 1/0/0

=== nsMsgContentPolicy::ShouldLoad: URI=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/April%202017?number=1&part=1.2&filename=cllpafiinbhdbgdm.bmp|
=== nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
=== nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/April%202017?number=1|

So first we get ShouldLoad() for the message and later for the part where the message has a code principal.

So 918682c3ff30 did the damage.
Hmm, what I find strange that https://hg.mozilla.org/mozilla-central/rev/918682c3ff30 makes a difference to mailbox URLs but not to IMAP and NNTP URLs.
Further to comment #10, here are some typical URLs:
news://news.mozilla.org:119/mailman... snip ....mozilla.org?group=mozilla.test.multimedia&key=3453
imap://jorgk@jorgk.com@someserver.de:143/fetch>UID>.INBOX.HUHU>7
mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/April%202017?number=1
The last one my be different on Linux.

We note the following pre-paths:
news://news.mozilla.org:119
imap://jorgk@jorgk.com@someserver.de:143
mailbox://

So maybe some processing takes offence at that?
Aceman kindly provided me with a mailbox: URL on Linux, in fact, what I hadn't noticed, it comes with a log error:

Security Error: Content at
moz-nullprincipal:{2ebaab2e-dcda-4823-9b7a-386cbfd5954f} may not load or
link to
mailbox:///var/SSD/TB-profile/.thunderbird/nightly.default/Mail/Local%20Folders/bug154853-remoteContent?number=154204&part=1.2.4&filename=image_03.jpg.

The question remains: What makes mailbox: URLs special?
Attached patch WIP test (obsolete) — Splinter Review
Preliminary test for the missing image. The test fails in current trunk as the image is not properly loaded. We will see if the test starts to work when the underlying bug is fixed. If not, I'll have to check other attributes in the test.
Flags: needinfo?(acelists)
Attachment #8853770 - Flags: feedback?(jorgk)
Comment on attachment 8853770 [details] [diff] [review]
WIP test

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

Great start, but please add a test for a message in a folder since this is what fails visibly right now. Also, maybe, move the clean-up stuff to another patch.

::: mail/test/mozmill/composition/test-blocked-content.js
@@ +7,5 @@
>   */
>  
>  // make mozmill-one SOLO_TEST=composition/test-blocked-content.js
>  
> +var MODULE_NAME = "test-blocked-content";

I see you're cleaning up a few mistakes other people made.

::: mail/test/mozmill/composition/test-image-display.js
@@ +61,5 @@
> +  let msgc = open_message_from_file(file);
> +  let image = msgc.e("messagepane").contentDocument.getElementById("cidImage");
> +  check_image_size(msgc, image);
> +
> +  // Our image should also be in the forwarded composition.

If we compose anything (reply/forward), the image will be turned into a data: URL, and that should always be displayed. I don't think we need this part.

More useful would be to store the message into a folder and then check the display there, see test-forward-utf8.js or test-reply-multipart-charset.js (// Copy the message to a folder).
Attachment #8853770 - Flags: feedback?(jorgk) → feedback+
(In reply to Jorg K (GMT+2) from comment #14)
> Great start, but please add a test for a message in a folder since this is
> what fails visibly right now.

It fails now also in the opening-eml case. The URL in the eml case also turns into the mailbox:// schema.

> > +  // Our image should also be in the forwarded composition.
> 
> If we compose anything (reply/forward), the image will be turned into a
> data: URL, and that should always be displayed. I don't think we need this
> part.

I would leave it in. What if somebody breaks data: URLs? :) More likely, what if we break the conversion from cid: to data: ?
 
> More useful would be to store the message into a folder and then check the
> display there, see test-forward-utf8.js or test-reply-multipart-charset.js
> (// Copy the message to a folder).

OK, I can add that.
(In reply to :aceman from comment #15)
> The URL in the eml case also turns into the mailbox:// schema.
I know, most recently I've touched this in bug 1347598 comment #55. 

> I would leave it in. What if somebody breaks data: URLs? :) More likely,
> what if we break the conversion from cid: to data: ?
Well, you forward and not reply? BTW, there is not conversation from cid: to data:
The cid: gets converted to mailbox: or imap: or news: for display already, see comment #7 and comment #9. That's been like this for the last decade. What's new is that those are converted to data: when you put them into a composition (since the compose window is no longer an app type editor, thus blocking basically everything including those mailnews URLs, so we have to use data:). If you want, you can  leave that test, but then it would also make sense to check the data: scheme in the compose window for both reply and forward. But it's not really what this test here is about. Maybe you could structure it that you first test the display for the "eml" case and the folder case and then test the reply/forward.

> > More useful would be to store the message into a folder and then check the
> > display there ...
> OK, I can add that.
That will test that all the principal stuff and content policy stuff works, which is already covered by the "eml case", but we can never test enough ;-)

Boris, sorry, there's a second thread going in this bug now to implement a test. The bits I'd like you to look at are in comment #7 to comment #12.
Flags: needinfo?(bzbarsky)
Attached patch WIP test 2 (obsolete) — Splinter Review
Better?
Attachment #8853770 - Attachment is obsolete: true
Attachment #8853809 - Flags: feedback?(jorgk)
Comment on attachment 8853809 [details] [diff] [review]
WIP test 2

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

Excellent, many thanks. We might hold our horses for now and get the bug fixed. I'd rather not write any more comments here so that Boris doesn't have to skip the second thread about the test here.

::: mail/test/mozmill/composition/test-image-display.js
@@ +90,5 @@
> +  image = messageDoc.getElementById("cidImage");
> +  check_image_size(mc, image, gImageFolder.server.localStoreType + "://");
> +
> +  // Our image should also be in composition when the message is forwarded/replied.
> +  for (let msgOperation of [open_compose_with_forward(), open_compose_with_reply()]) {

Super elegant ;-)

@@ +99,5 @@
> +  }
> +}
> +
> +function teardownModule(module) {
> +}

Not needed, is it?
Attachment #8853809 - Flags: feedback?(jorgk) → feedback+
(In reply to Jorg K (GMT+2) from comment #18)
> > +  // Our image should also be in composition when the message is forwarded/replied.
> > +  for (let msgOperation of [open_compose_with_forward(), open_compose_with_reply()]) {
> 
> Super elegant ;-)

Too elegant ;) It runs both functions and opens the windows then runs the loop.

I'll change it to run sequentially so that we can get our head around it:
  for (let msgOperation of [open_compose_with_forward, open_compose_with_reply]) {
    let cwc = msgOperation();
Some debugging shows that for mailbox:// I get here
https://dxr.mozilla.org/mozilla-central/rev/38894655c89e68bcd8f45d31a0d3005f2c2b53db/dom/base/nsContentUtils.cpp#3235
with rv==NS_ERROR_DOM_BAD_URI (0x805303f4)
but for imap:// I don't.

So |sSecurityManager->CheckLoadURIWithPrincipal()| when called in an nsImapUrl succeeds, but when called on an nsMailboxUrl fails. Hmm.
(In reply to Jorg K (GMT+2) from comment #21)
> So lets see whether setting ALLOWS_PROXY will make it work for mailbox:
It doesn't.
More debugging:
bool schemesMatch = scheme.Equals(otherScheme, stringComparator);
https://dxr.mozilla.org/comm-central/rev/38894655c89e68bcd8f45d31a0d3005f2c2b53db/mozilla/caps/nsScriptSecurityManager.cpp#742
For imap, 'scheme' and 'otherScheme' are both "imap", for mailbox, 'scheme' is an empty string, and 'otherScheme' is "mailbox".

A little above, 'currentURI' is NullPrincipalURI for mailbox. For imap, 'currentURI' is nsImapUrl and hence schemesMatch==true and no call to CheckLoadURIFlags() which denies load based on the "dangerous" flag for mailbox.
OK, going back a bit to nsContentUtils::CanLoadImage():
For IMAP the part is loaded with a content principal, for mailbox it's loaded with a null principal and as detailed above, the load fails based on the schemes not matching and the "dangerous" flag. But the point is that the wrong loading principal is used.
So the obvious thing that changed is that mailbox:// used to get a codebase principal and now gets a nullprincipal.

Presumably news:// and imap:// are not affected because unlike mailbox:// they have actual hostnames, but more on this below.

Also, it sounds like it's confirmed that this is fallout from https://hg.mozilla.org/mozilla-central/rev/918682c3ff30 ?

Here's my best guess as to what happened here.  Before that changeset, BasePrincipal::CreateCodebasePrincipal worked fine for mailbox:// URIs.  But after that changeset the call to ContentPrincipal::Init lands in GenerateOriginNoSuffixFromURI which proceeds to fall through all the failure cases (because we have no hostport), lands in the QI to nsIStandardURL, this QI _fails_ (my guess, but some searching around comm-central suggests it's true; the mail urls don't implement nsIStandardURL), and ends up returning failure.  And that makes GetChannelURIPrincipal return failure, which makes GetChannelResultPrincipal return failure.  Still guessing here; this should probably be confirmed.

In any case, back in nsDocument::Reset, if GetChannelResultPrincipal returns failure, we ignore the return value and just pass nullptr for the "principal" argument to ResetToURI.  nsDocument::ResetToURI then tries, if aPrincipal is null, to create a codebase principal from its mDocumentURI (see the GetLoadContextCodebasePrincipal call), which I expect also fails, and then the document just has its initial principal, which is a nullprincipal.

Jorg, if you still have a tree build from rev 918682c3ff30 it might be good to confirm that the GetLoadContextCodebasePrincipal call in ResetToURI is in fact reached and returns an error rv.

Anyway, later changesets from bug 1347817 go ahead and explicitly return a nullpruncipal even earlier in the process if GenerateOriginNoSuffixFromURI fails.  So once all those patches are applied you won't even be reaching that GetLoadContextCodebasePrincipal call.

My question is this: why are we not just using the entire URL as the origin in this GenerateOriginNoSuffixFromURI code?  nsMailboxService::GetProtocolFlags returns the ORIGIN_IS_FULL_SPEC flag.  Shouldn't we maybe pay attention to it?  Sounds to me like we're generating bogus origins for news:// and nntp:// urls too!
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
Flags: needinfo?(amarchesini)
Blocks: 1347817
No longer blocks: 1340710
Oh, wire crossed, Boris replied, but let's post this anyway.

I'm rambling on, I hope Boris will help me out at some stage.

Let's capture all the call stacks that lead to nsContentUtils::CanLoadImage().

Mailbox:
nsContentUtils::CanLoadImage(nsIURI * aURI, ...) Line 3232	C++
nsDocument::MaybePreLoadImage(nsIURI * uri, ...) Line 9532	C++ - uses NodePrincipal()
nsHtml5TreeOpExecutor::PreloadImage(const nsAString & ...) Line 999	C++
nsHtml5SpeculativeLoad::Perform(nsHtml5TreeOpExecutor * aExecutor) Line 38	C++
nsHtml5TreeOpExecutor::FlushSpeculativeLoads() Line 307	C++
nsHtml5LoadFlusher::Run() Line 145	C++

nsContentUtils::CanLoadImage(nsIURI * aURI, ...) Line 3232	C++
nsImageLoadingContent::LoadImage(nsIURI * aNewURI, ...) Line 843	C++ - uses aDocument->NodePrincipal()
nsImageLoadingContent::LoadImage(const nsAString & aNewURI, ...) Line 769	C++
mozilla::dom::HTMLImageElement::LoadSelectedImage(bool aForce, bool aNotify, bool aAlwaysLoad) Line 1001	C++
mozilla::dom::HTMLImageElement::MaybeLoadImage() Line 700	C++

IMAP:
nsContentUtils::CanLoadImage(nsIURI * aURI, ...) Line 3232	C++
nsImageLoadingContent::LoadImage(nsIURI * aNewURI, ...) Line 843	C++ - uses aDocument->NodePrincipal()
nsImageLoadingContent::LoadImage(const nsAString & aNewURI, ...) Line 769	C++
mozilla::dom::HTMLImageElement::LoadSelectedImage(bool aForce, bool aNotify, bool aAlwaysLoad) Line 1001	C++
mozilla::dom::HTMLImageElement::MaybeLoadImage() Line 700	C++

So for Mailbox nsContentUtils::CanLoadImage() gets called twice with a null principal, for IMAP it gets called with a code principal. The caller gets the principal via aDocument->NodePrincipal(), so I really don't understand why that it doesn't always used the code-based principal as it should.
But note that we kinda broke creation of codebase principals in all sorts of case where it used to work that might not be using ORIGIN_IS_FULL_SPEC.  Not sure whether that's a problem in practice.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #25)
> My question is this: why are we not just using the entire URL as the origin
> in this GenerateOriginNoSuffixFromURI code? 
> nsMailboxService::GetProtocolFlags returns the ORIGIN_IS_FULL_SPEC flag. 
> Shouldn't we maybe pay attention to it?  Sounds to me like we're generating
> bogus origins for news:// and nntp:// urls too!

Because, in general, we can't be sure that they don't have ^ in the spec, right. See the comment at:
http://searchfox.org/mozilla-central/rev/990055a4902952e038cc350c9ffe1ac426d1c943/caps/ContentPrincipal.cpp#171

If this breaks too many addons, we may need to back it out on 55 and 56, and then we're home free for 57. That doesn't fix things for thunderbird though, so we may need to special-case a few more schemes (assuming they are in fact well-behaved).
Flags: needinfo?(bobbyholley)
Flags: needinfo?(amarchesini)
Umm, where does the previous comment leave us? TB is broken right now since a HTML viewer without images isn't really very good :-(
(In reply to Jorg K (GMT+2) from comment #29)
> Umm, where does the previous comment leave us?

As noted in that comment, we should (a) verify that the schemes that TB needs are well-behaved, and then (b) add them to the whitelist at the linked callsite.
So the real question is whether bare non-%-encoded '^' can appear in the GetSpec of one of the mailnews URIs, right?
The problem is that TB now offers a technology "JS Account" which lets you define new account types in a JS add-on, since binary add-ons are no longer, see bug 430716. Kent James has an add-on, ExQuilla (https://addons.mozilla.org/en-US/thunderbird/addon/exquilla-exchange-web-services/), which builds on that technology. I've just grepped through the add-on and I think he uses "exquilla" as the scheme.

What I'm trying to say is that there is not a fixed list of schemes used in TB. Well, I doubt that anyone will clone ExQuilla and add another scheme, but I assume that M-C doesn't really want to hard-code our schemes, of which we have a few, in fact, I've just removed some hard-coding here where you can see most of them:
https://hg.mozilla.org/comm-central/rev/c322afd9bf99c3b9c168e1319ccd4b419c209564#l1.58

Not all these are "message" schemes, we also use URLs to access servers for sending mail (smtp) and receiving mail (pop). There's also the cid scheme:
http://searchfox.org/comm-central/search?q=GetProtocolFlags(&case=false&regexp=false&path=

As for Boris's question:
We really have a lot of junk in our mailnews URLs, but as far as I know we only use
? & . / @ :
in our URLs and > for IMAP which should be %-encoded. Of course the user can choose folder names which become part of the URL, but all special characters are %-encoded. I've just tried the ominous ^ and got %5E.
I am fine hardcoding 5 or 10 mailnews schemes, as long as with #ifdef them to only be checked on TB builds. We could alternatively add a flag, but that flag is going to need to be manually added to all the schemes we support (along with an audit for ^), and so it's probably easier to just add them to the scheme whitelist.
> we also use URLs to access servers for sending mail (smtp) and receiving mail (pop)

Those don't use ORIGIN_IS_FULL_SPEC, right?  I'm primarily worried about the ORIGIN_IS_FULL_SPEC stuff here, which is just imap/mailbox/nntp.

Though if there are other schemes that actually end up as the principals of documents in Thunderbird we should try to figure out what's happening with them too, of course.
As far as I can see, ORIGIN_IS_FULL_SPEC is only set for imap/mailbox/nntp. Looking through ExQuilla it doesn't use the flag (maybe it should), but I guess that has a port number.
Attached patch 1352701-hardcoding-scheme.patch (obsolete) — Splinter Review
That hard-codes "mailbox" into M-C and embedded pictures work again. I didn't list the other schemes since they still work.

Thoughts?
Attachment #8854190 - Flags: review+
Thanks, that's very kind, but maybe Boris has a different idea. I've just tried what you said and it works.
Looks like an ugly hack :)
But at least it shows the test works and passes with the patch.
Also I can visually see images in messages when I look into my local dev folder.
I still think not using the entire spec as origin for URIs that explicitly have ORIGIN_IS_FULL_SPEC just means we will have security bugs.  How could we not?  We'll end up considering two different messages same-origin and whatnot...

So to be clear, I think the current trunk behavior for nntp and imap "works" in the sense of allowing the loads in this case, but I will be it also allows too much in general and hence is a security bug.
How can we get this fixed properly? And what is proper? While debugging this earlier on, I was amazed by highly complex code, so I wouldn't have any idea how to implement what Boris suggests.

As for the security issues:
mailbox stuff lives in the local machine and is accessible anyway, so I don't think there's a security issue.

If I understand it correctly, the origin for NNTP and IMAP is the part before the port? Or including the port? So it's basically the pre-path, right, so a combination of username and server, see comment #11. Considering everything on the same user account on the same server as same-origin isn't a problem, or is it?

I noticed that the ORIGIN_IS_FULL_SPEC is conditionally compiled based on IS_ORIGIN_IS_FULL_SPEC_DEFINED:
http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/netwerk/base/nsIProtocolHandler.idl#20
And that's only in the box for TB. Effectively, that just go discontinued, right?
> Considering everything on the same user account on the same server as same-origin isn't a problem, or is it?

It's totally a problem.  The threat model here is one of your emails reading _all_your_other_mails_.  That is not an OK thing for Thunderbird to allow.  And it will be allowed if you treat all things for the same user on the same server as same-origin.

ORIGIN_IS_FULL_SPEC exists specifically so Thunderbird will not have that security bug.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #41)
> > Considering everything on the same user account on the same server as same-origin isn't a problem, or is it?
> It's totally a problem.  The threat model here is one of your emails reading
> _all_your_other_mails_.
Hmm, perhaps I'm just not sensitised to security issues.

That said, how do we fix it maintaining the ORIGIN_IS_FULL_SPEC functionality? As I said in the first paragraph of comment #40, I wouldn't know where to start.
> That said, how do we fix it maintaining the ORIGIN_IS_FULL_SPEC functionality?

Bobby and I are hashing this out.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #43)
> Bobby and I are hashing this out.
Thanks Boris and Bobby. I'm subscribed to the other bug. So this bug will be only for providing the test.
Summary: Embedded cid: images not displayed when message is stored in local folder → Write test for display of embedded cid: images when message is stored in local folder
Attachment #8854190 - Attachment is obsolete: true
Assigning to Aceman since he wrote the test.
Assignee: nobody → acelists
Comment on attachment 8853809 [details] [diff] [review]
WIP test 2

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

Nice, fails when the image doesn't show and passes when applying the M-C patch that makes it show. Since Bobby and Boris are working on a proper fix for M-C in another bug, we'll soon be able to land this patch.

::: mail/test/mozmill/composition/test-image-display.js
@@ +5,5 @@
> +/**
> + * Tests that we load and display embedded images in messages.
> + */
> +
> +// make mozmill-one SOLO_TEST=composition/test-image-display.js

The mozmill-one goes at the end, right?
This is not strictly a composition test since it was first requested as a test for image display for messages in folder. But since we're also testing the somewhat unrelated reply/forward, it might as well stay in "composition".

@@ +89,5 @@
> +  messageDoc = mc.e("messagepane").contentDocument;
> +  image = messageDoc.getElementById("cidImage");
> +  check_image_size(mc, image, gImageFolder.server.localStoreType + "://");
> +
> +  // Our image should also be in composition when the message is forwarded/replied.

Would it be possible to move this to a separate subtest, so we get a more detailed report of anything fails.
(In reply to Jorg K (GMT+2) from comment #46)
> > +  // Our image should also be in composition when the message is forwarded/replied.
> 
> Would it be possible to move this to a separate subtest, so we get a more
> detailed report of anything fails.

You mean splitting the forward/reply part?
(In reply to :aceman from comment #47)
> You mean splitting the forward/reply part?
Well, there is only one function here: function test_cid_images().
It would be nice, as you've told me before, to make it multiple test functions so we know in more detail what failed. You're the Mozmill master ;-)
I hope there will be more functions added in the future :)
But yes, I can split it.
Attached patch text v2.1Splinter Review
Attachment #8853809 - Attachment is obsolete: true
Attachment #8855488 - Flags: review?(jorgk)
Severity: critical → major
Status: NEW → ASSIGNED
Component: General → Message Compose Window
Flags: in-testsuite+
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Comment on attachment 8855488 [details] [diff] [review]
text v2.1

Nice, test passes now since M-C got fixed so embedded images work again. I'll get it landed.
Attachment #8855488 - Flags: review?(jorgk) → review+
https://hg.mozilla.org/comm-central/rev/a40da21295b132d4c5e7abcbcc1b5ea4641c13a3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
You need to log in before you can comment on or make changes to this bug.