Bobby Holley (:bholley) Assignee
	Description • 11 years ago

Since bug 809290 landed, the spec has ended up doing an about-face on this issue, introducing the notion of incumbent script, which for us roughly translates to JS_DescribeScriptedCaller. In [1], Hixie decided that location.href should use the incumbent script. This means we need to undo the change in bug 809290 (though hopefully the final setup can be cleaner than the old one).

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=19662#c16

	Bobby Holley (:bholley) Assignee
	Comment 1 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=00f6b940b655

	Bobby Holley (:bholley) Assignee
	Comment 2 • 11 years ago

Green. Flagging for review.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 10 years ago

Comment on attachment 8350164 [details] [diff] [review]
document.referrer should be based on the incument script settings object. v1

(I'll need to go through the spec again and our implementation of GetIncumbentGlobal to understand what they mean and do and how GetDynamicScriptGlobal is different to GetIncumbentGlobal.
And what is the difference between incumbent and entry settings objects)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 10 years ago

(uh, the spec is hard to read.)
"When the assign(url) method is invoked, the UA must resolve the argument, relative to the API base URL specified by the entry settings object..."
"Navigation for the assign() and replace() methods must be done with the responsible browsing context specified by the incumbent settings object as the source browsing context."

Why that? Why using entry settings object for one thing and right after that incumbent settings object
for other thing. Odd inconsistency. I think both should use incumbent settings object.
(If I've understood the horribly named incumbent settings object correctly. I tend to rename it in my mind to 'effective script context' to make some sense to it.)

	Bobby Holley (:bholley) Assignee
	Comment 6 • 10 years ago

(In reply to Olli Pettay [:smaug] from comment #4)
> (uh, the spec is hard to read.)
> "When the assign(url) method is invoked, the UA must resolve the argument,
> relative to the API base URL specified by the entry settings object..."
> "Navigation for the assign() and replace() methods must be done with the
> responsible browsing context specified by the incumbent settings object as
> the source browsing context."
> 
> Why that? Why using entry settings object for one thing and right after that
> incumbent settings object
> for other thing. Odd inconsistency. I think both should use incumbent
> settings object.

In general, all the stuff about the entry settings object is purely for web-compat. Base URI resolution is one of the cases where it's been shown that we need to use the entry (rather than incumbent) URI.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 10 years ago

Comment on attachment 8350164 [details] [diff] [review]
document.referrer should be based on the incument script settings object. v1

r=me on the code-level changes.

A question about the spec change, though: what do other UAs do?

	Bobby Holley (:bholley) Assignee
	Comment 8 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #7)
> A question about the spec change, though: what do other UAs do?

http://people.mozilla.org/~bholley/testcases/a-calls-b-navigates-c/a.html

Firefox and IE use the entry script. Chrome and Safari use the incumbent script.

With this change, IE will be the only UA left using the entry script.

	Bobby Holley (:bholley) Assignee
	Comment 9 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5370ef3abcdc

	Bobby Holley (:bholley) Assignee
	Comment 10 • 10 years ago

The old bug title was pretty misleading. Updating.

	Bobby Holley (:bholley) Assignee
	Comment 11 • 10 years ago

Actually, no, I think I was just confused.

	Bobby Holley (:bholley) Assignee
	Comment 12 • 10 years ago

Ah, ok. Now I think I understand what's going on.

Before Bob's changes in bug 785310 (which have not yet landed), we explicitly tracked the referrer, but not the source browsing context. That's what's being moved around in this bug.

If I read the spec right ([1]), the referrer should generally come from the source browsing context. With Bob's changes, we explicitly track the source browsing context. Once we do that, I think we can get rid of the explicit referrer override in nsLocation.cpp, and instead have the docshell code default the referrer to the source browsing context.

Bob, does that all sound right? If so, we should get a bug filed.

[1] http://www.whatwg.org/specs/web-apps/current-work/#processing-model

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 10 years ago

> Firefox and IE use the entry script. Chrome and Safari use the incumbent script.

OK.  Change sounds fine, then.

I think the comment 10 summary was closer to right, since this patch only affects location-based navigation, not referrers in other cases.

	Bob Owen (:bobowen)
	Comment 14 • 10 years ago

(In reply to Bobby Holley (:bholley) from comment #12)
> Before Bob's changes in bug 785310 (which have not yet landed), we
> explicitly tracked the referrer, but not the source browsing context. That's
> what's being moved around in this bug.
> 
> If I read the spec right ([1]), the referrer should generally come from the
> source browsing context. With Bob's changes, we explicitly track the source
> browsing context. Once we do that, I think we can get rid of the explicit
> referrer override in nsLocation.cpp, and instead have the docshell code
> default the referrer to the source browsing context.
> 
> Bob, does that all sound right? If so, we should get a bug filed.
> 
> [1] http://www.whatwg.org/specs/web-apps/current-work/#processing-model

That's certainly how I read that part of the spec.
In nsDocShell, it looks like the referrer is passed into InternalLoad (and then DoURILoad) from LoadURI out of the LoadInfo.
So, using the source browsing context passed into InternalLoad, should be relatively straight forward, on the face of it.
Not sure how the "specific override referrer source" part of the algorithm currently works, but I assume we would have to allow for that.

I've had very little time over the holidays, so I still have to re-write those tests for bug 785310.
Even once bug 785310 has landed, I think there are still at least three places where the source browsing context being passed is null when it shouldn't be. These are:
1) Navigation of an iframe by setting src or srcdoc (should be the iframe's document's browsing context).
2) History traversal (should be original source browsing context from when the history entry was created, bug 947716 raised).
3) For window.open (should be incumbent script's browsing context. However, because the code changes the state of the window we are navigating before the point at which we pass the source browsing context (into LoadURI), I have to do the sandboxing check earlier. So we effectively pass a null source browsing context to prevent the checks being done again.)

I still need to file bugs for 1 and 3, so I'll get onto that.
They don't currently affect the sandboxing behaviour.
I also need to check any other things that navigate by calling nsDocShell::LoadURI.

You, also mentioned that there may be some cases in Gecko where we will not be able to pass a source browsing context to LoadURI, but I suppose this would just result in an empty string for the referrer.

So, I assume any bug we raise for this will need to depend on the two I need to file.

	Bobby Holley (:bholley) Assignee
	Comment 15 • 10 years ago

Great - sounds like you're on top of it! I'll leave it to you to file all the relevant bugs and set up the dependency chain to get ourselves fully sorted out here.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/5370ef3abcdc