622088 - XHR referer header is not modified with history.pushState

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jakub Vrána]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8

When the script changes the location through history.pushState then further requests send the Referer header with the original location and not with the modified one.

Google Chrome works correct.

Reproducible: Always

Steps to Reproduce:
history.pushState('', '', location.href + '?test');
var req = new XMLHttpRequest();
req.open('GET', 'history.php?referer=1');
req.onreadystatechange = function () {
	if (req.readyState == 4) {
		alert(req.responseText);
	}
};
req.send();

Actual Results:  
'http://www.vrana.cz/test/history.php'

Expected Results:  
'http://www.vrana.cz/test/history.php?test'

Comment 1

[Boris Zbarsky [:bzbarsky]]

Jason, do you want to fix, or should I?

Comment 2

[Justin Lebar (not reading bugmail)]

Presumably it's just XMLHttpRequests?  I could have sworn we have a test for regular requests.

Comment 3

[Justin Lebar (not reading bugmail)]

I'll try and have a look at this soon.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah.  Indeed, XHR gets its referrer value from the principal, which is unaffected by pushstate.

We really need a better setup for getting the referrer off the JS stack.  :(

Comment 5

[Benjamin Smedberg]

I really don't think this is serious enough to block. bz do you disagree?

Comment 6

[Jakub Vrána]

It's quite important for me (if my vote counts).

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's a sorta-serious bug in one of our new gecko 2.0 features that will force web developers into contorted workarounds.  I agree this should probably not block ship, but I think we should try to fix for gecko 2.0 if we can.

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Pushing to try now.

Comment 9

[Justin Lebar (not reading bugmail)]

Try results pending at http://tbpl.mozilla.org/?tree=MozillaTry&rev=c825450a73f6

Comment 10

[Boris Zbarsky [:bzbarsky]]

1)  The principal during Send() may not match mPrincipal (e.g. if the XHR is created in one window but send() is called from another one).  Which one does the spec say to actually use for the referrer?  Maybe we need to save the referrer URI when we save mPrincipal.

2)  GetContextDocumentCurrentURI doesn't do what the documentation says or what the name implies.  It does the right thing, so the documentation and name need to change.

Comment 11

[Justin Lebar (not reading bugmail)]

(In reply to comment #10)
> 2)  GetContextDocumentCurrentURI doesn't do what the documentation says or what
> the name implies.  It does the right thing, so the documentation and name need
> to change.

Indeed.  Is it correct that, in the case that we don't use the principal's URI because the document's original URI doesn't match the principal's URI, the function does what the name implies and the documentation says?

Also, was it appropriate to add GetContextFromStack to nsJSUtils?  The comment at the top of the file [1] made it seem like maybe not.

[1] http://hg.mozilla.org/mozilla-central/file/f539d05eec83/dom/base/nsJSUtils.cpp#l41

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Is it correct that, in the case that we don't use the principal's URI
> because the document's original URI doesn't match the principal's URI, the
> function does what the name implies and the documentation says?

No.  The document involved is the document associated with whatever script is running, which might be wholly unrelated to the document that "owns" (the ownership is in the other direction, sorta) the given JSContext.

As for nsJSUtils, it's fine with me, but double-check with jst...

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to comment #10)
> 1)  The principal during Send() may not match mPrincipal (e.g. if the XHR is
> created in one window but send() is called from another one).  Which one does
> the spec say to actually use for the referrer?

Do you know where this might be?  I don't see referer/referrer mentioned in [1].

[1] http://www.w3.org/TR/XMLHttpRequest/#dom-xmlhttprequest-send

Comment 14

[Justin Lebar (not reading bugmail)]

