Last Comment Bug 522645 - Determining what is a feed message regression
: Determining what is a feed message regression
Status: RESOLVED FIXED
: regression
Product: Thunderbird
Classification: Client Software
Component: Folder and Message Lists (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: alta88
:
:
Mentors:
: 554101 (view as bug list)
Depends on: 647699
Blocks: gloda-ui-regressions 760157
  Show dependency treegraph
 
Reported: 2009-10-15 21:54 PDT by alta88
Modified: 2012-05-31 12:17 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
restore content-base feed check. (1.08 KB, patch)
2009-10-16 10:33 PDT, alta88
mozilla: review-
Details | Diff | Splinter Review
add contentBase as db hdr property if it exists in message (1021 bytes, patch)
2009-10-16 13:02 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
patch (2.38 KB, patch)
2012-05-29 17:53 PDT, alta88
mozilla: review+
Details | Diff | Splinter Review

Description alta88 2009-10-15 21:54:56 PDT
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.
Comment 1 Siddharth Agarwal [:sid0] (inactive) 2009-10-16 03:21:26 PDT
> there won't be a gFolderDisplay object in a standalone message window

There should be.
Comment 2 Dan Mosedale (:dmose) 2009-10-16 09:34:31 PDT
Can you describe exactly what the user-visible effects of this bug are?
Comment 3 Magnus Melin 2009-10-16 10:26:10 PDT
xref bug 493221 which likely WFM now (by the same cause)
Comment 4 alta88 2009-10-16 10:33:44 PDT
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.
Comment 5 Andrew Sutherland [:asuth] 2009-10-16 10:45:01 PDT
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?
Comment 6 Dan Mosedale (:dmose) 2009-10-16 10:45:52 PDT
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!
Comment 7 Dan Mosedale (:dmose) 2009-10-16 10:47:03 PDT
Mmmm, bugzilla-collision-riffic.  Will aim review at bienvenu.
Comment 8 David :Bienvenu 2009-10-16 10:51:50 PDT
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.
Comment 9 Andrew Sutherland [:asuth] 2009-10-16 11:03:51 PDT
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.
Comment 10 David :Bienvenu 2009-10-16 11:07:17 PDT
Comment on attachment 406713 [details] [diff] [review]
restore content-base feed check.

minusing based on asuth's comments.
Comment 11 Dan Mosedale (:dmose) 2009-10-16 11:26:21 PDT
Should we consider putting the Content-Base into the message header?
Comment 12 David :Bienvenu 2009-10-16 11:34:42 PDT
We could, since only rss messages will have content-base. It wouldn't help with already downloaded rss messages, however, w/o a reparse.
Comment 13 alta88 2009-10-16 11:54:20 PDT
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 David :Bienvenu 2009-10-16 11:59:25 PDT
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.
Comment 15 alta88 2009-10-16 12:20:30 PDT
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 David :Bienvenu 2009-10-16 12:34:43 PDT
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.
Comment 17 Andrew Sutherland [:asuth] 2009-10-16 12:45:20 PDT
(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 David :Bienvenu 2009-10-16 12:48:06 PDT
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.
Comment 19 alta88 2009-10-16 13:02:08 PDT
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 David :Bienvenu 2009-10-16 13:02:34 PDT
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 David :Bienvenu 2009-10-16 13:05:43 PDT
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 David :Bienvenu 2009-10-16 13:12:31 PDT
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.
Comment 23 Dan Mosedale (:dmose) 2009-10-16 14:05:17 PDT
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?
Comment 24 alta88 2009-10-16 14:25:14 PDT
(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..
Comment 25 alta88 2011-04-02 18:28:36 PDT
*** Bug 554101 has been marked as a duplicate of this bug. ***
Comment 26 alta88 2011-04-04 09:49:33 PDT
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).
Comment 27 alta88 2012-05-03 12:24:11 PDT
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 David :Bienvenu 2012-05-29 07:17:55 PDT
(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
Comment 29 alta88 2012-05-29 17:53:16 PDT
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.
Comment 30 David :Bienvenu 2012-05-30 15:27:26 PDT
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.
Comment 31 alta88 2012-05-31 08:49:33 PDT
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.
Comment 32 Mike Conley (:mconley) 2012-05-31 12:16:53 PDT
Landed in comm-central as https://hg.mozilla.org/comm-central/rev/81cb0eec3150

Note You need to log in before you can comment on or make changes to this bug.