Last Comment Bug 633743 - reverting closed tab with pushState changes sends request with HTTP_X_REQUESTED_WITH:XMLHttpRequest
: reverting closed tab with pushState changes sends request with HTTP_X_REQUEST...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: x86 Windows 7
: -- major (vote)
: mozilla7
Assigned To: Bjarne (:bjarne)
:
:
Mentors:
: 388141 636007 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-12 09:12 PST by Jan Skrasek
Modified: 2016-02-03 11:21 PST (History)
8 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (6.38 KB, patch)
2011-05-20 04:56 PDT, Bjarne (:bjarne)
bzbarsky: review+
Details | Diff | Splinter Review
Proposed fix extended with more tests (9.22 KB, patch)
2011-05-23 04:54 PDT, Bjarne (:bjarne)
bzbarsky: review+
Details | Diff | Splinter Review

Description Jan Skrasek 2011-02-12 09:12:43 PST
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 2011-02-12 09:20:05 PST
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 [:bz] (still a bit busy) 2011-02-14 07:35:22 PST
> 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 2011-02-14 15:05:56 PST
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 [:bz] (still a bit busy) 2011-02-14 19:45:52 PST
> 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 5 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-03-04 17:46:19 PST
*** Bug 636007 has been marked as a duplicate of this bug. ***
Comment 6 Jan Skrasek 2011-03-05 02:55:56 PST
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 [:bz] (still a bit busy) 2011-03-05 06:17:18 PST
The tab restore code says to load from cache unconditionally, so the necko code does just that.
Comment 8 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-03-08 09:19:02 PST
(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 [:bz] (still a bit busy) 2011-03-08 09:43:13 PST
Well, the question is whether the restore code really wants to be doing what it does...
Comment 10 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-03-11 14:36:45 PST
(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 2011-05-05 02:24:05 PDT
Well, is there any progress? I'd like see this fixed in Fx6 :)
Comment 12 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-17 17:57:59 PDT
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 [:bz] (still a bit busy) 2011-05-17 18:08:15 PDT
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) 2011-05-18 02:00:27 PDT
(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 [:bz] (still a bit busy) 2011-05-18 07:37:36 PDT
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) 2011-05-19 03:44:28 PDT
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 [:bz] (still a bit busy) 2011-05-19 06:34:31 PDT
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) 2011-05-19 06:53:13 PDT
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 [:bz] (still a bit busy) 2011-05-19 06:55:19 PDT
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 [:bz] (still a bit busy) 2011-05-19 06:55:45 PDT
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) 2011-05-19 07:00:46 PDT
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 [:bz] (still a bit busy) 2011-05-19 07:09:38 PDT
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) 2011-05-19 15:09:13 PDT
Fair points. I guess we can safely say that comment #21 was not very well thought through. :\
Comment 24 Bjarne (:bjarne) 2011-05-20 04:56:58 PDT
Created attachment 533931 [details] [diff] [review]
Proposed fix

Passes simple local testing. Pushed to tryserver.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2011-05-20 07:28:57 PDT
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) 2011-05-20 13:46:38 PDT
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 [:bz] (still a bit busy) 2011-05-20 13:58:21 PDT
> Is there a bug for this?

Doubt it.  Please file one?
Comment 28 Bjarne (:bjarne) 2011-05-20 14:07:11 PDT
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 [:bz] (still a bit busy) 2011-05-20 14:16:08 PDT
File bugs, dunno.  Write tests usually yes.
Comment 30 Bjarne (:bjarne) 2011-05-23 04:54:29 PDT
Created attachment 534398 [details] [diff] [review]
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 [:bz] (still a bit busy) 2011-05-23 11:21:02 PDT
Comment on attachment 534398 [details] [diff] [review]
Proposed fix extended with more tests

r=me
Comment 32 Bjarne (:bjarne) 2011-05-23 12:43:09 PDT
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 2011-05-23 12:44:55 PDT
Sadly, I am unable to compile & test it.
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2011-05-23 17:38:05 PDT
Pushed to cedar: http://hg.mozilla.org/projects/cedar/rev/7827bde9f8d1
Comment 35 Matt Brubeck (:mbrubeck) 2011-05-24 15:03:25 PDT
http://hg.mozilla.org/mozilla-central/rev/7827bde9f8d1
Comment 36 Michael Lefevre 2011-05-25 11:59:05 PDT
(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 2011-05-25 12:18:14 PDT
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!
Comment 38 Honza Bambas (:mayhemer) 2016-02-03 11:21:35 PST
*** Bug 388141 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.