js has to be enabled to parse rss-feeds (regression?)

RESOLVED FIXED in Firefox 17



Security: CAPS
5 years ago
5 years ago


(Reporter: matthias koplenig, Assigned: sicking)


({regression, reproducible})

regression, reproducible
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox16 unaffected, firefox17+ fixed, firefox18+ verified, firefox19 fixed)



(2 attachments)



5 years ago

1) disable js
2) open: http://www.heise.de/newsticker/heise-atom.xml

result: feeds doesn't get parsed.

expected result: feed should be displayed. possible regression, as it works as expected in the current stable release.


5 years ago
Keywords: qawanted, regressionwindow-wanted
FWIW, Aurora (16) is WFM.
OS: Mac OS X → All

Comment 2

5 years ago
this build works as expected:

this one doesn't anymore:

hope this helps.

Comment 3

5 years ago
Relevant range:


Yes, that helps.  Jonas, I will bet this is a regression from bug 774585: the script security manager looks at the principal URI in CanExecuteScripts() and if it's an about: URI checks for the ALLOW_SCRIPT flag on the about module.  I assume in the feeds case the channel's original URI is not about:feeds (perhaps it's the feed URI) and hence the principal's URI has changed with the patch.

That actually seems like a bad idea to start with, because we suddenly changed permissions on the feeds page....
Blocks: 774585
tracking-firefox17: --- → ?
Component: General → Security: CAPS
Ever confirmed: true
Keywords: qawanted, regressionwindow-wanted → regression
Boris: Any ideas for how to fix this? I'm really not a fan of how we used to hardcode the principal based on calling GetCodebasePrincipal. In general we have *way* too many callers of GetCodebasePrincipal, any time we call that we're basically doing security checks based on URIs rather than on principals.

Comment 5

5 years ago
So the problem is that this case is weird.  The original URI for this channel is whatever it is.  Then the feed reader redirects to about:feeds which is itself a redirect.

We have a way to use the original URI for the principal, or the final URI, but not some intermediate URI....

We could add some explicit way of setting the URI to use for the principal, I guess.  But if we're going to do that, it's saner to just set the owner: overriding the principal is what it's for.

What would break if we did that?
Arg, lost a longer comment.

Basically I don't know of anything that would break. In general I'd like to reduce the number of callsites that we have that create principals so that we can clean them up a bit. Right now they are a DOM-specific and not much more than a wrapper around a URI, and making them anything else is pretty hard. For example if we want to add cookie jars to firefox it'd be pretty hard to make sure that we're using the right principal everywhere. Likewise we end up losing the CSP policy which is stored in the principal unless we go directly to the document and get the principal from there.

I'd like to be able to pass around principals to more than DOM code. For example necko really should use principals more often. But right now that's not terribly reliable. The extra appid/browserflag information we've added to principals is pretty B2G specific. There's simply too much firefox code where we simply didn't have enough context and just wrapped a principal around a URI instead.

None of this is very easy of course. And definitely for other bugs.

Ssetting a specific principal on about:feeds would be an ok solution for now. I don't think anything would break.

Comment 7

5 years ago
OK.  Note that it's not just about:feeds.  It's everything that has ALLOW_SCRIPT and also has URI_SAFE_FOR_UNTRUSTED_CONTENT.  That seems to be "feeds", "neterror", "blocked", "certerror", "rights", "robots", "home".

I'm actually kinda surprised we haven't had more problems reported.....

Comment 8

5 years ago
Knowing how slammed Jonas is right now with B2G, assigning to Andrew to help find somebody to look at this regression from bug 774585.
Assignee: nobody → overholt
tracking-firefox17: ? → +
Boris: I don't understand why we have to do this for other URLs than about:feed?

Isn't about:feed the only URL where we're starting with http URL (or other "normal protocol) redirect to an about: url, which redirects to a chrome: (or resource:?) URL, and then want to use the about: URL for the principal?

All other examples you listed start with about: URLs and that's what we want to use for the principal, no?

Comment 10

5 years ago
That's a good question.  I guess for the neterror and certerror cases we do start with those right now and fake the url bar via another mechanism?  They do sort of start with an http:// URI, but we do some weirdness after that..

What about about:blocked?  In that case we _do_ start with an http:// URI, I believe.  Is it handled like neterror/certerror?

But mainly, I wanted us to do something consistent so we wouldn't have to do fragile reasoning about exactly how all sorts of weird cases are loaded....
FeedConverter.js opens about:feeds to handle the preview, which the about redirector then redirects to a chrome: URL, but then FeedConverter.js sets the channel's original URI to be that of the feed. This means that the document's URI is that of the feed, not about:feeds or the chrome: URL.

Error pages never change the docshell's idea of the current URI. This means that the document's URI is an about: page, but the UI still displays the originally attempted URI.
Jonas says he thinks he knows what to do so assigning to him.
Assignee: overholt → jonas
Jonas - are you going to be able to work on this in the next few weeks?  This regression will go to our Beta audience in just over a week and we might start seeing more reports about this issue.
status-firefox16: --- → unaffected
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox18: --- → +


5 years ago
Keywords: reproducible
Also - can someone comment on how this affects users with disabled js outside of this one reproducible issue?  Would this have potential for wide impact in a variety of XML-related web interactions?

Comment 15

5 years ago
This issue is specific to about: URIs that are loaded as "untrusted" but include script.  And not only that, but you have to have a redirect to such a URI from another URI.

So we're pretty sure that this only impacts about:feeds in practice.

sicking, are you fixing this?
Try push: https://tbpl.mozilla.org/?tree=Try&rev=d9fe5cc2b919
Created attachment 673788 [details] [diff] [review]
Patch to fix

Given that we haven't heard of any other features than feeds breaking. I'd prefer to only change the logic here for feeds.
Attachment #673788 - Flags: review?(bzbarsky)
Comment on attachment 673788 [details] [diff] [review]
Patch to fix

>+        if (!strcmp(path.get(), "feeds")) {
ITYM if (path.EqualsLiteral("feeds")) {
Doh! Yes!

Comment 20

5 years ago
Comment on attachment 673788 [details] [diff] [review]
Patch to fix

r=me with Neil's comment addressed.
Attachment #673788 - Flags: review?(bzbarsky) → review+
Comment on attachment 673788 [details] [diff] [review]
Patch to fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 774585

User impact if declined: The feed reader would be broken for users that turn off Javascript (which is rare). All in all I don't think this would affect a lot of users since I suspect the intersection of disabling JS and using feed reader is small.

Testing completed (on m-c, etc.): Try run seems to have passed with only known oranges: https://tbpl.mozilla.org/?tree=Try&rev=d9fe5cc2b919

Risk to taking this patch (and alternatives if risky): The patch is very safe. It simply reverts behavior to what we had in Firefox 16 when handling about:feed URLs. Backing out the original patch would be a bigger change and likely a bigger risk given how much other changes were made in the FF17 timeframe around principal handling.

String or UUID changes made by this patch: None
Attachment #673788 - Flags: approval-mozilla-beta?
Attachment #673788 - Flags: approval-mozilla-aurora?


5 years ago
Blocks: 804411
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
So, I've been looking at this to port it to SeaMonkey, and it has just dawned on me that a better place to set the owner might be in FeedConverter.js:
(Does it matter if you set the owner before or after the originalURI?)
Created attachment 674728 [details] [diff] [review]
Alternative patch (only for m-c)

Yes, that's a much better approach!

This patch uses that approach. I think we should still stick with the previous patch for the release branches since that is a safer approach as it just reverts us to previous behavior.

But this patch would be a better way to fix things for future releases.
Attachment #674728 - Flags: review?(bzbarsky)

Comment 26

5 years ago
Comment on attachment 674728 [details] [diff] [review]
Alternative patch (only for m-c)

Hrm.  So "chromeURI" is horribly misnamed, eh?

r=me on the owner bit; the rest could use review from someone who actually knows this code.
Attachment #674728 - Flags: review?(bzbarsky) → review+


5 years ago
Attachment #673788 - Flags: approval-mozilla-beta?
Attachment #673788 - Flags: approval-mozilla-beta+
Attachment #673788 - Flags: approval-mozilla-aurora?
Attachment #673788 - Flags: approval-mozilla-aurora+
Attachment #674728 - Flags: review?(neil)
status-firefox17: affected → fixed
status-firefox18: affected → fixed
status-firefox19: --- → fixed
Comment on attachment 674728 [details] [diff] [review]
Alternative patch (only for m-c)

Sorry for the delay in redirecting this but I'm not a browser peer, although the patch looks reasonable.

>-      var ios = 
>-          Cc["@mozilla.org/network/io-service;1"].
>-          getService(Ci.nsIIOService);
>+      var ios = Services.io;
[Not sure whether it's worth keeping the temporary variable.]
Attachment #674728 - Flags: review?(neil) → review?(gavin.sharp)
Comment on attachment 674728 [details] [diff] [review]
Alternative patch (only for m-c)

Can you land the service.jsm cleanup separately from the actual functional change here?
Attachment #674728 - Flags: review?(gavin.sharp) → review+
Verified fixed on Firefox 18 beta 4.

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20121212073002
status-firefox18: fixed → verified
QA Contact: manuela.muntean
Do we need a new bug to track the "alternative patch"? Looks like it was forgotten.
status-firefox17: fixed → affected
status-firefox18: verified → affected
status-firefox19: fixed → ---
Target Milestone: mozilla19 → ---
status-firefox17: affected → fixed
status-firefox18: affected → verified
status-firefox19: --- → fixed
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.