(In reply to comment #12)
> As for nsJSUtils, it's fine with me, but double-check with jst...

cc'ing jst.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> I don't see referer/referrer mentioned in [1].

Sounds like some spec work is needed here.

Comment 16

[Justin Lebar (not reading bugmail)]

(In reply to comment #15)
> > I don't see referer/referrer mentioned in [1].
> 
> Sounds like some spec work is needed here.

Anne, do you have thoughts on this?

FWIW, it seems to me that the referrer should be the window which calls send(), just as the referrer on a normal navigation is the window which initiates the navigation, not the window being navigated.  But I plead ignorance.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Well, should it be the window that calls send(), the window that calls open(), the window that makes the |new XMLHttpRequest()| call, or the window that the call is made on?  Those 4 can all be different windows.

Comment 18

[Boris Zbarsky [:bzbarsky]]

And fwiw, for a scripted form submit the referrer is the form's document, not the script that called submit().  Assuming that's at all analogous to our situation here.

Comment 19

[Justin Lebar (not reading bugmail)]

Maybe we shouldn't spin waiting for the Final Word on the spec here.  We can fix the big problem (pushState doesn't affect referrer) now and then figure out exactly which window should be the referrer later.

bz, which do you think should be the referrer?

Comment 20

[Boris Zbarsky [:bzbarsky]]

I think we should preserve the old behavior for now and get the referrer at the same time as we set mPrincipal.

Comment 21

[Anne (:annevk)]

XMLHttpRequest references HTML5 for "fetch". HTML5 says that you should look at the "entry script's document" in this case. So it is defined.

We could change it however to make it the XMLHttpRequest Document instead. This might be more logical as a lot of XMLHttpRequest logic is tied to the XMLHttpRequest Document. This would require changes to both HTML5 (to let fetch accept a document as argument) and XMLHttpRequest (to make use of that).

I do not feel strongly either way.

Comment 22

[Boris Zbarsky [:bzbarsky]]

OK, so that sounds like the equivalent of getting the "dynamic script context" (as opposed to "static script context", which is what the attached patch does) at the point when send() is called, then.

I'm not sure that makes much sense, necessarily, but in the edge cases nothing we do can make sense....

Comment 23

[Justin Lebar (not reading bugmail)]

Is getting the dynamic script context just a matter of replacing 

static already_AddRefed<nsIDocument>
GetFrameDocument(JSContext *cx, JSStackFrame *fp)
{
  [...]
  nsCOMPtr<nsIDOMWindow> window =
    do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(cx, scope));

with GetDynamicScriptGlobal?  Also, if I were to write a test to check that this gets the right referrer, what would I do?

Comment 24

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: interdiff v1 -> v2

Comment 26

[Justin Lebar (not reading bugmail)]

Patch v2 uses the dynamic script context for the XHR referrer.

The function names / comments might be off.  Also, I should explicitly test that we're getting the right referrer in the edge case.  But I'm not clear on what that edge case is.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Edge case for which?  A case where dynamic vs script context matters?  Or something else?

Comment 28

[Justin Lebar (not reading bugmail)]

> A case where dynamic vs script context matters?

If s/script/static, yes.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Dynamic vs static matters in this situation.  You have two windows A and B with different URLS (say a page and its subframe).  You click a button in window A; the onclick handler is a function in window A which calls a function in window B which does an XHR.  In that situation, the dynamic script context is the one for window A, but the static one is the one for window B.

Comment 30

[Justin Lebar (not reading bugmail)]

Either my test or my code is wrong here.  I suspect it's the code, but to check: Here's code which is running inside the main test window.  In this case, the dynamic script context for the inner window should be the outer window?

The referrer I'm getting here is the iframe's location.

var iframeFinishedLoading = false;
function callXHR() {
  if (innerFinishedLoading) {
    var inner = document.getElementById('iframe').contentWindow;
    var referrer = inner.doXHR();
    is (referrer, document.location); // FAILS
    SimpleTest.finish();
  }
  else {
    setTimeout(callXHR, 0); // ugh.
  }
}

callXHR();

Comment 31

[Boris Zbarsky [:bzbarsky]]

> In this case, the dynamic script context for the inner window should be the
> outer window?

Yes.  If it's not, that's really odd.  Can you post a full testcase or mail it to me or something?

Comment 32

[Justin Lebar (not reading bugmail)]

Well, it's probably a bug in my code!  I'll post the patch in a moment.

Comment 33

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2.1 (added test)

Comment 34

[Justin Lebar (not reading bugmail)]

The file in question is test_bug622088_2.html, but presuming that's right, the relevant code is in the nsContentUtils.cpp hunk.

I need to double-check that XHR really is causing the dynamic context branch to run, but I'm pretty sure it is.

Comment 35

[Justin Lebar (not reading bugmail)]

Yup, the two XHRs both go through the dynamic script global branch in nsContentUtils.cpp.

Comment 36

[Justin Lebar (not reading bugmail)]

Ah.  The problem isn't the GetDynamicScriptGlobal() call -- that works.  The issue is that the principal we get from GetCxSubjectPrincipalAndFrame corresponds to the static context.  So since the dynamic context's original URL doesn't match the principal's URL, we take the principal's.

Comment 37

[Justin Lebar (not reading bugmail)]

So the question is: How do I get the right principal in the dynamic case?

Comment 38

[Boris Zbarsky [:bzbarsky]]

In the dynamic case, can't you just get the principal from the document?

Comment 39

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Oh.  That was simple.  Only changes relative to v2.1 are in nsContentUtils.cpp.

Comment 40

[Justin Lebar (not reading bugmail)]

Hm.  Causes test failures on try...

http://tbpl.mozilla.org/?tree=MozillaTry&rev=2e98589a692a

Comment 41

[Boris Zbarsky [:bzbarsky]]

>+   * Get the current URI associated with the given context, either going
>+   * through its static global or its dynamic global.

How about documenting GetStaticContextURI as "get the current uri associated with the code currently running on cx" and GetDynamicContextURI as "get the current uri associated with the window corresponding to cx"?  That would be clearer, imo.... (and feel free to rename the functions as you do this if you think of better names!).

> +  // First, we get the document corresponding to fp.

That's not quite true anymore.

Looks good to me otherwise, but need the failures sorted out, right?  The ctypes thing looks unlikely to be you... can you reproduce the others?

Comment 42

[Justin Lebar (not reading bugmail)]

The ctypes certainly does appear not to be from this.  And the other one (timeouts in test_bug277724.html, test_bug172261.html) disappeared when I pushed without bug 615501, so presumably that bug (or its test) is doing something wrong.

I was pretty sure I was building off a green changeset!  But I'll update and push again.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 506254 [details] [diff] [review]
Patch v3

r=me with the comments fixed.

Comment 44

[Justin Lebar (not reading bugmail)]

I think the errors on try are just a simple null pointer dereference.  I'll fix and push again.

> // XXX Do I need to enter a compartment here?
> window = do_QueryInterface(nsJSUtils::GetDynamicScriptGlobal(cx));
>
> nsCOMPtr<nsIScriptObjectPrincipal> scriptObj = do_QueryInterface(window);
> NS_ENSURE_TRUE(scriptObj, NS_ERROR_FAILURE);
> principal = scriptObj->GetPrincipal();

Do I need to enter a compartment before I call GetDynamicScriptGlobal?  I seem to recall that I don't.

Comment 45

[Justin Lebar (not reading bugmail)]

Attachment: Patch v4

Added null check, updated comment.

Comment 46

[Boris Zbarsky [:bzbarsky]]

I have no idea for compartments.  Blake?

Comment 47

[Justin Lebar (not reading bugmail)]

Attached patch is green on try.  I'll push once I hear about compartments.

Comment 48

[Justin Lebar (not reading bugmail)]

jst says we don't need a compartment.  Let's do this.

Comment 49

[Justin Lebar (not reading bugmail)]

Requesting a2.0.  This patch looks a little scarier than it actually is, since the non-XHR referrer code it's touching was just changed as a result of bug 593174.

Comment 50

[Justin Lebar (not reading bugmail)]

This test is messing up mochitest, I think because it calls pushState on the main window.  I of all people should know better...

Comment 51

[Justin Lebar (not reading bugmail)]

Attachment: Patch v5

Updated test.

Comment 52

[Justin Lebar (not reading bugmail)]

In v5, I removed test_bug622088_1.html, renamed test_bug6220088_2.html to test_bug622088.html, and added the following chunk to the test:

> +  // Now change the location of the inner frame.  This should be reflected in
> +  // the XHR's referrer.
> +  inner.history.pushState('', '', Math.random());
> +  referrer = inner.doXHR();
> +  is (referrer, inner.document.location, 'Expected inner frame location after pushstate');

The point is to avoid calling pushState on the main window and instead call it only on an inner window.

bz, can you review this test change?  It's all green on try now.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Hmm.  How was the _1 test doing pushState on the main window?

Comment 54

[Justin Lebar (not reading bugmail)]

It did, outside a function,

> history.pushState('', '', Math.random());

So by "main window", I meant the top-level window for the test, not the top-level window which runs this test in an iframe.

When I ran the test stand-alone (outside Mochitest's iframe), the pushState worked fine, but going back caused Mochitest to reload and run the test again (onload fired and everything).  I'm not sure why this happened, but I presume this has to do with something strange Mochitest is doing, because I couldn't get it to happen on a regular page.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Hmm.  I don't think Mochitest is doing anything particularly weird.

But with your modification the test no longer test what it should be testing, does it?

Comment 56

[Justin Lebar (not reading bugmail)]

We need to test that:

a) the referrer is updated as a result of push/replaceState, and
b) we get the dynamic script context.

