matthias koplenig [:metasieben] Reporter
	Description • 12 years ago

str:

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.

	(mostly gone) XtC4UaLL [:xtc4uall]
	Comment 1 • 12 years ago

FWIW, Aurora (16) is WFM.

	matthias koplenig [:metasieben] Reporter
	Comment 2 • 12 years ago

this build works as expected:
20120718030544
http://hg.mozilla.org/mozilla-central/rev/ae22909cef5a

this one doesn't anymore:
20120719030543
http://hg.mozilla.org/mozilla-central/rev/6d8456a77e57


hope this helps.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 12 years ago

Relevant range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ae22909cef5a&tochange=6d8456a77e57

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....

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 4 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 12 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?

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 6 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 12 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.....

	Alex Keybl [:akeybl]
	Comment 8 • 12 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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 9 • 12 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 12 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....

	neil@parkwaycc.co.uk
	Comment 11 • 12 years ago

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.

	Andrew Overholt [:overholt]
	Comment 12 • 12 years ago

Jonas says he thinks he knows what to do so assigning to him.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 13 • 12 years ago

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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 14 • 12 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 12 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?

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 16 • 12 years ago

Try push: https://tbpl.mozilla.org/?tree=Try&rev=d9fe5cc2b919

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 17 • 12 years ago

Given that we haven't heard of any other features than feeds breaking. I'd prefer to only change the logic here for feeds.

	neil@parkwaycc.co.uk
	Comment 18 • 12 years ago

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

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 19 • 12 years ago

Doh! Yes!

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 12 years ago

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

r=me with Neil's comment addressed.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 21 • 12 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 22 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2594523cb92

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/e2594523cb92

	neil@parkwaycc.co.uk
	Comment 24 • 12 years ago

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:
http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/src/FeedConverter.js#248
(Does it matter if you set the owner before or after the originalURI?)

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 25 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 12 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/abdfd1bc6e5a
https://hg.mozilla.org/releases/mozilla-beta/rev/fd73a7608750

	neil@parkwaycc.co.uk
	Comment 28 • 12 years ago

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.]

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 29 • 12 years ago

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?

	Manuela Muntean [Away]
	Comment 30 • 12 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 31 • 12 years ago

Do we need a new bug to track the "alternative patch"? Looks like it was forgotten.