887928 - document.referrer should be based on the incumbent script for location-based navigation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

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

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: document.referrer should be based on the incument script settings object. v1

Green. Flagging for review.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Bobby Holley (:bholley)]

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

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Bobby Holley (:bholley)]

The old bug title was pretty misleading. Updating.

Comment 11

[Bobby Holley (:bholley)]

Actually, no, I think I was just confused.

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Bob Owen (:bobowen)]

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

Comment 15

[Bobby Holley (:bholley)]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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