1284004 - Improve remote attachment handling

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[alta88]

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.

Comment 1

[alta88]

Attachment: test

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

Comment 2

[alta88]

Attachment: feedEnclosure.patch

Comment 3

[alta88]

Attachment: attachmentLen.patch

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.

Comment 4

[alta88]

Attachment: attachmentLen.patch

tweak for file urls already sized.

Comment 5

[Magnus Melin [:mkmelin]]

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 6

[Magnus Melin [:mkmelin]]

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

Comment 7

[alta88]

(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).

Comment 8

[Magnus Melin [:mkmelin]]

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

Comment 9

[alta88]

Attachment: feedEnclosure.patch

updated.

Comment 10

[alta88]

Attachment: attachmentLen.patch

updated.

Comment 11

[alta88]

Attachment: feedEnclosure.patch

also update jsmimeemitter.js part.

Comment 12

[alta88]

ping?

Comment 14

[Magnus Melin [:mkmelin]]

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

Comment 15

[Jorg K (CEST = GMT+2)]

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

Comment 16

[aleth [:aleth]]

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?

Comment 17

[alta88]

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

Comment 18

[aleth [:aleth]]

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.

Comment 19

[Jorg K (CEST = GMT+2)]

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

Comment 20

[alta88]

Attachment: fixAndTests.patch

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 21

[Magnus Melin [:mkmelin]]

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

Comment 22

[alta88]

Attachment: fixAndTests.patch

updated.

Comment 23

[alta88]

the fixAndTests.patch should be applied last, thanks.

Comment 24

[Jorg K (CEST = GMT+2)]

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