The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla7

Status

()

Core
Networking: HTTP
--
major
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: Jan Skrasek, Assigned: bjarne)

Tracking

unspecified
mozilla7
x86
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
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
(Reporter)

Updated

6 years ago
Component: History: Global → Document Navigation
QA Contact: history.global → docshell
> 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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
(Reporter)

Comment 3

6 years ago
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.
> 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....
Duplicate of this bug: 636007
(Reporter)

Comment 6

6 years ago
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.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
The tab restore code says to load from cache unconditionally, so the necko code does just that.
Component: Document Navigation → Session Restore
Product: Core → Firefox
QA Contact: docshell → session.restore
(In reply to comment #7)
> The tab restore code says to load from cache unconditionally, so the necko code
> does just that.

Re-resolve?
Well, the question is whether the restore code really wants to be doing what it does...
(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?
(Reporter)

Comment 11

6 years ago
Well, is there any progress? I'd like see this fixed in Fx6 :)
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)?
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?
(Assignee)

Comment 14

6 years ago
(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.
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....
(Assignee)

Comment 16

6 years ago
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)?
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...
(Assignee)

Comment 18

6 years ago
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...?
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.
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.
(Assignee)

Comment 21

6 years ago
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..)
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.
(Assignee)

Comment 23

6 years ago
Fair points. I guess we can safely say that comment #21 was not very well thought through. :\
(Assignee)

Comment 24

6 years ago
Created attachment 533931 [details] [diff] [review]
Proposed fix

Passes simple local testing. Pushed to tryserver.
Assignee: nobody → bjarne
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #533931 - Flags: review?(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.
Attachment #533931 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 26

6 years ago
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.
> Is there a bug for this?

Doubt it.  Please file one?
(Assignee)

Comment 28

6 years ago
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?
File bugs, dunno.  Write tests usually yes.
(Assignee)

Comment 30

6 years ago
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.
Attachment #533931 - Attachment is obsolete: true
Attachment #534398 - Flags: review?(bzbarsky)
Comment on attachment 534398 [details] [diff] [review]
Proposed fix extended with more tests

r=me
Attachment #534398 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 32

6 years ago
Requesting check-in. If the reporter have any way to verify that this resolves the original issue, feedback would be appreciated.
Keywords: checkin-needed
(Reporter)

Comment 33

6 years ago
Sadly, I am unable to compile & test it.
Pushed to cedar: http://hg.mozilla.org/projects/cedar/rev/7827bde9f8d1
Component: Session Restore → Networking: HTTP
Flags: in-testsuite+
Keywords: checkin-needed
Product: Firefox → Core
QA Contact: session.restore → networking.http
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla7
http://hg.mozilla.org/mozilla-central/rev/7827bde9f8d1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar

Comment 36

6 years ago
(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/
(Reporter)

Comment 37

6 years ago
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!
Duplicate of this bug: 388141
You need to log in before you can comment on or make changes to this bug.