633743 - reverting closed tab with pushState changes sends request with HTTP_X_REQUESTED_WITH:XMLHttpRequest

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jan Skrasek]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110212 Firefox/4.0b12pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110212 Firefox/4.0b12pre ID:20110212030346

Firefox sends HTTP_X_REQUESTED_WITH=XMLHttpRequest when you restoring closed tab, which was before closing changed by history.pustState().

The requested url is the same as for ajax, as for "common" access. Script returned json or html in dependence on http header HTTP_X_REQUESTED_WITH.

Reproducible: Always

Steps to Reproduce:
1. Open page
2. Change content by ajax (returned json data are transformed into html) and stored url by history.pushState()
3. Close the tab.
4. Reopen the tab (CTRL+SHIFT+T)
Actual Results:  
Firefox sends request with header: HTTP_X_REQUESTED_WITH=XMLHttpRequest.

Expected Results:  
Firefox sends request without HTTP_X_REQUESTED_WITH.

Comment 1

[Jan Skrasek]

There is a test: http://temp.skrasek.com/ff/testcase_633743.php

Try clicking the link, then close the tab and reopen, you will see json (parsed as html because of no content type header).

Php script source: http://temp.skrasek.com/ff/testcase_633743.phps

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Firefox sends HTTP_X_REQUESTED_WITH=XMLHttpRequest when you restoring 

Actually, no.  It doesn't.

The idea of pushState is that loading the url that was pushed will hand back the page that was created while calling pushState.

This PHP script does NOT do that.  It hands back JSON data while actually loading the URL that was pushed will hand back some other page.

Worse yet, the return value of the script is conditioned on a request header, but the response does not set a Vary header to indicate this, and the response is cacheable.

So what happens here is that when reopening the tab we try to load <http://temp.skrasek.com/ff/testcase_633743.php?param=2>, see that we have it cached (the JSON response to the previous XHR) and just treat that JSON data we have cached as the server response.  You can see this clearly if you view source after your step 4 above.

This PHP script should be setting "Vary: X-Requested-With" or not allowing caching of the response or _something_ to indicate to the browser that it does wacky things based on not just the URL.

Comment 3

[Jan Skrasek]

Thank you for your reply!
I added Vary header and I get still the same results, page is cached.
In my real case I solved it by additional url parameter, but I think it's quite common to serve the same contents based on X-Requested-with header.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I added Vary header

Hmm.  Looks like the other issue is that restoring a tab forces a read from cache... which is where the other issue comes in: the fact that the pushState'd url doesn't actually return the data that matches the page that results when the pushState is done....

Comment 6

[Jan Skrasek]

I reopen this bug.
Reverting closed tabs use cache AND doesn't validate it by Vary header.
Different headers in history and cache are caused by history.pushState.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The tab restore code says to load from cache unconditionally, so the necko code does just that.

Comment 8

[u279076]

