490095 - Going back reloads pages from the network more than it used to

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Sylvain Pasche]

Let's start with the testcase:

 * Open http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=1+week+ago&enddate=now
 * Navigate to the bottom of the page and click on a bug.
 * Once the bugzilla page has loaded, click "Back".
Notice that the hg.mozilla.org page is fetched again from the network which can take a non negligible amount of time and makes browsing between that page and bugzilla very painful.

This is a regression coming from bug 468594 (which has only landed on trunk). Before that, going back didn't fetch the page from the network, so the page load was very fast (but not as fast as a page loaded from the bfcache, see below).

The same issue happens on any page that is dynamically generated (contains no freshness information), contains GET parameters and has some JS which sets an unload handler (such as jQuery). Such environment is not uncommon, it happens with trac timeline pages for instance: http://trac.dojotoolkit.org/timeline?from=04%2F24%2F09&daysback=5&ticket=on&changeset=on&milestone=on&wiki=on&update=Update or http://trac.webkit.org/?from=04%2F24%2F09&daysback=4&changeset=on&wiki=on&update=Update

I tested with a few other browsers: any of IE 7, Opera 9, Safari 4 or Chrome 1 do not fetch the page from the network when you go back on such pages.

This doesn't mean bug 468594 is the issue. From the caching point of view, it makes things more conformant (if you bookmark such a page, you would want to have it fetched every time you open the URL). The issue is that jQuery is setting an unload handler which disables the bfcache and so makes the browser fetch the resource again. That issue is filed there: http://dev.jquery.com/ticket/3015 (discussion on http://groups.google.com/group/jquery-dev/browse_thread/thread/13e8f300f1ce1893).

Some may say that jQuery needs to be fixed and this bug should be simply WONTFIXED. However, even if jQuery is fixed right now, there will still be plenty of websites containing older versions that will exhibit this issue. And this isn't restricted to jQuery, this could affect all pages that set an unload handler (don't know if other toolkits do this).

From the user experience point of view this regression can be pretty annoying. So it would be nice to have it addressed somehow.

Comment 1

[WADA:World Anti-bad-Duping Agency]

