355473 - FeedProtocolHandler.newChannel changes the URI

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect, P4)

Description

[Boris Zbarsky [:bzbarsky]]

The code does:

508 sayrer       1.6      if (uri.spec.substr(0, httpsChunk.length) == httpsChunk)
509                         uri.spec = "https://" + uri.spec.substr(httpsChunk.length

etc.

Then it does:

516                       channel.originalURI = uri;

But it's changed the URI, so that's pointless. And worse, it's changed the caller's data structure...  One of these days we will fix things so that URIs will be immutable general while being passed around, but we're not there yet.

Should there be a clone() call somewhere here that we're missing?

Comment 1

[Robert Sayre]

hmm, yeah. It doesn't look like that code is correct. httpsChunk is "feed://https//". I think it should be "feed:https://", per http://www.25hoursaday.com/draft-obasanjo-feed-URI-scheme-02.html. I think that is the spec?

Thanks for volunteering to review, Boris. ;)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ergh.  As long as you don't expect fast review.  :(  See my queue.  :(

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that I'm _this_ close to making HTTP channel URIs immutable for 1.9.

Comment 4

[Phil Ringnalda (:philor)]

(In reply to comment #1)
> hmm, yeah. It doesn't look like that code is correct. httpsChunk is
> "feed://https//". I think it should be "feed:https://"

Certainly a bug, but not this one (I don't think) - filed bug 408599 on the way FeedProtocolHandler.newURI creates URIs with host == "https".

Comment 5

[Phil Ringnalda (:philor)]

Attachment: Enough? v.1

You know it's truly horrific when *I* understand that it's bad.

Given bug 408599, is this tacky behavior enough for this bug? I haven't been able to figure out whether or not setting .originalURI to something random of your own choosing is allowed, but it's certainly necessary here, since the feed preview page needs to be able to get to "what the URI would have been, if people didn't just make protocols up" to allow it to give up and show the real URL if it turns out not to be something that can be previewed.

If it's not, if .originalURI has to be the URI that was passed to newChannel, then I think we have to fix bug 408599 with my first option there, creating genuine URIs in newURI, which I suspect would work other than that we wouldn't get feed preview for feed: URIs that we wouldn't sniff as being feeds, but I don't know how to do it to test.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 293628 [details] [diff] [review]
Enough? v.1

Hmm...  The thing is, that if the load is happening in a window (e.g. a tab in Firefox) the caller is a docshell... which will go ahead and set the originalURI on the channel to the URI it passed to newChannel.  This used to work because this code actually mutated that URI, right?  But with this change, the originalURI will now be the feed: URI...  so with this patch things break, no?

We should probably not bother setting the originalURI here (or set it to aURI, better yet), and set the nsIChannel::LOAD_REPLACE flag on the channel we return.  That should work, I think...

Comment 7

[Phil Ringnalda (:philor)]

How would I know if things are broken? (Both a serious question and a nice expression of the state of this crud.)

I was confused about which originalURI preview needs: that's actually http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/feeds/src/FeedConverter.js&rev=1.29&mark=232#215 much later, when we've parsed the feed and are loading our XUL page but want to be able to sort of pretend that it's the http URL that the feed URL has turned into by then.

I thought maybe it was the way you see what evil newURI has done until the page starts loading (feed:http://example.com:449 is handy for that, since it gives you a long time to look until it times out), but that seems to be from a change around the 4th of July - with my patch, without it, or with LOAD_REPLACE, you still see feed://http//example.com:449 (please, oh please, talk me through fixing bug 408599, or just fix it for me).

So while I'm happy to change to LOAD_REPLACE and not set originalURI, so far I'm unable to tell whether or not it's doing what you want it to do.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> How would I know if things are broken?

I have no idea; I don't know what this code is trying to accomplish, really...

I checked the docshell code again, and setting LOAD_REPLACE in newChannel won't work because NS_NewChannel will clobber the load flags.  This has come up before; see bug 337339.

Comment 9

[Phil Ringnalda (:philor)]

Working backward from the far end, I sort of doubt it's trying to accomplish anything, other than maybe do what every other newChannel impl seems to do - if I was writing it from scratch, based on the other things in the tree, I'd very probably think I was required to set originalURI.

Down the line, in FeedConverter.handleResult, when we create a channel from about:feeds and set the originalURI to the http feed URL, we want something - to have the feed URL shown in the addressbar, and to be able to access that originalURI in FeedWriter._getOriginalURI, and that's all working fine.

To get there, we want nsFeedSniffer::GetMIMETypeFromContent to find the X-Moz-Is-Feed header, that tells it the request came from either the addressbar icon or a feed: URI, and is thus a sure-feed, and doesn't need to be sniffed.

So from newChannel, we want two things (beyond "get a URL that will actually load the damn thing, so we can get to sniffing and get it back"): to add the X-Moz-Is-Feed header, which we're doing fine at, and to show something less hideous than the actual feed://http//example.org/ URI in the addressbar. We used to be okay at that, but the new urlbarBindings from bug 366797 caused bug 408891, so now we aren't, but that seems to fail no matter what we do in newChannel, and succeeds without the new urlbar no matter what we do, so unless that problem is obvious to you, and fixable from here, I think the answer to what we want it to do is exactly "whatever will make you happy with it."

Comment 10

[Boris Zbarsky [:bzbarsky]]

If it really doesn't matter (and in fact unless there is an extremely good reason to do otherwise), the originalURI should be set to the URI that was passed to newChannel.

Comment 11

[Phil Ringnalda (:philor)]

Attachment: Enough! v.2

With a little bonus workaround for bug 408599, since I doubt anyone will fix it directly for 1.9. This way, instead of going to www.realscheme.com//host/path, we just won't linkify links to feed: URLs other than http/https, and for ones typed in the addressbar we'll go to the strange, but not as terrible, keyword search for "feed".

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 294927 [details] [diff] [review]
Enough! v.2

Yummy.

Comment 13

[Phil Ringnalda (:philor)]

browser/components/feeds/src/FeedConverter.js 1.30
browser/components/feeds/test/Makefile.in 1.3
browser/components/feeds/test/unit/head_feeds.js 1.1
browser/components/feeds/test/unit/test_355473.js 1.1