Closed Bug 953165 Opened 6 years ago Closed 6 years ago

picture in feed not showing

Categories

(Thunderbird :: Message Reader UI, defect)

24 Branch
x86_64
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: luuk34, Assigned: alta88)

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

1. i linked the RSS-feed from: http://www.nu.nl/feeds/rss/algemeen.rss
2. articles show up with an attached image, but image is not shown
3. when trying to open an attachement, next error message is shown:
The file http://bin.snmmd.nl/m/m1mxdj4a8gsy_sqr256.jpg?part=1.2 cannot be found. Please check the location and try again.
4. visiting  the link http://bin.snmmd.nl/m/m1mxdj4a8gsy_sqr256.jpg (without the '?part=' stuff) shows the image


Actual results:

image is not shown.


Expected results:

if 'open' was chosen, the image should be show
if 'save-as' was chosen the image should be saved.
This also happens on linux (opensuse), version 24
OS: Windows 7 → All
Attached patch feedattachmentPart.patch (obsolete) — Splinter Review
well, that server is too smart for its own good.  everyone else ignores the unnecessary ?part and returns the image; here they return an error..

nevertheless, a bogus ?part doesn't make sense for remote urls.  it's not changed in mimei.cpp since we do want to have the partID in the url filter up, even for remote attachments.
Assignee: nobody → alta88
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8351495 - Flags: review?(mkmelin+mozilla)
Sorry, i've never submitted a bug to thunderbird before....
But do i get this correct, and is above message indicating that this 'bug' is fixed?
and, if so, how can i test this?
(compiling stuff on a linux machine should not be a problem ;)
Comment on attachment 8351495 [details] [diff] [review]
feedattachmentPart.patch

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

::: mail/base/content/msgHdrViewOverlay.js
@@ +1804,5 @@
>    let match = GlodaUtils.PART_RE.exec(url);
>    this.partID = match && match[1];
> +  // Remove ?part= from remote urls, after storing the partID.
> +  this.url = (url.startsWith("http") || url.startsWith("file")) ?
> +               url.split("?part=")[0] : url;

This doesn't work out for &part=, or if part isn't the last param. 
It looks like you have partID already above, so you can probably use that to remove it for all cases.
Attachment #8351495 - Flags: review?(mkmelin+mozilla) → review-
I think i'm missing something here

when i look at the xml which is found at the url from my first post, i see something like:
.....
                  <link>http://www.nu.nl/buitenland/3664039/turkse-minister-wijst-eu-kritiek-van-hand.html</link>
          <guid>http://www.nu.nl/buitenland/3664039/index.html</guid>
                <description>De nieuwe Turkse minister voor Europese Zaken, Mevlüt Çavusoglu, heeft in een zondag verspreide verklaring kritiek van de EU op Ankara van de hand gewezen. </description>
                <pubDate>Sun, 29 Dec 2013 14:14:39 +0100</pubDate>
        <category>Algemeen</category>
                          <enclosure url="http://bin.snmmd.nl/m/m1mxdptao189_sqr256.jpg" type="image/jpeg" />
          <copyrightPhoto>nu.nl</copyrightPhoto>
        
      </item>
.....


The attached picture is enclosed (in the 'enclosure'-tag) as:
http://bin.snmmd.nl/m/m1mxdptao189_sqr256.jpg

If the attachment just refers to this (unchanged) URL, than all should be good.

Can anyone explain why there is a '?part=' ?
Where is it coming from, and why is it added to this URL?
Attached patch feedattachmentPart.patch (obsolete) — Splinter Review
Attachment #8351495 - Attachment is obsolete: true
Attachment #8351708 - Flags: review?(mkmelin+mozilla)
the ?part= is added automatically in attachment mime processing.  prior to feeds being implemented, attachment urls were only message urls with the partID # being used to identify them in a message stored in a berkely mbox file.  feed enclosures use the same mime construct, but a remote link is different from an mbox attachment url.

btw, if you've never built Tb it might be a bit of a curve just for this.  it's easier (if you're familiar with the tools) to unzip the omni.ja file, find msgHdrViewOverlay.js in the tree, make the changes per the patch, and re-zip (next update will wipe them though).
Comment on attachment 8351708 [details] [diff] [review]
feedattachmentPart.patch

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

Looks good, thx! r=mkmelin
Attachment #8351708 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8351708 [details] [diff] [review]
feedattachmentPart.patch

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

Oh, wait. It needs to preserve ? if it's the first param
Attachment #8351708 - Flags: review+ → review-
i don't understand.  for a feed enclosure part, constructed by us, the only string added to the url as a result of mime is ?part= and it's always appended making it first and last and needing removal in its entirety for this bug.  any query url would be retained as published.

i don't believe there are any other ways to create an http url in Tb attachments (cloud stuff is an inline link).  do you have some other use case in mind?
It looks to me mime can put in both ?part= or &part=
http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimei.cpp#1263

I wanted to make it robust with current code and possible future changes.
Wouldn't it break if the enclosure had a url with params to start with?

Maybe something lik url.replace(new RegExp("([?])part=x&?|[&]part=x&?"), "$1")
we could fall into a bottomless regex pit trying to futureproof this ;)  

in fact, the current code, for http urls, *only* appends one ?part= and does not break urls with ? or & as mentioned (i tried it).  we're not touching message urls (mailbox://).  for http protocol handlers, none of our internal url mucking should remain once passed to the handler.

even if a publisher had ?part=x in the url and x matched the part we assigned it, we'd only remove one. so perhaps:

new RegExp("[?&]part=" + this.partID + "$")
I guess that could be reasonable. Or maybe even better, fix mime to never add the part to the url for https+ urls in the first place
(In reply to alta88 from comment #7)
> the ?part= is added automatically in attachment mime processing.  prior to
> feeds being implemented, attachment urls were only message urls with the
> partID # being used to identify them in a message stored in a berkely mbox
> file.  feed enclosures use the same mime construct, but a remote link is
> different from an mbox attachment url.
> 
> btw, if you've never built Tb it might be a bit of a curve just for this. 
> it's easier (if you're familiar with the tools) to unzip the omni.ja file,
> find msgHdrViewOverlay.js in the tree, make the changes per the patch, and
> re-zip (next update will wipe them though).

thanks for the info!

unzipping sounds easy, but i'm not sure if these extra bytes bite:
Archive:  omni.ja
warning [omni.ja]:  11070463 extra bytes at beginning or within zipfile
  (attempting to process anyway)
error [omni.ja]:  reported length of central directory is
  -11070463 bytes too long (Atari STZip zipfile?  J.H.Holm ZIPSPLIT 1.1
  zipfile?).  Compensating...
    testing: chrome.manifest          OK
    testing: chrome/chrome.manifest   OK
    testing: components/components.manifest   OK
..........

But i will find out soon enough ;)
(In reply to Magnus Melin from comment #9)
> Comment on attachment 8351708 [details] [diff] [review]
> feedattachmentPart.patch
> 

I applied changes to msgHdrViewOverlay.js in omni.ja, and it works (for this feed)!

I did this by simply unzipping omni.ja, changing file, and zipping everything back

Thanks
as mentioned, there may be value in having the partID in the js attachment object, and since the mime emitter mechanism is to add part= to the url, it really has do be done higher up.

so lets go with new RegExp("[?&]part=" + this.partID + "$")
Attachment #8351708 - Attachment is obsolete: true
Attachment #8355221 - Flags: review?(mkmelin+mozilla)
Attachment #8355221 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/dcdaf7115ff8
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
(we should get fixed bugs out of Untriaged)
Component: Untriaged → Message Reader UI
You need to log in before you can comment on or make changes to this bug.