(For Back/Forward of http : //hg.mozilla.org/.../?.... case)
Bfcahce looks to be bypassed.
Do you enable bfcache? If no, HTTP GET upon Back/Forward is very normal.  
(browser.sessionhistory.max_total_viewers>0 or -1)
What should be do on bcache(categorized as cache of history instead of HTTP cache)?

Comment 2

[Sylvain Pasche]

(In reply to comment #1)
> (For Back/Forward of http : //hg.mozilla.org/.../?.... case)
> Bfcahce looks to be bypassed.

Right, because jQuery sets an unload handler, which disables the bfcache.

> Do you enable bfcache? If no, HTTP GET upon Back/Forward is very normal.  
> (browser.sessionhistory.max_total_viewers>0 or -1)

This issue happens on a clean profile with the default Firefox settings, which has the bfcache enabled by having browser.sessionhistory.max_total_viewers sets to -1. I'm not sure what you mean by "very normal" in your last sentence and in which situation it happens.

> What should be do on bcache(categorized as cache of history instead of HTTP
> cache)?

I'm not sure what the solution should be. Caching could be done either at the Back/Forward level or at the resource level.
WebKit will also not put pages with unload handlers in the back/forward cache:
http://www.google.com/codesearch/p?hl=en#N6Qhr5kJSgQ/WebCore/loader/FrameLoader.cpp&q=hasWindowEventListener&exact_package=git://android.git.kernel.org/platform/external/webkit.git&l=1941

Instead, it will ask the cache to return the data without validation:
http://www.google.com/codesearch/p?hl=en#N6Qhr5kJSgQ/WebCore/loader/FrameLoader.cpp&q=ReturnCacheDataElseLoad&exact_package=git://android.git.kernel.org/platform/external/webkit.git&l=4688

Maybe the same should be done here, using the LOAD_FROM_CACHE nsIRequest flags on page loads when going back. That's what the documentation (see below) seems to assume if "browsing via history" == when going back/forward. But this flag isn't used when you go back (seems to be used for image loads somehow http://mxr.mozilla.org/mozilla-central/search?string=LOAD_FROM_CACHE).

    /**
     * Load from the cache, bypassing protocol specific validation logic.  This
     * flag is used when browsing via history.  It is not recommended for normal
     * browsing as it may likely violate reasonable assumptions made by the
     * server and confuse users.
     */
    const unsigned long LOAD_FROM_CACHE   = 1 << 10;

Comment 3

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #2)
> I'm not sure what you mean by "very normal" in your last sentence and in which situation it happens.

Because Expires: of cache is set to Epoc Time for the site, HTTP GET is issued always(irrelevant to existence of query in URL) upon Back/Forward if bfcache=off, unless user intentionally changes browser.cache.check_doc_frequency to 0 or 2.

> 
http://kb.mozillazine.org/Browser.cache.check_doc_frequency.
> Firefox's default=3 : Check for a new version when the page is out of date. 
>(Disk Cache Entry for the site)
> Key: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=1+week+ago&enddate=now
>(snip)
> Expires: 1970-01-01 09:00:00

Comment 4

[Sylvain Pasche]

Should be a Necko issue: see bug 468594 comment 11

Comment 5

[Bjarne (:bjarne)]

Attachment: Initial fix

Good catch. :)

Does this fix the issue? The idea is to let this particular case override the behaviour introduced by bug #468594.

Comment 6

[Sylvain Pasche]

Yes, that fixes the issue. Thanks.

Comment 7

[Sylvain Pasche]

by the way, it would be nice to update the unit test with this new change.

One way could be to have an additional optional loadFlags property on the test objects in the tests array which would be set on the channel in setupChannel().  Then it could be checked that passing the VALIDATE_NEVER flag makes the request use the cache.

Comment 8

[Bjarne (:bjarne)]

Attachment: Suggested fix w/ testcase

Please note that I'm not stating anything about bfcache or other usage of this flag, just acknowledging that the fix for bug #465954 introduced a regression. :)

Comment 9

[Sylvain Pasche]

Yeah, I think this is the safest thing to do (at least this brings us to the previous behavior).

I'm not pretending to do a review here, but I just wanted to share a few thoughts. I'm not sure duplicating the test code into a new one is the best option here, code duplication should be avoided whenever possible.

Personally I prefer to have the test file be named according to what it is testing instead of the bug number. When you look at the tests, you don't need go to bugzilla to see what this is about. I think test for bug 465954 and this one could be merged into a single test file named something like test_validation_caching.js (there's already a test_redirect_caching.js) for instance.

Comment 10

[Bjarne (:bjarne)]

Observe that the referenced bug in comment #9 and comment #8 is wrong...  it should of-course be bug #468594. My bad!

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 374741 [details] [diff] [review]
Suggested fix w/ testcase

You should do this also for the LOAD_FROM_CACHE case.

And at that point... wouldn't it be more elegant if you just added this as another else-if right before the last else? Combined with moving this into a function MustValidateBasedOnQuery or something.

Comment 12

[Bjarne (:bjarne)]

Attachment: Improved patch & testcase

(In reply to comment #11)
> (From update of attachment 374741 [details] [diff] [review])
> You should do this also for the LOAD_FROM_CACHE case.
> 
> And at that point... wouldn't it be more elegant if you just added this as
> another else-if right before the last else? Combined with moving this into a
> function MustValidateBasedOnQuery or something.

Fair enough - see new patch. :)

However, the updated test fails due to a strange phenomena : On the subsequent load *after* loading the resource with either flag set (i.e. the tests requesting '99'), mCacheAccess in CheckCache() is ACCESS_READ. This means that we get caught in the if-block commented "Don't bother to validate items that are...." (starting approximately at line 2030 in nsHttpChannel.cpp) and return from CheckCache() without even trying to validate the request.

Does the sequence of requests and expectations in the test make sense to you, or do I misunderstand something? My theory is that setting either of these flags somehow changes settings in the cache-entry, which comes to the surface next time we try to load the resource. I'll have a look at this tomorrow...

Comment 13

[Bjarne (:bjarne)]

Uhh... not exactly "tomorrow" but anyway...

Looks like we are granted read-only access to the cache entry because it is considered active. Not sure why this happens, though. Must investigate further.

Comment 14

[Bjarne (:bjarne)]

Attachment: Fixed unit-test

The reason for the funny behaviour was in the unit-test. See comment in CheckValueAndTrigger() for an explanation.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 379882 [details] [diff] [review]
Fixed unit-test

Sorry for the delay here.

+++ b/netwerk/test/unit/test_bug490095.js	Wed May 27 16:19:05 2009 +0200
+    channel.asyncOpen(new ChannelListener(checkValueAndTrigger, null),null);

should add a space after the comma

Comment 16

[Bjarne (:bjarne)]

Attachment: Added missing space

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 383230 [details] [diff] [review]
Added missing space

+    channel.asyncOpen(new ChannelListener(checkValueAndTrigger, null) ,null);

the space should have been after the comma :)

Comment 18

[Bjarne (:bjarne)]

Attachment: Corrected space-position

Ehhrmm....  :-}

Comment 19

[Bjarne (:bjarne)]

requesting check-in

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

http://hg.mozilla.org/mozilla-central/rev/1838dc6af85a

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)