971432 - Consider moving baseURI from Channel to new "document settings object"

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

What and where?  ;)

Comment 2

[James Kitchener (:jkitch)]

Bug 464222 and Bug 964239 introduce a baseURI attribute to nsIViewSourceChannel and nsIInputStreamChannel to allow relative links to work in situations where it cannot be inferred by other means.

Per bug 464222 comment 7, this attribute may be a better fit within the new document settings object.

Comment 3

[James Kitchener (:jkitch)]

Attachment: Part 1: Channel and Docshell changes

The baseURI is now moved to the nsILoadInfo object.

This simplifies the view source use of it, as there are no longer two places in which to look.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I am unlikely to get to this review before Tuesday afternoon at best.

Comment 5

[James Kitchener (:jkitch)]

Attachment: Part 2: Parser changes

We now look for the baseURI in a new place.

No hurry with the reviews, it isn't blocking anything.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8511438 [details] [diff] [review]
Part 1: Channel and Docshell changes

Please document in the IDL that baseURI can be null.

r=me

Comment 7

[James Kitchener (:jkitch)]

Attachment: Part 1: Channel and Docshell changes

Comment added.

Also added is a warning that the attribute may have no effect if it isn't needed.

Comment 8

[James Kitchener (:jkitch)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bc1c7d0de411

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80793ffb4e3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d23660998a7b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80793ffb4e3c
https://hg.mozilla.org/mozilla-central/rev/d23660998a7b

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why was this change made? Were there any specific bugs you were trying to fix?

The problem is that this fits pretty poorly with the infrastructure that we're trying to build around LoadInfo. Over in bug 1104623 it's currently causing us to add the baseURI argument to all sorts of functions on the ioservice as well as NetUtil.h which is very unfortunate.

Generally speaking the loadinfo should be context about how and why a network request got started so that we can make security and similar decisions during the request's lifetime. It's not intended to be general purpose metadata about a request.

Comment 12

[James Kitchener (:jkitch)]

It was implemented because of bz's suggestion in bug 464222 comment 7 (and later approved by him).

If it makes things unpleasant for the rest of the codebase, I have no objections to reverting it.  It doesn't do anything important.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd prefer that yeah. At least for now. LoadInfo is in a fairly half-implemented state, but we're working hard on finishing it up. It might make more sense to revisit this once it's in a more complete state.

I'd be happy to hear bz's thoughts too though.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What I mean is, it might make more sense to back out for now, and then look at re-landing once LoadInfo and the infrastructure around it is more complete.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Jonas, I believe what we want LoadInfo to represent is information that is invariant across a Fetch instance.  How and why the Fetch was started is one example of such information, but not all of them.

If we don't put this info on LoadInfo, we just have to invent another object that has the lifetime of a Fetch (complete with propagating across redirects) and carries the other data we care about....

If we do back this out, what is out plan for baseURI, exactly?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The main problem is that in our current architecture we have lots of functions that create channels. And lots of them are calling each other.

Including the version of newchannel that takes a loadinfo. It will soon (in order to fix various other bugs) be calling functions on ioservice to actually create the channel and the loadinfo with it.

I'd rather not add rarely needed arguments like baseURI to a bunch of these channel constructors.

Possibly we could enable the whole call chain to pass forward the loadinfo to where the channel is created. So that we would never need to pass forward separate "extra" arguments like baseURI separately. But we're not currently there. And that still wouldn't enable JS callers to define these "extra" arguments since JS can't instantiate loadinfos.

Another approach that could work is to enable nsILoadInfo to have setters for "extra" information. And then make those setters throw after AsyncOpen has been called on the channel.

Either way, I'd rather not add extra info to loadinfo until one of these solutions are in place if that's ok?


Another question, which should probably be asked first, is if baseURI is truly constant for the lifetime of a Fetch? Is that why it was moved to nsILoadInfo? I.e. does the baseURI stay constant across redirects? I would not have thought so?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Another approach that could work is to enable nsILoadInfo to have setters for "extra" 
> information.

That seems fine to me.  

> And then make those setters throw after AsyncOpen has been called on the channel.

That's harder, since the loadinfo has no idea about where the channel lives....

> is if baseURI is truly constant for the lifetime of a Fetch?

Realistically, the only cases in which anyone looks at the base URI are input stream channels (which do not do redirects) and view-source channels (which can do redirects underneath, I guess; that would do some weirdness with the base URI if you created a view-source URI for the pre-redirect channel... which we normally don't do).

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > is if baseURI is truly constant for the lifetime of a Fetch?
> 
> Realistically, the only cases in which anyone looks at the base URI are
> input stream channels (which do not do redirects) and view-source channels

If we're counting on these channels to not get redirected, then the question of which one is more "constant" for the lifetime of a Fetch seems academic. I.e. there doesn't seem to be any real advantages to sticking it on the loadinfo in that case, no?

> (which can do redirects underneath, I guess; that would do some weirdness
> with the base URI if you created a view-source URI for the pre-redirect
> channel... which we normally don't do).

Is there no situations in which we don't have a cached source and end up pinging the server? Most likely the server will not respond with a redirect since it didn't last time for the URL. But it seems like it could?

Though I guess in the view-source case, the baseURI used to live on the view-source channel which also means that it wouldn't be adjusted as it possibly should. Maybe we should explicitly prohibit redirects when doing view-source.

> > And then make those setters throw after AsyncOpen has been called on the channel.
> 
> That's harder, since the loadinfo has no idea about where the channel
> lives....

My thinking was that the channel could call some function on the loadinfo to "lock it down" from within AsyncOpen.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> I.e. there doesn't seem to be any real advantages to sticking it on the loadinfo in that
> case, no?

Well, there's uniformity in how one gets at it, right?  Instead of QIing to different interfaces and hoping...

> Is there no situations in which we don't have a cached source and end up pinging the
> server?

Sure, hence "we normally don't do".  But it can in fact happen, just as you describe.  Really, the use of the baseURI stuff is to have something sane for relative URLs in <a>s and <link>s and <script>s and forth in the source viewer; if it's wrong in weird edge cases that's not fatal.

> My thinking was that the channel could call some function on the loadinfo to "lock it
> down" from within AsyncOpen.

Ah, that would work, yes.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #19)
> > I.e. there doesn't seem to be any real advantages to sticking it on the loadinfo in that
> > case, no?
> 
> Well, there's uniformity in how one gets at it, right?  Instead of QIing to
> different interfaces and hoping...

Sure, but then it's no longer about using loadInfo for information that is constant for the lifetime of a fetch, but rather something that is generic to nsIChannels? Which granted is implementation-wise awkward given how many nsIChannel implementations that we have. (We might at some point want to consider reducing the number of implementations and instead have a shared base of some sort).

The other downside is, what does it mean to set the baseURI is now something that can be set on all channels? There's certainly a lot of other places that deal with baseURIs than nsHtml5TreeOpExecutor. Should that be honored as the baseURI of a document loaded through such a channel?

So there's definitely complexity involved in making baseURI fully cross-channel as well.


Anyhow, at this point we've found another solution which doesn't force us to pass the baseURI in a bunch of additional places. But the landed patch still doesn't sit super well with me. But there's no urgent need to find a different solution.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> what does it mean to set the baseURI is now something that can be set on all channels?

Fair.  I think filing a followup bug on sorting this stuff out is a good idea.