Remote urls may contain ? query strings in the url, in addition to ?part=. This is not handled by the PART_RE regex, leading to incorrect counts in attachment pane as well as the MsgHdrToMimeMessage api.

Test mbox with 2 feed items, one with content-type unknown, the second with content-type image/jpeg (to demonstrate inline display).

Part 2 - have libmime pass on a remote link's reported size to the header sink so it can be displayed like for internal/file attachments.

tweak for file urls already sized.

Comment on attachment 8767415 [details] [diff] [review] feedEnclosure.patch Review of attachment 8767415 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/msgHdrViewOverlay.js @@ +1785,5 @@ > // Remove [?&]part= from remote urls, after getting the partID. > + // Remote urls, unlike non external mail part urls, may also contain query > + // strings starting with ?; PART_RE does not handle this. > + if (url.startsWith("http") || url.startsWith("file")) { > + match = url.match(/[?&]part=[^part=]+$/); shouldn't this be something like match = url.match(/[?&]part=[^&]+$/);

Comment on attachment 8767778 [details] [diff] [review] attachmentLen.patch Review of attachment 8767778 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/msgHdrViewOverlay.js @@ +641,3 @@ > !last.isDeleted) { > let size = parseInt(value); > +dump("addAttachmentField: external size:value - "+size+":"+value+"\n"); left over debug dump should go of course

(In reply to Magnus Melin from comment #5) > Comment on attachment 8767415 [details] [diff] [review] > feedEnclosure.patch > > Review of attachment 8767415 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/base/content/msgHdrViewOverlay.js > @@ +1785,5 @@ > > // Remove [?&]part= from remote urls, after getting the partID. > > + // Remote urls, unlike non external mail part urls, may also contain query > > + // strings starting with ?; PART_RE does not handle this. > > + if (url.startsWith("http") || url.startsWith("file")) { > > + match = url.match(/[?&]part=[^part=]+$/); > > shouldn't this be something like > > match = url.match(/[?&]part=[^&]+$/); the idea was to not match "part=" other than the real "?part=1.2$", eg, that always ends the string. it seems like [^&] is not be enough for, eg, http://myurl.com?part=foo?part=1.2 did you mean [^&?]; that probably works (but seems less sure in a way i can't make up an example for).

But that url isn't valid, the second ? would have to be encoded. (Or rather, it would be an &)

updated.

updated.

also update jsmimeemitter.js part.

ping?

Sorry, probably won't get to it until next week.

https://hg.mozilla.org/comm-central/rev/d74812cfa1c0 https://hg.mozilla.org/comm-central/rev/9294fb7e2811

Looks like this broke some tests: TEST-UNEXPECTED-FAIL | mailnews/mime/test/unit/test_attachment_size.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | mailnews/mime/test/unit/test_attachment_size.js | gStreamListener.onStopRequest - [gStreamListener.onStopRequest : 199] false == true Backout or can you fix it quickly?

thanks for asking, but not right now, pls backout.

https://hg.mozilla.org/comm-central/rev/d9ab6d03bab9741b48700feb09c0f05a65883054 Backed out changeset 9294fb7e2811 (bug 1284004) for test failures. https://hg.mozilla.org/comm-central/rev/f94907eab997537c91c3657836b13dacc54e1deb Backed out changeset d74812cfa1c0 (bug 1284004) for test failures.

Just for the record: Test failures for example here: https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=43497

simply need to initialize m_isExternalLinkAttachment, as random bits in the memory location incorrectly evaluate to true in an uncommon code path. tests too. all pass on a debug build.

Comment on attachment 8778717 [details] [diff] [review] fixAndTests.patch Review of attachment 8778717 [details] [diff] [review]: ----------------------------------------------------------------- r=mkmelin with the below changed ::: mailnews/compose/public/nsMsgAttachmentData.h @@ +54,5 @@ > > int32_t m_size; // The size of the attachment. May be 0. > nsCString m_sizeExternalStr; // The reported size of an external attachment. Originally set at "-1" to mean an unknown value. > bool m_isExternalAttachment; // Flag for determining if the attachment is external > + bool m_isExternalLinkAttachment = false; // Flag for determining if the attachment is external and an http link. It should probably be initialized here instead: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#4953 (and change m_isExternalAttachment(0) to m_isExternalAttachment(false) while you're at it) ::: mailnews/mime/test/unit/test_attachment_size.js @@ +233,5 @@ > var gMessageHeaderSink = { > handleAttachment: function(aContentType, aUrl, aDisplayName, aUri, > aIsExternalAttachment) { > + dump("*** handleAttachment: aContentType:aDisplayName:aIsExternalAttachment:aUrl - "+ > + aContentType+":"+aDisplayName+":"+aIsExternalAttachment+":"+aUrl+"\n\n"); this should be removed of course

updated.

the fixAndTests.patch should be applied last, thanks.

https://hg.mozilla.org/comm-central/rev/294c2e76579a https://hg.mozilla.org/comm-central/rev/dcba7b05f616 https://hg.mozilla.org/comm-central/rev/512d3c0a11d2