Closed Bug 971432 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Document Navigation, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jkitch, Assigned: jkitch)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
What and where?  ;)
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.
Depends on: 965413, 464222
Summary: Move → Consider moving baseURI from Channel to new "document settings object"
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.
Assignee: nobody → jkitch.bug
Status: NEW → ASSIGNED
Attachment #8511438 - Flags: review?(bzbarsky)
I am unlikely to get to this review before Tuesday afternoon at best.
We now look for the baseURI in a new place.

No hurry with the reviews, it isn't blocking anything.
Attachment #8511452 - Flags: review?(hsivonen)
Attachment #8511452 - Flags: review?(hsivonen) → review+
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
Attachment #8511438 - Flags: review?(bzbarsky) → review+
Comment added.

Also added is a warning that the attribute may have no effect if it isn't needed.
Attachment #8511438 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/80793ffb4e3c
https://hg.mozilla.org/mozilla-central/rev/d23660998a7b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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.
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.
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.
Flags: needinfo?(bzbarsky)
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.
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?
Flags: needinfo?(bzbarsky) → needinfo?(jonas)
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?
Flags: needinfo?(jonas)
> 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).
> > 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.
> 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.
(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.
> 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.
No longer blocks: 1193552
Depends on: 1193552
You need to log in before you can comment on or make changes to this bug.