Improve remote attachment handling

RESOLVED FIXED in Thunderbird 51.0

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

unspecified
Thunderbird 51.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

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.
Posted file test
Test mbox with 2 feed items, one with content-type unknown, the second with content-type image/jpeg (to demonstrate inline display).
Posted patch feedEnclosure.patch (obsolete) — Splinter Review
Assignee: nobody → alta88
Attachment #8767415 - Flags: review?(mkmelin+mozilla)
Attachment #8767415 - Flags: feedback?(jonathan.protzenko)
Posted patch attachmentLen.patch (obsolete) — Splinter Review
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.
Attachment #8767751 - Flags: review?(mkmelin+mozilla)
Posted patch attachmentLen.patch (obsolete) — Splinter Review
tweak for file urls already sized.
Attachment #8767751 - Attachment is obsolete: true
Attachment #8767751 - Flags: review?(mkmelin+mozilla)
Attachment #8767778 - Flags: review?(mkmelin+mozilla)
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
Attachment #8767778 - Flags: review?(mkmelin+mozilla) → review+
(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 &)
Posted patch feedEnclosure.patch (obsolete) — Splinter Review
updated.
Attachment #8767415 - Attachment is obsolete: true
Attachment #8767415 - Flags: review?(mkmelin+mozilla)
Attachment #8767415 - Flags: feedback?(jonathan.protzenko)
Attachment #8772975 - Flags: review?(mkmelin+mozilla)
updated.
Attachment #8767778 - Attachment is obsolete: true
Attachment #8772976 - Flags: review+
also update jsmimeemitter.js part.
Attachment #8772975 - Attachment is obsolete: true
Attachment #8772975 - Flags: review?(mkmelin+mozilla)
Attachment #8772978 - Flags: review?(mkmelin+mozilla)
ping?
Duplicate of this bug: 655726
Sorry, probably won't get to it until next week.
Attachment #8772978 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d74812cfa1c0
https://hg.mozilla.org/comm-central/rev/9294fb7e2811
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
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?
Flags: needinfo?(alta88)
thanks for asking, but not right now, pls backout.
Flags: needinfo?(alta88)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Just for the record: Test failures for example here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=43497
Posted patch fixAndTests.patch (obsolete) — Splinter Review
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.
Attachment #8778717 - Flags: review?(mkmelin+mozilla)
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
Attachment #8778717 - Flags: review?(mkmelin+mozilla) → review+
updated.
Attachment #8778717 - Attachment is obsolete: true
Attachment #8779099 - Flags: review+
the fixAndTests.patch should be applied last, thanks.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.