(In reply to comment #7)
> The tab restore code says to load from cache unconditionally, so the necko code
> does just that.

Re-resolve?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Well, the question is whether the restore code really wants to be doing what it does...

Comment 10

[u279076]

(In reply to comment #9)
> Well, the question is whether the restore code really wants to be doing what it
> does...

Who's best to answer that question? Paul?

Comment 11

[Jan Skrasek]

Well, is there any progress? I'd like see this fixed in Fx6 :)

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I sort of like restoring from cache if available. I'd be up for changing that in cases where that's not appropriate though.

We're just calling nsSHistory::ReloadCurrentEntry... Is it possible to call Reload with a set of flags that makes it Just Work (reload from cache where appropriate, otherwise don't)?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  Not easily.

Maybe we should add this header and others like it to the cache key?  Or still apply Vary even in cases when ONLY_FROM_CACHE?

Comment 14

[Bjarne (:bjarne)]

(In reply to comment #12)
> I sort of like restoring from cache if available. I'd be up for changing
> that in cases where that's not appropriate though.

[ snip ]

> Is it possible to call
> Reload with a set of flags that makes it Just Work (reload from cache where
> appropriate, otherwise don't)?

What would be "appropriate" in your case? I.e. could you describe when you want to get the resource from cache and when you'd like to load it from server?

If I understand this correctly ONLY_FROM_CACHE-flag is used, and as bz says it causes the resource to be loaded from cache, period. It might be better to use other flags, possibly introduce more granularity to them, but it would be a good starting point to get a feeling of how fine-grained this issue is.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Bjarne, the fundamental issue here is that we use the url as the key for GET, but the cached resource depends on more than just the url.  The most common dependence is time, of course, and LOAD_FROM_CACHE (which is what I believe this codepath uses) should ignore time-dependence issues like expiration time and such.  But the other obvious thing that can cause a response for a given url to change is different request headers.  As long as the server is making that clear via Vary, I think we should maybe honor that even for LOAD_FROM_CACHE loads....

Comment 16

[Bjarne (:bjarne)]

Right. So we could increase the granularity of control from clients by e.g. introducing a flag LOAD_FROM_CACHE_IF_RESOURCE_WOULD_NOT_VARY or something, no? (Alternatively LOAD_FROM_CACHE_BUT_IGNORE_ONLY_TIME_DEPENDENCIES... :) )

Seriously: I guess we're looking for a way to request a specific entity for a resource from the cache? Or at least making the LOAD_FROM_CACHE flag be aware of entities (i.e. honor Vary headers)?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm arguing for the latter; I can't think of _any_ case where we'd want to load the wrong entity via LOAD_FROM_CACHE (as opposed to an outdated view of an entity, which we want all the time).  The fact that the two entities share the same cache key is a cache implementation detail, in fact; I would argue that it should never be exposed to HTTP clients...

Comment 18

[Bjarne (:bjarne)]

I tend to agree when you put it like that...

Would it be a solution to extend the cache-key to also specify the entity (somehow)? That way we can cache multiple entities of the same resource - might be interesting also in the context of partial requests...?

Comment 19

[Boris Zbarsky [:bzbarsky]]

I would be fine with that, as coment 13 makes clear.  ;)

That's probably more work than just having Vary apply to LOAD_FROM_CACHE, though.

Comment 20

[Boris Zbarsky [:bzbarsky]]

To be clear, I think it's worth doing the work; the only question is whether we want to fix this in the simple way in the meantime.

Comment 21

[Bjarne (:bjarne)]

Could we concatenate all entity request-headers (as defined in RFC2616 section 7.1), hash the result and append it to the existing key? (Maybe even hash the uri also..)

Comment 22

[Boris Zbarsky [:bzbarsky]]

HTTP_X_REQUESTED_WITH is not in 7.1, right? 

Using the varying headers as part of the key would involve changing the key once we get the response and know what Vary says.  So like I said, it would be a good bit of work.

Comment 23

[Bjarne (:bjarne)]

Fair points. I guess we can safely say that comment #21 was not very well thought through. :\

Comment 24

[Bjarne (:bjarne)]

Attachment: Proposed fix

Passes simple local testing. Pushed to tryserver.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533931 [details] [diff] [review]
Proposed fix

r=me.  I think this also fixes another bug: in cases when ResponseWouldVary() and mCachedResponseHead->MustValidate() were both true we'd send If-Modified-Since, which I think is wrong, right?  With this patch we won't do that anymore.  If you agree that this was wrong, might be worth adding a test for it too.

Comment 26

[Bjarne (:bjarne)]

Tryserver looks ok.

(In reply to comment #25)
> I think this also fixes another bug: in cases when
> ResponseWouldVary() and mCachedResponseHead->MustValidate() were both true
> we'd send If-Modified-Since, which I think is wrong, right? 

Is there a bug for this? I'd prefer to add a test there, as opposed to here...

I think you're right, btw, and there might be similar subtle issues for other cases of that long if-else-if block. I'm not aware of any concrete bugs though... I guess it makes sense to verify that we're requesting the right entity first and perform other checks only if we do.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Is there a bug for this?

Doubt it.  Please file one?

Comment 28

[Bjarne (:bjarne)]

Well, to do this thoroughly I guess we should add bugs and tests for all the possible issues we might have seen because the check for ResponseWouldVary() was pretty late in the if-else-if..?

Not entirely convinced that's worth the trouble - should we file bugs and write tests for all possible issues we manage to *avoid* when coding?

Comment 29

[Boris Zbarsky [:bzbarsky]]

File bugs, dunno.  Write tests usually yes.

Comment 30

[Bjarne (:bjarne)]

Attachment: Proposed fix extended with more tests

Added tests for the other relevant variations in the if-else-if block. Re-requesting review to verify my understanding of what should work and what should not.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534398 [details] [diff] [review]
Proposed fix extended with more tests

r=me

Comment 32

[Bjarne (:bjarne)]

Requesting check-in. If the reporter have any way to verify that this resolves the original issue, feedback would be appreciated.

Comment 33

[Jan Skrasek]

Sadly, I am unable to compile & test it.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Pushed to cedar: http://hg.mozilla.org/projects/cedar/rev/7827bde9f8d1

Comment 35

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/7827bde9f8d1

Comment 36

[Michael Lefevre]

(In reply to comment #33)
> Sadly, I am unable to compile & test it.

Jan - You should now be able to test it using a nightly build from http://nightly.mozilla.org/

Comment 37

[Jan Skrasek]

Well, at first it didn't work and then I realized there is the old x64 build. 
I confirm it works ok in 7a1.
Great work! Thank you!