Test 2 initially tested (b), and the hunk I quoted in comment 52 tests (a).

No?

Comment 57

[Boris Zbarsky [:bzbarsky]]

Oh, because we actually got called from the onload handler in the inner.  OK, that makes sense.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 508444 [details] [diff] [review]
Patch v5

r=me; thanks for explaining!

Comment 59

[Justin Lebar (not reading bugmail)]

Comment on attachment 508444 [details] [diff] [review]
Patch v5

Thanks for the review!  Requesting approval again; see comment 49 for rationale.

Comment 60

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

Comment on attachment 508444 [details] [diff] [review]
Patch v5

Actually, I think this is wrong. We shouldn't use the context of the caller, but rather use mScriptContext.

Since we resolve the URL passed to xhr.open against the window whose constructor was used to create the XHR, it makes sense to me to use that as the referrer too. In fact, the spec should specify this.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Please fight that out with Anne, then.  That's not what he said the spec says!

Comment 62

[Justin Lebar (not reading bugmail)]

A quick, decisive fight would be preferable!

Comment 63

[Justin Lebar (not reading bugmail)]

Jonas, should it be

  GetDynamicContextURI(mContext)

or

  GetStaticContextURI(mContext)

?

Comment 64

[Justin Lebar (not reading bugmail)]

