Determining what is a feed message regression

RESOLVED FIXED in Thunderbird 15.0

Status

Thunderbird
Folder and Message Lists
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Thunderbird 15.0
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

8 years ago
the patch in Bug 438429 specifically handled the following cases:
1)a feed message can be in any folder, not just News
2)a feed message in a standalone window is recognized as such

Bug 474701 removed the content-base check, which should be restored.  in addition, there won't be a gFolderDisplay object in a standalone message window, i believe, so the selectedMessageIsFeed there isn't going to work.
> there won't be a gFolderDisplay object in a standalone message window

There should be.
Can you describe exactly what the user-visible effects of this bug are?

Comment 3

8 years ago
xref bug 493221 which likely WFM now (by the same cause)
(Assignee)

Comment 4

8 years ago
Created attachment 406713 [details] [diff] [review]
restore content-base feed check.


(In reply to comment #1)
> > there won't be a gFolderDisplay object in a standalone message window
> 
> There should be.

yep, it's there.  standalone message from News folder is recognized.

(In reply to comment #2)
> Can you describe exactly what the user-visible effects of this bug are?

any feed message not in the News folder will not have feed specific menuitems displayed.  this patch should revert to the spirit of the previous check.

(In reply to comment #3)
> xref bug 493221 which likely WFM now (by the same cause)

imo it is far more important to enable feed messages in different folder organizations, than remove the check to address that bug, which is a quite nonstandard mail header issued by the publisher, and in fact not formatted to spec.
Attachment #406713 - Flags: superreview?(dmose)
Attachment #406713 - Flags: review?(dmose)
Comment on attachment 406713 [details] [diff] [review]
restore content-base feed check.

This code should probably be reviewed by myself, sid0, or bienvenu.

You shouldn't use currentHeaderData here.  Is there something on the nsIMsgDBHdr itself you can use?
Attachment #406713 - Flags: superreview?(dmose)
Attachment #406713 - Flags: superreview?
Attachment #406713 - Flags: review?(dmose)
Attachment #406713 - Flags: review-
Attachment #406713 - Flags: superreview?
Comment on attachment 406713 [details] [diff] [review]
restore content-base feed check.

No need for sr, since this is in mail/.  Myk has graciously agreed to review, which is good, because I'm not turning around reviews as quickly as I'd like to be at the moment.  Thanks, Myk!
Attachment #406713 - Flags: review- → review?(myk)
Mmmm, bugzilla-collision-riffic.  Will aim review at bienvenu.
Assignee: nobody → alta77

Updated

8 years ago
Attachment #406713 - Flags: review?(myk) → review?(bienvenu)

Comment 8

8 years ago
Comment on attachment 406713 [details] [diff] [review]
restore content-base feed check.

We don't put the content base in the msg hdr - currentHeaderData could be empty if there's no message pane displayed.
Right, this sounds familiar.  I think I tried to avoid removing the check in the code but there was no easy way to reliably have the data.

The best-case solution for this is to have the MessageDisplayWidget register for header notifications from the message reader.  If it sees that header is set, it can set an internal flag like "bodyContentIndicatesFeed" which the FolderDisplayWidget can check.  It could also trigger a toolbar update or whatever is required if it can't rely on one being produced elsewhere (although I suspect it can).

We definitely want to avoid having FolderDisplayWidget access a global provided by the message reader without active/inactive scoping and without some awareness of the availability of the data being event-driven.

Updated

8 years ago
Attachment #406713 - Flags: review?(bienvenu) → review-

Comment 10

8 years ago
Comment on attachment 406713 [details] [diff] [review]
restore content-base feed check.

minusing based on asuth's comments.
Should we consider putting the Content-Base into the message header?

Comment 12

8 years ago
We could, since only rss messages will have content-base. It wouldn't help with already downloaded rss messages, however, w/o a reparse.
(Assignee)

Comment 13

8 years ago
hmm, the new internals look quite different from what i recall.  since the purpose here is to only do the check/show menuitems if 1 message is selected (not meaningful for possible mixed types in multiselect), it seems reasonable to require that message pane or message in standalone window is visible.  in which case header data should be very easily accessible and currentHeaderData should work.  otherwise the menuitems can be disabled.

any reason not to just go back to the prior method?

Comment 14

8 years ago
See Andrew's comment #9 

>We definitely want to avoid having FolderDisplayWidget access a global provided
>by the message reader without active/inactive scoping and without some
>awareness of the availability of the data being event-driven.
(Assignee)

Comment 15

8 years ago
right, i took that to mean using gFolderDisplay methods in folderDisplay.js, not the already available header data in mailWindowOverlay.js.  perhaps incorrectly..

Comment 16

8 years ago
ah, by prior method, you mean going back to code in mailWindowOverlay.js...I don't know this code that well, asuth should probably chime in.
(In reply to comment #15)
> right, i took that to mean using gFolderDisplay methods in folderDisplay.js,
> not the already available header data in mailWindowOverlay.js.  perhaps
> incorrectly..

In an ideal world we could identify messages that are RSS messages without streaming them.  It would be most preferable to move towards that world, which would mean setting the message header and just using that.  (Given that RSS is a 'timely' medium, even if users do not reindex, most would probably not even have a problem since all the new messages would still get the header.)

Having said that, mailWindowOverlay.js is still an untamed wilderness from my perspective; I'm fine with doing the currentHeaderData check there.  The caveat is that when we actually improve the message reader situation, it may get broken again unless RSS becomes a priority and starts getting a lot of love and unit tests.  The 'more correct' solution is less likely to get steamrolled.

Comment 18

8 years ago
yeah, I'm happy to make the local message parser set a content-base property and let alta88 query that property. I'll put up a patch in a minute for him to play with.
(Assignee)

Comment 19

8 years ago
david, i don't know that you need to do anything, it would just go back to what worked before.  content-base was just available, filled in when a message was selected..

the thing that would be most correct to do, and take no time, would be to not depend on content-base, since per strict rfc it is freeform and an email client *could* publish it, though it's very much not customary.  it would be simple to add something like x-moz-feed and fill it with something useful like rss/atom version etc.

then we could check for x-moz-feed instead of content-base.

Comment 20

8 years ago
Created attachment 406755 [details] [diff] [review]
add contentBase as db hdr property if it exists in message

alta88, if you run with this patch, newly downloaded rss messages should have a string property set to the content-base header value, so nsIMsgDBHdr.getStringProperty("content-base") would tell if you if the message has a content base.

Comment 21

8 years ago
ah, didn't see your comment til after. The same patch could be tweaked to add x-moz-feed instead...but you'd need to set that header in the feed code, as you know, I'm sure.

Comment 22

8 years ago
What would be even nicer is to make nsILocalMailFolder::addMessage return the msg hdr of the header added, and you could set the property yourself. I think I'm going to do that. That would also make some of our test cases easier to write.
Assuming we use Content-Base this way, what are the security implications (if any) of a black-hat/phisher sending an email with a Content-Base header pointing to site of their choosing.  Is there any chance we'd load that site rather than displaying the message itself?
(Assignee)

Comment 24

8 years ago
(In reply to comment #23)
> Assuming we use Content-Base this way, what are the security implications (if
> any) of a black-hat/phisher sending an email with a Content-Base header
> pointing to site of their choosing.  Is there any chance we'd load that site
> rather than displaying the message itself?

yes, if the default were to show web page.  the issue exists in Tb2 as well.  and it would be true if this functionality depended on any header; x-moz-feed could be spoofed in an email too.

feedparser or whatever stores a msg to the db would have to set a feed flag, which could be trusted, or similar arch..
(Assignee)

Updated

7 years ago
Duplicate of this bug: 554101
(Assignee)

Comment 26

7 years ago
filed bug 647699, as per c#22 it's the best way to go.  we really need a trusted way of identifying feed messages (not based on folder).
Depends on: 647699
(Assignee)

Comment 27

5 years ago
now that 647699 is fixed thanks to magnus, i'd like to complete this, such that a message that is a feed is treated as such regardless of folder server (including re content policy).

it seems there is a free slot at x40 for an isFeed flag here:
http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsMsgMessageFlags.idl#43

david, is it ok to take this bit?  initially i thought there could be a separate feed uint property with other flags but couldn't think of any other flags.

Comment 28

5 years ago
(In reply to alta88 from comment #27)

> david, is it ok to take this bit?  initially i thought there could be a
> separate feed uint property with other flags but couldn't think of any other
> flags.

yes, I'm OK with that; it seems like a relatively important attribute worthy of one of our precious remaining bits :-) Or you could also simply use a property on the nsIMsgDBHdr, that defaults to false and is only set for feeds. It's marginally slower, and adds a bit of bloat to the .msf files for feeds
(Assignee)

Comment 29

5 years ago
Created attachment 628166 [details] [diff] [review]
patch

yeah, if there were any reason to store anything more informative, a property would be the way to go, but there isn't.  so this is probably a 'bit' better.

part 1 to merely store the flag.
Assignee: alta77 → alta88
Attachment #628166 - Flags: review?(dbienvenu)

Comment 30

5 years ago
Comment on attachment 628166 [details] [diff] [review]
patch

looks OK. I forget what 0x40 used to be used for. It looks like it's been unused for a very long time, but there's a small chance it might exist out there in some local mail folders in x-mozilla-status headers.
Attachment #628166 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 31

5 years ago
if that's indeed the case, subsequent code can also check to see if there's a content-base header.

making use of the new flag will be a separate bug.
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Blocks: 760157
Landed in comm-central as https://hg.mozilla.org/comm-central/rev/81cb0eec3150
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0

Updated

5 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.