Last Comment Bug 662907 - web site from RSS feed not rendered correctly (due to noscript tags)
: web site from RSS feed not rendered correctly (due to noscript tags)
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Feed Reader (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: Thunderbird 33.0
Assigned To: Magnus Melin
:
:
Mentors:
: 511416 518522 565022 614795 639400 665126 678591 700653 (view as bug list)
Depends on:
Blocks: 1151497
  Show dependency treegraph
 
Reported: 2011-06-08 13:08 PDT by Christian Riechers
Modified: 2015-04-06 09:39 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
snapshot (bad) in 3-pane feed reader (285.49 KB, image/png)
2012-01-11 17:28 PST, Tony Mechelynck [:tonymec]
no flags Details
snapshot (good) in browser (343.70 KB, image/png)
2012-01-11 17:30 PST, Tony Mechelynck [:tonymec]
no flags Details
feedNoscript.patch (3.21 KB, patch)
2013-11-04 18:00 PST, alta88
standard8: review-
Details | Diff | Splinter Review
feedNoscript.patch (2.87 KB, patch)
2013-11-05 06:53 PST, alta88
no flags Details | Diff | Splinter Review
bug662907_rss_noscript.patch (11.13 KB, patch)
2014-06-18 12:38 PDT, Magnus Melin
no flags Details | Diff | Splinter Review
bug662907_rss_noscript.patch (11.68 KB, patch)
2014-06-19 12:30 PDT, Magnus Melin
standard8: review+
standard8: approval‑comm‑aurora-
standard8: approval‑comm‑beta-
Details | Diff | Splinter Review
rss.jpg (229.56 KB, image/jpeg)
2015-04-05 13:56 PDT, Joe Sabash [:JoeS1]
no flags Details

Description Christian Riechers 2011-06-08 13:08:29 PDT
User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20110607 Thunderbird/5.0b2pre

I've subscribed to the RSS feed below (a German IT news site):
http://www.heise.de/newsticker/heise-atom.xml
When opening any post from this feed in either a new tab or a new window, the
page isn't rendered correctly in TB. Both, the top frame and the right frame contain html code.
Here is a screenshot:
http://imageshack.us/photo/my-images/585/screenshotjavase7.png/

Pages from this feed are rendered just fine in both, TB 3.1.10, and FF 4.0.1.
Also the page is rendered fine in TB5 when opening it in the message area.

Reproducible: Always

Steps to Reproduce:
1. Subscribe to feed http://www.heise.de/newsticker/heise-atom.xml
2. Open any post from that feed in either a new message window or a new tab.

Actual Results:  
Page from RSS feed isn't rendered properly when opened in TB. The top and right frame contain html code. 
See screenshot:
http://imageshack.us/photo/my-images/585/screenshotjavase7.png/

Expected Results:  
Page should be rendered properly without showing HTML code.
Comment 1 alta88 2011-06-09 08:50:22 PDT
it seems the <div> child of NOSCRIPT tags is being parsed as text rather than a node.
Comment 2 Joe Sabash [:JoeS1] 2011-06-09 14:10:18 PDT
Disabling the html5 parser seems to fix it for me, which would point to a core issue.
ConfigEdit   html5.parser.enable  false

BTW This might be a local (different) problem, but I am unable to display any feeds in the messagepane here. (I normally open everything in a standalone window)

Testing with:
Mozilla/5.0 (Windows NT 5.0; rv:7.0a1) Gecko/20110605 Thunderbird/7.0a1 ID:20110605030344
Comment 3 Christian Riechers 2011-06-09 14:25:30 PDT
There is another example where a site from a RSS feed isn't rendered correctly. The feed url is:
http://news.bbc.co.uk/rss/newsonline_world_edition/front_page/rss091.xml
There is a daily post 'Day in pictures' in this feed. As an example, the url of today's post is:
http://www.bbc.co.uk/go/rss/int/news/-/news/world-13708703
As in the other example, in the top banner, as well as above the actual picture HTML code is displayed. Here is a screenshot:
http://imageshack.us/photo/my-images/819/screenshotdayinpictures.png/
Comment 4 Wilf 2011-07-03 04:18:53 PDT
I'm getting the same problem.  With BBC feeds I see html code either before or within the article heading banner.   With Sky New feeds I see code in a block somewhere in the mid righthand side of the article.
Comment 5 Mark Banner (:standard8) 2011-07-04 01:30:42 PDT
Interestingly the pages seem fine if viewed in a content tab. So it could be an issue with how these pages are being loaded into the message pane, or some settings on the message pane itself.
Comment 6 Alexander Kriegisch 2011-07-10 06:18:26 PDT
I can confirm the bug on Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20110624 Thunderbird/5.0 ID:20110624125636 with Heise news feed. I also confirm the workaround (html5.parser.enable -> false) to be helpful. I am interested in seeing the root cause resolved, though.
Comment 7 alta88 2011-07-16 13:36:02 PDT
i was wondering why this problem wasn't happening in TotalMessage, and it turns out that i handle js on a docShell level etc.

the upshot is that setting
  html5.parser.enable = true, and 
  javascript.enabled = false
will also prevent incorrect parsing of noscript tags.  perhaps someone can confirm..  the js pref seems to be core and perhaps gecko 2.0.

this doesn't explain why a content tab should not be affected, i believe both do loadURI the same way, on a <browser> that is content-primary.  it may be that messagepane docShell.appType is APP_TYPE_MAIL..?
Comment 8 Pepe 2011-07-19 23:37:38 PDT
  html5.parser.enable = true, and 
  javascript.enabled = false

works for me (MacOSX), thank you!
Comment 9 Ludovic Hirlimann [:Usul] 2011-07-20 03:21:55 PDT
Alta88 is this a dup ?
Comment 10 alta88 2011-07-21 15:13:28 PDT
(In reply to comment #9)
> Alta88 is this a dup ?

of what?
Comment 11 Jens Hatlak (:InvisibleSmiley) 2011-10-12 10:47:04 PDT
Does anyone have an idea how we can get some traction here, e.g. by CC'ing some HTML5 parser people? Disabling HTML5 may be a viable option for Thunderbird users, but not SeaMonkey users (who are affected, too) since the pref disables the new parser for the whole suite, i.e. including the browser.

I've been able to reproduce this all the time for months, using major news sites such as Heise, Golem, freshmeat. Since Scriptish/Greasemonkey doesn't work for feeds displayed in MailNews, hacking a workaround by removing NOSCRIPT areas unfortunately doesn't work either. Or is there some other means to attach some JS fixup code to the feed webpage loading process?
Comment 12 Sascha Grage 2011-10-31 13:36:07 PDT
****, the html5.parser.enable Pref is Gone...
http://hsivonen.iki.fi/old-parser-pref/

Feedreader almost not usable...
Comment 13 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-11-01 00:14:23 PDT
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #11)
> Does anyone have an idea how we can get some traction here, e.g. by CC'ing
> some HTML5 parser people?

Yeah, that would have been a good idea earlier. In general, it's a really bad idea to to flip the pref and not let me know why you needed to flip it.

> Disabling HTML5 may be a viable option for Thunderbird users

No, it really isn't, since the old parser code *will* go away!

(In reply to alta88 from comment #1)
> it seems the <div> child of NOSCRIPT tags is being parsed as text rather
> than a node.

When scripting is enabled, the source between <noscript> and </noscript> becomes a text node. This is  correct per spec. Reasonable rendering then relies on the user agent style sheet supplying a
noscript { display: none; }
rule.

Does the feed view run scripts? If not, the correct fix is to run the parser in the "scripting disabled" mode. If it does, the parser should be run in the scripting enabled mode and the user agent style sheet should supply the CSS rule mentioned above.
Comment 14 Alexander Kriegisch 2011-11-01 09:03:10 PDT
Henri, how could I (as an end-user) change my Thunderbird settings to get noscript { display: none; } into action locally as a workaround until the problem is fixed upstream? I would like to get rid of the old html5.parser.enable dependency which I used as a workaround so far.
Comment 15 drghughes 2011-11-01 09:31:08 PDT
(In reply to Henri Sivonen (:hsivonen) from comment #13)

> When scripting is enabled, the source between <noscript> and </noscript>
> becomes a text node. This is  correct per spec. Reasonable rendering then
> relies on the user agent style sheet supplying a
> noscript { display: none; }
> rule.

If this is the case, why do the pages display correctly in Firefox but incorrectly in Thunderbird?  Doesn't that indicate that there is a difference between the two?
Comment 16 Joe Sabash [:JoeS1] 2011-11-01 17:33:12 PDT
WAT extension does a nice job of rendering problematic feeds
https://addons.mozilla.org/en-US/thunderbird/addon/wat-webapplicationtab/
And plays nicely with:
https://addons.mozilla.org/en-US/thunderbird/addon/totalmessage/versions/
(version<tb9)
Comment 17 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-11-03 03:37:45 PDT
(In reply to drghughes from comment #15)
> If this is the case, why do the pages display correctly in Firefox but
> incorrectly in Thunderbird?  Doesn't that indicate that there is a
> difference between the two?

Presumably, the difference is the bug here that needs fixing.

Since the "scripting disabled" state can change, the noscript { display: none; } rule isn't hard-coded into the UA stylesheet .css file. Instead, the rule is synthetized into the UA style sheet data structure.

Chances are that there's something about the way Thunderbird uses the presshell that make the rule injection not happen.

(In reply to Alexander Kriegisch from comment #14)
> Henri, how could I (as an end-user) change my Thunderbird settings to get
> noscript { display: none; } into action locally as a workaround until the
> problem is fixed upstream?

You could try adding noscript { display: none; } to your user CSS. I don't know where the user CSS file live in Thunderbird.
Comment 18 Alexander Kriegisch 2011-11-03 04:36:39 PDT
Henri, I can acknowledge that resetting 'html5.parser.enable' to 'true' plus adding  'noscript { display: none; }' to the user CSS file  (<profile_folder>/chrome/userContent.css) and then restarting Thunderbird is a functioning workaround for the moment. So this should be safe enough to work after 'html5.parser.enable' will have vanished from TB and until the actual bug causing the misbehaviour will have been fixed. Thanks for now. :)
Comment 19 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-11-03 06:03:01 PDT
The code that is used for deciding how to parse <noscript> differs from the code that decides whether to generate noscript { display: none; }.

The parser code originally came from
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/html/document/src&command=DIFF_FRAMESET&file=nsHTMLContentSink.cpp&rev2=3.460&rev1=3.459
Comment 20 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-11-03 06:14:38 PDT
(In reply to Mark Banner (:standard8) from comment #5)
> Interestingly the pages seem fine if viewed in a content tab.

Are these pages supposed to have scripting enabled or scripting disabled?
Comment 21 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-11-03 06:18:42 PDT
The UA style sheet creation code checks the scripting state when the shell is created. The HTML parser checks the scripting state when OnStartRequest fires.
Comment 22 Mark Banner (:standard8) 2011-11-03 06:55:26 PDT
(In reply to Henri Sivonen (:hsivonen) from comment #21)
> The UA style sheet creation code checks the scripting state when the shell
> is created. The HTML parser checks the scripting state when OnStartRequest
> fires.

Hmm, that could be an issue. We currently set the scripting state in the content policy in ShouldLoad:

http://hg.mozilla.org/comm-central/annotate/d7ed2d9fa348/mailnews/base/src/nsMsgContentPolicy.cpp#l243

I'm not quite sure where that fits in with the requests you've got there.
Comment 23 Jens Hatlak (:InvisibleSmiley) 2011-11-03 07:21:02 PDT
(In reply to Alexander Kriegisch from comment #18)
> Henri, I can acknowledge that resetting 'html5.parser.enable' to 'true' plus
> adding  'noscript { display: none; }' to the user CSS file 
> (<profile_folder>/chrome/userContent.css) and then restarting Thunderbird is
> a functioning workaround for the moment.

Good to know, thanks for checking! I'll add a Known Issue entry to the release notes of all affected SeaMonkey versions until this bug gets fixed. TB responsibles should probably do the same (or update SUMO or whatever they're using nowadays).

(In reply to Henri Sivonen (:hsivonen) from comment #20)
> Are these pages supposed to have scripting enabled or scripting disabled?

My voice is not authoritative here but I'd say that feeds should be displayed/rendered exactly like in a normal browser tab. Unlike messages (mail/news), they have scripting and plugins support enabled (in fact, both cannot be enabled for mail/news messages anymore so feeds are purposely special-cased).
Comment 24 Christian Riechers 2011-11-03 14:23:02 PDT
(In reply to Alexander Kriegisch from comment #18)
> Henri, I can acknowledge that resetting 'html5.parser.enable' to 'true' plus
> adding  'noscript { display: none; }' to the user CSS file 
> (<profile_folder>/chrome/userContent.css) and then restarting Thunderbird is
> a functioning workaround for the moment. 

This workaround doesn't work for me, neither in TB 7.0.1 nor 8beta running on Linux.
The original problem is still seen.
Comment 25 Alexander Kriegisch 2011-11-03 14:32:03 PDT
I am surprised. Did you restart Thunderbird? I had to. The CSS will not be reloaded during the session. And did you really edit the correct file?
I tested with Heise News, by the way. TB 7.0.1 on Win XP.
Comment 26 drghughes 2011-11-03 18:35:20 PDT
The 'noscript { display: none; }' fix worked for me in Thunderbird 7.0.1 on Windows 7 Home Premium SP1 64 bit.

I've tested this on a number of feeds that had problems without the fix.
Comment 27 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-11-03 23:15:45 PDT
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #23)
> My voice is not authoritative here but I'd say that feeds should be
> displayed/rendered exactly like in a normal browser tab. Unlike messages
> (mail/news), they have scripting and plugins support enabled

In that case, the HTML parser is doing the right thing. It's now up to the Thunderbird developers to make sure that the security manager, the script global object, etc., are in the appropriate state before presshell creation.

Specifically, the state needs to be such that the line
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#1549
runs on presshell creation.
Comment 28 Tobias Weibel 2011-11-10 23:12:03 PST
*** Bug 678591 has been marked as a duplicate of this bug. ***
Comment 29 Mark Banner (:standard8) 2011-11-18 01:26:14 PST
*** Bug 511416 has been marked as a duplicate of this bug. ***
Comment 30 Mark Banner (:standard8) 2011-11-18 01:26:48 PST
*** Bug 614795 has been marked as a duplicate of this bug. ***
Comment 31 alta88 2011-11-26 19:29:57 PST
*** Bug 665126 has been marked as a duplicate of this bug. ***
Comment 32 Magnus Melin 2011-12-29 22:33:19 PST
*** Bug 700653 has been marked as a duplicate of this bug. ***
Comment 33 Tony Mechelynck [:tonymec] 2012-01-11 17:28:47 PST
Created attachment 587890 [details]
snapshot (bad) in 3-pane feed reader

Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20120110 Firefox/12.0a1 SeaMonkey/2.9a1 ID:20120110233317

I get this bug on all newsfeeds from the New York Times. Here is the "bad" snapshot (in 3-pane feed reader). I'll attach the same page next, as seen in the browser (good), for comparison.
Comment 34 Tony Mechelynck [:tonymec] 2012-01-11 17:30:02 PST
Created attachment 587891 [details]
snapshot (good) in browser
Comment 35 Tony Mechelynck [:tonymec] 2012-01-12 02:21:20 PST
Bug has been seen in TB5 and TB7 and I'm still seeing it in Sm trunk (~ Tb12) -- setting "affected" to everything still tracked on the assumption that it has not gone away and come back. Of course, it isn't for me to decide whether to port the fix (after it lands) to which branch.
Comment 36 Mark Banner (:standard8) 2012-01-12 02:37:33 PST
(In reply to Tony Mechelynck [:tonymec] from comment #35)
> Bug has been seen in TB5 and TB7 and I'm still seeing it in Sm trunk (~
> Tb12) -- setting "affected" to everything still tracked on the assumption
> that it has not gone away and come back. Of course, it isn't for me to
> decide whether to port the fix (after it lands) to which branch.

You don't need to set affected for everything. If the bug is still open, then it is still open. The only time we tend to use affected is when we've also got a release in the range we're currently tracking that is unaffected and we're specifically looking at issues on branches - but even then these are more just helpful indicators than anything that'll help the bug get fixed.
Comment 37 kloor68373 2012-02-01 03:52:21 PST
This behaviour reappeared after today's update to TB 10.0 (Win). In addition the workaround described above (html5.parser.enable:false) does not work anymore.
Comment 38 Alexander Kriegisch 2012-02-01 04:03:33 PST
If you read the thread you will find the explanation for why the old workaround is no longer functional. Reading comments 17+18 also points you to a workaround which still works.
Comment 39 Jens Hatlak (:InvisibleSmiley) 2012-02-01 04:07:00 PST
The working workaround is to add this rule to your chrome/userContent.css (or use Stylish and add a custom style):

noscript { display: none; }
Comment 40 Christian Riechers 2012-02-01 12:40:57 PST
(In reply to Alexander Kriegisch from comment #25)
> I am surprised. Did you restart Thunderbird? I had to. The CSS will not be
> reloaded during the session. And did you really edit the correct file?

I picked the wrong file. Have been using userChrome.css instead of userContent.css. My bad.
Now the workaround works for me too.
Comment 41 alta88 2012-02-14 13:14:23 PST
*** Bug 565022 has been marked as a duplicate of this bug. ***
Comment 42 Magnus Melin 2012-03-18 05:49:07 PDT
I debugged this a bit, it's definitely nsMsgContentPolicy enabling of js being applied to late.

This is a test for one of the feeds i read - http://www.digitoday.fi/feeds/Digitoday.xml which displays some garbage code <a href="http://adserver.adtech.de/adlink/969/2178185/0/225/AdId=7 ... on top of pages due to this.

Subscribe to the feed, then thunderbird | view | feed message body as | web page

LoadURI will get called - http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#3292

nsMsgContentPolicy::SetDisableItemsOnMailNewsUrlDocshells => msgURL: mailbox:///home/magnus/.thunderbird/oa2zpb8z.testing/Mail/Feeds/digitoday?number=2696
PresShell::Init: mailbox:///home/magnus/.thunderbird/oa2zpb9z.testing/Mail/Feeds/digitoday?number=2696
PresShell::SetPrefNoScriptRule
...scriptEnabled=0 - rule to hide noscript content NOT added
nsMsgContentPolicy::OnLocationChange: mailbox:///home/magnus/.thunderbird/oa2zpb8z.testing/Mail/Feeds/digitoday?number=2696
nsMsgContentPolicy::OnLocationChange - location is nsIMsgMessageUrl; setting allowjs=false

nsMsgContentPolicy::SetDisableItemsOnMailNewsUrlDocshells => !msgURL: http://www.digitoday.fi/p/201225431
PresShell::Init: http://www.digitoday.fi/tiede-ja-teknologia/2012/03/16/robohanska-vahvistaa-ihmisen-katta/201225431/66
PresShell::SetPrefNoScriptRule
...scriptEnabled=0 - rule to hide noscript content NOT added
nsMsgContentPolicy::OnLocationChange: http://www.digitoday.fi/tiede-ja-teknologia/2012/03/16/robohanska-vahvistaa-ihmisen-katta/201225431/66
nsMsgContentPolicy::OnLocationChange - location is NOT nsIMsgMessageUrl; setting allowjs=true

One fix would be to get rid of this: http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgContentPolicy.cpp#247 which from the comment there is just "extra" protection.
Comment 43 Magnus Melin 2012-03-18 12:44:05 PDT
What i don't understand is, for the http url, nsMsgContentPolicy::SetDisableItemsOnMailNewsUrlDocshells will never get to the point that it's disabling javascript. But javascript is still disabled when its presshell is inited - can there be some kind of inheritance, or why would that be? In docshell javascript is enabled by default.
Comment 44 alta88 2012-04-18 21:47:47 PDT
*** Bug 639400 has been marked as a duplicate of this bug. ***
Comment 45 alta88 2012-04-20 08:00:09 PDT
*** Bug 518522 has been marked as a duplicate of this bug. ***
Comment 46 Tony Mechelynck [:tonymec] 2012-07-12 06:56:58 PDT
(In reply to Magnus Melin from comment #43)
> What i don't understand is, for the http url,
> nsMsgContentPolicy::SetDisableItemsOnMailNewsUrlDocshells will never get to
> the point that it's disabling javascript. But javascript is still disabled
> when its presshell is inited - can there be some kind of inheritance, or why
> would that be? In docshell javascript is enabled by default.

Javascript is enabled by default in the browser; but isn't Javascript disabled in the mailer (including Thunderbird's and SeaMonkey's 3-pane windows)?
Comment 47 Jens Hatlak (:InvisibleSmiley) 2012-07-12 07:11:35 PDT
(In reply to Tony Mechelynck [:tonymec] from comment #46)
> Javascript is enabled by default in the browser; but isn't Javascript
> disabled in the mailer (including Thunderbird's and SeaMonkey's 3-pane
> windows)?

Not for RSS (feeds).
Comment 48 alta88 2013-11-04 18:00:20 PST
Created attachment 827135 [details] [diff] [review]
feedNoscript.patch

The (a) docshell is always js disabled first, since for feeds a mailbox:// uri is loaded, and then loadURI is done after the web page pref is checked.  In fact, mailnews uris are js disabled twice, once in SetDisableItemsOnMailNewsUrlDocshells and again in onLocationChange.

So the (some) docshell is js enabled in onLocationChange for http content, but the problem is it's not the same docshell as was js disabled in SetDisableItemsOnMailNewsUrlDocshells the first time, since it was derived with some frameloader business (and the reason isn't clear).  Both places need to set on the same docshell (unknown why rootDocshell wasn't used).

It's possible to just get rid of SetDisableItemsOnMailNewsUrlDocshells; js will be disabled properly for mailnens urls in onLocationChanged.

Anyway, in the interest of not bothering the fragile assumptions here, this patch fixes setting js on urls of interest correctly in SetDisableItemsOnMailNewsUrlDocshells.
Comment 49 Mark Banner (:standard8) 2013-11-05 00:05:21 PST
(In reply to alta88 from comment #48)
> It's possible to just get rid of SetDisableItemsOnMailNewsUrlDocshells; js
> will be disabled properly for mailnens urls in onLocationChanged.

See http://hg.mozilla.org/comm-central/annotate/6e39f5e0dfd6/mailnews/base/src/nsMsgContentPolicy.cpp#l218 for the reason that we call both.
Comment 50 Mark Banner (:standard8) 2013-11-05 00:05:48 PST
(In reply to Mark Banner (:standard8) from comment #49)
> (In reply to alta88 from comment #48)
> > It's possible to just get rid of SetDisableItemsOnMailNewsUrlDocshells; js
> > will be disabled properly for mailnens urls in onLocationChanged.
> 
> See
> http://hg.mozilla.org/comm-central/annotate/6e39f5e0dfd6/mailnews/base/src/
> nsMsgContentPolicy.cpp#l218 for the reason that we call both.

err, for the reason that we control javascript/plugins from both.
Comment 51 Mark Banner (:standard8) 2013-11-05 00:14:17 PST
Comment on attachment 827135 [details] [diff] [review]
feedNoscript.patch

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

Thanks for looking into this.

This needs some form of unit test to land. Although we've not got an explicit comment about it, we've generally been requiring unit tests for any content policy changes due to the complexity and that it has a significant affect on user security.

There are some existing MozMill tests in /content-policy/. Whilst I know that RSS cannot easily be brought into the MozMill tests at the moment, I would accept simulating this via http loading with noscript tags, e.g. through showing the start page and then also loaded in a tab.

::: mailnews/base/src/nsMsgContentPolicy.cpp
@@ +701,5 @@
>    if (itemType != nsIDocShellTreeItem::typeContent) {
>      return NS_OK;
>    }
>  
> +  if (!isAllowedContent) {

This section is clearly copied and pasted from onLocationChange, and should be made into a shared function.

@@ +704,5 @@
>  
> +  if (!isAllowedContent) {
> +    // Disable javascript on message URLs.
> +    rv = docShell->SetAllowJavascript(false);
> +    NS_ASSERTION(NS_SUCCEEDED(rv),

NS_ASSERTION is not the same as NS_ENSURE_SUCCESS and will mean that if the setting does fail, then the calling code won't reject as it should do.
Comment 52 alta88 2013-11-05 06:49:05 PST
(In reply to Mark Banner (:standard8) from comment #50)
> (In reply to Mark Banner (:standard8) from comment #49)
> > (In reply to alta88 from comment #48)
> > > It's possible to just get rid of SetDisableItemsOnMailNewsUrlDocshells; js
> > > will be disabled properly for mailnens urls in onLocationChanged.
> > 
> > See
> > http://hg.mozilla.org/comm-central/annotate/6e39f5e0dfd6/mailnews/base/src/
> > nsMsgContentPolicy.cpp#l218 for the reason that we call both.
> 
> err, for the reason that we control javascript/plugins from both.

well, it's safe to assume that has been duly read.  the massive equivocation there and here
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgContentPolicy.cpp#657 indicate no one really knows, hence "isn't clear" and "fragile assumptions" ;)

so why isn't it just set on root docshell, from which others inherit?
Comment 53 alta88 2013-11-05 06:53:10 PST
Created attachment 827390 [details] [diff] [review]
feedNoscript.patch


this addresses the NS_ENSURE_SUCCESS requirement for the calling situation.  since onLocationChange doesn't have a caller or this requirement, and the code is so small, it's a stretch to create a separate function, imo.

in any event, the root cause has been identified and solved.  if someone can pick up the tests, that would be great.
Comment 54 Alexander Kriegisch 2014-05-06 10:15:09 PDT
This bug is three years old, and now the assignee was removed. This really looks promising. Thanks. :-/
Comment 55 Magnus Melin 2014-06-18 12:38:15 PDT
Created attachment 8442297 [details] [diff] [review]
bug662907_rss_noscript.patch

alta88's patch, with added mozmill test.
Comment 56 Magnus Melin 2014-06-19 12:30:25 PDT
Created attachment 8442994 [details] [diff] [review]
bug662907_rss_noscript.patch

Updated test, I realized the earlier didn't test the right thing.
Comment 57 Mark Banner (:standard8) 2014-07-01 02:51:14 PDT
Comment on attachment 8442994 [details] [diff] [review]
bug662907_rss_noscript.patch

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

Ok, this looks reasonable. One issue is that it fails on my machine (error below), so if it is passing for you, please run through try server before landing just in case there's something wrong.

SUMMARY-UNEXPECTED-FAIL | test-js-content-policy.js | test-js-content-policy.js::test_jsContentPolicy
  EXCEPTION: The content pane is not displaying the right message! Should be: mailbox:///Users/mark/tb/central/objdir-tb/mozilla/_tests/mozmill/mozmillprofile/Mail/Local%20Folders/jsContentPolicy?number=1820 but it's: http://localhost:43336/content/remote-noscript.html
    at: test-folder-display-helpers.js line 2010
       _internal_assert_displayed test-folder-display-helpers.js:2010 13
       assert_selected_and_displayed test-folder-display-helpers.js:2065 3
       checkJsInFeedContent test-js-content-policy.js:149 3
       test_jsContentPolicy test-js-content-policy.js:209 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestFile frame.js:534 3
       runTestFile frame.js:713 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
Comment 58 Magnus Melin 2014-07-13 02:53:31 PDT
Made some slight adjustment to the test and try looks good: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=58e1cf28172a

https://hg.mozilla.org/comm-central/rev/4c17825e799e -> FIXED

Thx alta88, this bug has been really annoying!
Comment 59 Magnus Melin 2014-07-14 11:42:51 PDT
Comment on attachment 8442994 [details] [diff] [review]
bug662907_rss_noscript.patch

[Approval Request Comment]
User impact if declined: feed content show noscript content so stuff often look messed up
Testing completed (on c-c, etc.): landed on trunk
Risk to taking this patch (and alternatives if risky): low
Comment 60 Mark Banner (:standard8) 2014-07-14 14:12:30 PDT
Comment on attachment 8442994 [details] [diff] [review]
bug662907_rss_noscript.patch

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

Whilst I agree with the desire to have this earlier, changes to the content policy get me more concerned than other parts of the app. Given that we're only a few days from release, I think it is too much to push a change in like this, even though it appears relatively simple. The tests are a bonus, but this still needs real life user testing for a bit.
Comment 61 alta88 2014-07-19 07:51:51 PDT
thank *you* magnus, for the tests.  there is a specific reason i didn't do them.
Comment 62 Tobias Weibel 2014-09-25 03:18:54 PDT
I can verify it with:
* User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Thunderbird/33.0
* Application Build ID: 20140923230147
* Source hash: https://hg.mozilla.org/releases/mozilla-beta/rev/f7cb0ec31a9c
* URL: http://www.heise.de/newsticker/heise-atom.xml
Comment 63 freddym3 2015-03-30 14:27:49 PDT
Hi,

this bug really is annoying.
As you can see in the 1st picture, there is an img-tag-code visible thats inside a noscript-tag at the top of the page.
And in picture 2 that img-tag-code inside a noscript-tag is visible below that picture.

http://abload.de/img/noscripttag-1rep20.jpg
http://abload.de/img/noscripttag-26cpti.jpg

This is with the latest Thunderbird version 31.5 and still unresolved.

Will this be possible to fix in Thunderbird 31.6 for example?
Comment 64 Magnus Melin 2015-03-31 01:04:09 PDT
No, but it will be in thunderbird 38, which is not far off (scheduled for 2015-05-12)
Comment 65 Joe Sabash [:JoeS1] 2015-04-04 13:58:48 PDT
Well, as reported by nomis101 in testing TB beta 38, this bug seems to still be with us.
I see it also in current TB trunk.
Just a guess, but this might be a factor of your system speed and/or HWA enabled.
I'll test some more later, but for now, we should just re-open this bug.
Comment 66 Joe Sabash [:JoeS1] 2015-04-04 14:03:05 PDT
Oh, one more thing, if I open the feed message in a tab, I see the the crap. If I later come back to this tab, from another, the garbage is gone. So it seems like some sort of race condition.
Comment 67 Nomis101 2015-04-04 15:00:29 PDT
(In reply to Joe Sabash [:JoeS1] from comment #65)
> Well, as reported by nomis101 in testing TB beta 38, this bug seems to still
> be with us.

This is correct, I still see this issue. I see it on TB 38b1 and on TB trunk. Tested on OS X Yosemite. I see this with the feed from #0, but also with a few others. The workaround from this page does work for me:
https://hitco.at/blog/mozilla-thunderbird-heise-rss-feed-korrekt-darstellen/
With tells me, that it is still the noscript issue.
Comment 68 Magnus Melin 2015-04-05 04:57:45 PDT
Please file a follow-up bug with details. 
I just retested and it at least (still) fixes the feed that was causing problems for me. The mozmill test also still passes, so what you are seeing might be something different.
Comment 69 Joe Sabash [:JoeS1] 2015-04-05 13:56:41 PDT
Created attachment 8588464 [details]
rss.jpg
Comment 70 Joe Sabash [:JoeS1] 2015-04-05 14:02:06 PDT
I hate when that happens, I posted the attachment and my comment was lost
Anyhow it views fine in the messagepane, but fails in a tab
I'll file a new bug if needed

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