Oh, mScriptContext, not mContext.  So is it as simple as

> nsCOMPtr<nsIDocument> doc =
>   nsContentUtils::GetDocumentFromScriptContext(mScriptContext);
>
> nsCOMPtr<nsIURI> referrerURI;
> if (doc) {
>   referrerURI = doc->GetDocumentURI();
> }

?

this is how we get the URI to resolve the request against...

Comment 65

[Justin Lebar (not reading bugmail)]

Attachment: Patch v6 - Use mScriptContext's document's URI.

Comment 66

[Justin Lebar (not reading bugmail)]

Oh, I bet I need to do the same check I did in the navigation referrer code: Does the document's original URI match the principal's URI?  If so, use the document's current URI; otherwise, use the principal's URI.

Comment 67

[Justin Lebar (not reading bugmail)]

Attachment: Patch v7

This mostly works, but has a small problem.  The patch includes a failing test which does the following:

Document X at location x contains an iframe with a data: URI.  X then calls a function inside the iframe which returns an XMLHttpRequest.  Then X calls replaceState on itself, changing its URI to y.

When we finally send the XHR, it will observe that its document's original URI doesn't match its principal's URI, since the principal is X's original URI.  So it then uses x as the referrer, when in fact the referrer should be y.

Comment 68

[Justin Lebar (not reading bugmail)]

The problem above is basically the same as bug 631949.  Seeing as we already have this behavior for regular loads, maybe we can tolerate it for XHRs and revisit later.  Don't let perfect be the enemy of the good and all that.

Jonas, what do you think?

Comment 69

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

Comment on attachment 510174 [details] [diff] [review]
Patch v7

Yeah, I think that's fine for now.

However would be great to have this refactored into a function on nsIDocument so as to make it reusable at all needed locations.

Comment 70

[Justin Lebar (not reading bugmail)]

Comment on attachment 510174 [details] [diff] [review]
Patch v7

Requesting approval.  This newest patch is a nice, local change.

Comment 71

[Justin Lebar (not reading bugmail)]

By the way, don't try to check this patch in exactly as it is -- I need to remove the one failing test.

I should be able to push on Wednesday.

Comment 72

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/b3db908c1b87

Comment 73

[Jakub Vrána]

Thank you for your hard work.

Comment 74

[Justin Lebar (not reading bugmail)]

And than you for reporting it!

Comment 75

[Anne (:annevk)]

So where is the email to [e-mail]? *sighs*

Comment 76

[Boris Zbarsky [:bzbarsky]]

We should probably also document the behavior, whatever it is.  At this point, I've lost track of what got implemented here.

Comment 77

[Eric Shepherd [:sheppy]]

It would be helpful if someone can help figure out details of what the docs need to say, since if bz is confused, there's little hope for me.

Comment 78

[Justin Lebar (not reading bugmail)]

(In reply to comment #75)
> So where is the email to [e-mail]? *sighs*

I'm sorry about this.  Jonas or I will write to the list soon.

(In reply to comment #77)
> It would be helpful if someone can help figure out details of what the docs
> need to say, since if bz is confused, there's little hope for me.

I think it should be sufficient to say that the referrer is the current URL of the document which created the XHR object.

Comment 79

[Boris Zbarsky [:bzbarsky]]

Is it?  The code is using mScriptContext, which is not the document "that created the XHR object" (whatever that means), but the window that was the |this| when the constructor was invoked, no?

Comment 80

[Eric Shepherd [:sheppy]]

Anyone have further clarification here? Sounds like there's still some uncertainty about what exactly this fix accomplishes.

Comment 81

[Justin Lebar (not reading bugmail)]

bz might correct me again, but AIUI:

The referrer is the current URL of the document associated with mScriptContext.  Per comment 79, that document belongs to the window that was |this| when |new XmlHttpRequest()| was called.

This may change in the future, depending on what Jonas and Anne work out for the XHR spec.  But it should remain the case that the referrer reflects changes to the URL caused by push/replaceState.

Comment 82

[Eric Shepherd [:sheppy]]

This is noted here:

https://developer.mozilla.org/en/DOM/Manipulating_the_browser_history#Adding_and_modifying_history_entries