Bug 153230 – Network cache: old pages returned by disk cache on history navigation after session is restored

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Since r181734, the network cache never revalidates resources for history navigation. This is good for the memory cache, but in the case of disk cache, we might end up with outdated pages when restoring the session. As anttik pointed out on IRC we would still want to not revalidate resources for history navigation after a web process crash, especially important on mobile, in which case the page and memory caches are lost. But when restoring the session at browser startup we need a way to ensure pages are revalidated.

[Chris Dumez]

This change was done on purpose. When restoring a session, one would expect to see the same content as last time. This is also beneficial to battery life.

[Chris Dumez]

Also on iOS, it would be difficult to distinguish Safari getting killed (due to memory constraints) from the browser being restarted.

[Carlos Garcia Campos]

(In reply to comment #1)
> This change was done on purpose. When restoring a session, one would expect
> to see the same content as last time. This is also beneficial to battery
> life.

Yes, I know it was on done purpose, but I disagree people expect to see old contents. That's probably what I expect when I open the browser in my mobile, but not in my desktop. I save the session to ensure I don't lose the tabs I had opened when I closed the browser, but I want to see the updated contents when I open the browser again the next day, for example. I was very surprised the first time it happened to me. At least for bugs I knew there were changes thanks to the bug mail.

[Michael Catanzaro]

Chris, are you open to adding an option for API users to choose the desired behavior here?

[Carlos Garcia Campos]

(In reply to comment #4)
> Chris, are you open to adding an option for API users to choose the desired
> behavior here?

It's not just a matter of adding API, we need a way to figure out when you are doing a history navigation of an items restored from the session or not. It's obvious in the case of the first navigation after a session restore, but then you could go back/forward on any webview and you don't know if the item you are going to was restored from the session or not.

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #4)
> > Chris, are you open to adding an option for API users to choose the desired
> > behavior here?
> 
> It's not just a matter of adding API, we need a way to figure out when you
> are doing a history navigation of an items restored from the session or not.
> It's obvious in the case of the first navigation after a session restore,
> but then you could go back/forward on any webview and you don't know if the
> item you are going to was restored from the session or not.

I personally do not understand the history navigation problem. On history navigation, we always returned potentially stale data. I don't think it matters if the history navigation happened after a session restore or not (or I misunderstood something).

About the Restoring from cache on session restore. we could support both behavior with a setting if needing (basically rolling out http://trac.webkit.org/changeset/181815 and making it conditional based on the new setting). To my knowledge, Safari 9 shipped with this behavior (session restore from the cache). There has not been much push back so far so I am not planning to go back to the previous behavior at this point. If there is push back though, we could restore the previous behavior on desktop.

[Michael Catanzaro]

(In reply to comment #6)
> I personally do not understand the history navigation problem. On history
> navigation, we always returned potentially stale data. I don't think it
> matters if the history navigation happened after a session restore or not
> (or I misunderstood something).

I think it's OK for pages to be stale when loaded with the back/forward list -- that's the whole point of the page cache -- but when launching the browsers users expect the pages to be refreshed. At least that's certainly what we expect for Epiphany; do Safari users really want to see old pages on startup?

Are we using session restore improperly? We have started using it to save the session when closing the browser, and restore it when opening the browser.

> About the Restoring from cache on session restore. we could support both
> behavior with a setting if needing (basically rolling out
> http://trac.webkit.org/changeset/181815 and making it conditional based on
> the new setting). To my knowledge, Safari 9 shipped with this behavior
> (session restore from the cache). There has not been much push back so far
> so I am not planning to go back to the previous behavior at this point. If
> there is push back though, we could restore the previous behavior on desktop.

Interesting... I suggest we go ahead and do this, especially since it looks like a fairly small amount of code to bring back.

[Alexey Proskuryakov]

> There has not been much push back so far so I am not planning to go back to the previous behavior at this point.

Hmm, I wonder if that's why I have a feeling that making pages load properly after relaunching Mac Safari has become more fiddly recently. I have to manually reload pages after each restart/reboot/SU to make them actually load.

It is quite hard to realize what's going on, so providing feedback is more complicated than usual.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Chris, are you open to adding an option for API users to choose the desired
> > > behavior here?
> > 
> > It's not just a matter of adding API, we need a way to figure out when you
> > are doing a history navigation of an items restored from the session or not.
> > It's obvious in the case of the first navigation after a session restore,
> > but then you could go back/forward on any webview and you don't know if the
> > item you are going to was restored from the session or not.
> 
> I personally do not understand the history navigation problem. On history
> navigation, we always returned potentially stale data. I don't think it
> matters if the history navigation happened after a session restore or not
> (or I misunderstood something).

The main difference is that for normal history navigation you are in the same session, so it's very unlikely that the page has changed, and you know you are visiting a page that you have recently loaded. In case of session restoring, the fact that we need to do a history navigation is actually an implementation detail, what the user does is just starting up the browser. The back forward list restored can be very old, so in that case when you navigate back, you expect to visit the same page, but definitely not the same contents (imagine you started your browser after several days).

> About the Restoring from cache on session restore. we could support both
> behavior with a setting if needing (basically rolling out
> http://trac.webkit.org/changeset/181815 and making it conditional based on
> the new setting). To my knowledge, Safari 9 shipped with this behavior
> (session restore from the cache). There has not been much push back so far
> so I am not planning to go back to the previous behavior at this point. If
> there is push back though, we could restore the previous behavior on desktop.

I think it's a good idea to keep the current behaviour when recovering from a web process crash, when we are still in the same session but we have lost the page and memory caches. So, maybe we could do something smarter here, and mark somehow the items when they are created by session restore, and unmark them when they are first loaded, so that the first time we always revalidate them and then we don't.

[Carlos Garcia Campos]

(In reply to comment #8)
> > There has not been much push back so far so I am not planning to go back to the previous behavior at this point.
> 
> Hmm, I wonder if that's why I have a feeling that making pages load properly
> after relaunching Mac Safari has become more fiddly recently. I have to
> manually reload pages after each restart/reboot/SU to make them actually
> load.

Yes, Antti told me the other day on IRC that ha had seen complains of Safari users about this new behavior.

> It is quite hard to realize what's going on, so providing feedback is more
> complicated than usual.

[Carlos Garcia Campos]

Created attachment 270655 [details]
Patch

This patch should fix the problem, without affecting session restore after a web process crash. If this behavior is still undesired for Mac/iOS we can just a way to query every port.

[Chris Dumez]

Comment on attachment 270655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270655&action=review

What I would suggest would be:
1. Revert http://trac.webkit.org/changeset/181815 so that our API has a "allowStaleContent" parameter again (which I find clearer that the approach in this patch)
2. Add a setting to decide in restoreSession() if we want to allow stale content or not.
3. Set this setting to True on iOS and False elsewhere.

> Source/WebCore/history/HistoryItem.h:206
> +    void setShouldRevalidateInDiskCacheOnly(bool revalidateInDiskCache) { m_shouldRevalidateInDiskCacheOnly = revalidateInDiskCache; }

I think the naming here is not very clear. Also, I am not sure why we need to add a data member to HistoryItem.

> Source/WebKit2/WebProcess/WebCoreSupport/SessionStateConversion.h:41
> +    APIRequest

I personally don't find this to be clear.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2187
> +    restoreSessionInternal(itemStates, SessionRestoreMode::APIRequest);

I think allowing stale content on session restore should be based on a setting. I know that we want to keep allowing stale content on iOS for example.

[Carlos Garcia Campos]

(In reply to comment #12)
> Comment on attachment 270655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270655&action=review
> 
> What I would suggest would be:
> 1. Revert http://trac.webkit.org/changeset/181815 so that our API has a
> "allowStaleContent" parameter again (which I find clearer that the approach
> in this patch)

The problem of that approach is that API needs to decide, but we don't know if the BF navigation we are doing is for an item restored from the session or not. Also, changing the frame load type not only affects the disk cache, it would mena that the page cache wouldn't be used no? If an item is restored from the session, but it's in the page or memory cache, we want to use that, not to go to the disk cache or network.

> 2. Add a setting to decide in restoreSession() if we want to allow stale
> content or not.
> 3. Set this setting to True on iOS and False elsewhere.
> 
> > Source/WebCore/history/HistoryItem.h:206
> > +    void setShouldRevalidateInDiskCacheOnly(bool revalidateInDiskCache) { m_shouldRevalidateInDiskCacheOnly = revalidateInDiskCache; }
> 
> I think the naming here is not very clear. Also, I am not sure why we need
> to add a data member to HistoryItem.

To know from the FrameLoader if the item we are going to load wants to use the disk cache unconditionally or not. We need to flag the item to know what to do when it's loaded, if you go to current item right after restoring the session, you know you don't want stale data from the disk cache, but if that session contained more BF items, and then you go back, you don't want stale data from the disk cache in that case either. If we flag the items when they are created in the web process, we have that info when they are loaded.

> > Source/WebKit2/WebProcess/WebCoreSupport/SessionStateConversion.h:41
> > +    APIRequest
> 
> I personally don't find this to be clear.

We can find better names.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2187
> > +    restoreSessionInternal(itemStates, SessionRestoreMode::APIRequest);
> 
> I think allowing stale content on session restore should be based on a
> setting. I know that we want to keep allowing stale content on iOS for
> example.

Ok, I can add a setting then, but if it's not something you want to change via API ans we are going to use the same values always for every port, maybe adding another setting is not the best approach. We can just have a method to query the behavior and add an implementation on every port.

[Carlos Garcia Campos]

Let me explain what I think it's the desired behavior here, at least for GTK:

 - For any BF navigation we always want stale data. If the page is in the page cache, that should always be used, if the page cache is disabled, then the memory cache, and if it's not in the memory cache, then stale data from the disk cache.

 - For a BF navigation of an item that was restored from the session as requested by the API, we always want stale data from the page cache or the memory cache, but not from the disk cache. So, it's considered a normal load only by the disk cache. This not only affects the BF navigation that happens right after restoring the session, but also to any other history item created by the session restore.

The reasoning. We use the session restore for 3 different features in epiphany:

 1 - To restore a previous session at startup. In this case, there won't be page cache nor memory cache, so we will always end up in the disk cache. We don't want stale data in this case, users expect the browser to show updated contents. Not expired  resources are of course revalidated and loaded from the cache in any case.

 2 - To restore recently closed tabs. In this case we want to use stale data from any cache. It's not a problem if the history item is marked as normal load, because unless there's a web process crash, the resources will be in the page cache or memory cache (it's a recently closed tab)

 3 - In ephy, when you open a new a link in a new tab or window, the bf list is inherited, so that you can go back to the page from which the link was clicked. In this case, as in the previous one, we always want stale data from any cache, but we expect the resources to be in the page or memory cache. 

In the very unlikely case of not having the resources in the page or memory cache in cases 2 and 3, we would be doing a unnecessary revalidation, for resources that will very likely not be expired, so we will end up loading from the disk cache anyway. The unnecessary revalidation in these cases wouldn't be a problem for us at all.

Even id this is based on the particular cases of the epiphany browser, I think this is the desired behavior of any of our API users. If anybody complains we can always add a setting to enable/disable this from the API, but I don't think that's going to happen. 

Hope it's clearer now.

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 270655 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=270655&action=review
> > 
> > What I would suggest would be:
> > 1. Revert http://trac.webkit.org/changeset/181815 so that our API has a
> > "allowStaleContent" parameter again (which I find clearer that the approach
> > in this patch)
> 
> The problem of that approach is that API needs to decide, but we don't know
> if the BF navigation we are doing is for an item restored from the session
> or not. Also, changing the frame load type not only affects the disk cache,
> it would mena that the page cache wouldn't be used no? If an item is
> restored from the session, but it's in the page or memory cache, we want to
> use that, not to go to the disk cache or network.

Are you trying to get a different behavior from the one before <http://trac.webkit.org/changeset/181815> then? I thought the idea was to restore that behavior.

The page cache only matters on actual BackForward navigation. In this case, we are talking about changing the load type only in the case of session restoration. Therefore, I don't think we should worry about the PageCache (or memory cache for that matter). In any case, your issue seems to be with the behavior change introduced in r181815 so I would suggest going back to that behavior on non-iOS platforms before attempting further behavior changes.

> 
> > 2. Add a setting to decide in restoreSession() if we want to allow stale
> > content or not.
> > 3. Set this setting to True on iOS and False elsewhere.
> > 
> > > Source/WebCore/history/HistoryItem.h:206
> > > +    void setShouldRevalidateInDiskCacheOnly(bool revalidateInDiskCache) { m_shouldRevalidateInDiskCacheOnly = revalidateInDiskCache; }
> > 
> > I think the naming here is not very clear. Also, I am not sure why we need
> > to add a data member to HistoryItem.
> 
> To know from the FrameLoader if the item we are going to load wants to use
> the disk cache unconditionally or not. We need to flag the item to know what
> to do when it's loaded, if you go to current item right after restoring the
> session, you know you don't want stale data from the disk cache, but if that
> session contained more BF items, and then you go back, you don't want stale
> data from the disk cache in that case either. If we flag the items when they
> are created in the web process, we have that info when they are loaded.
> 
> > > Source/WebKit2/WebProcess/WebCoreSupport/SessionStateConversion.h:41
> > > +    APIRequest
> > 
> > I personally don't find this to be clear.
> 
> We can find better names.
> 
> > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2187
> > > +    restoreSessionInternal(itemStates, SessionRestoreMode::APIRequest);
> > 
> > I think allowing stale content on session restore should be based on a
> > setting. I know that we want to keep allowing stale content on iOS for
> > example.
> 
> Ok, I can add a setting then, but if it's not something you want to change
> via API ans we are going to use the same values always for every port, maybe
> adding another setting is not the best approach. We can just have a method
> to query the behavior and add an implementation on every port.

I don't know what's the best policy here. Maybe Sam can pitch in.
No matter how, I'd like to maintain the current behavior for iOS.

[Carlos Garcia Campos]

(In reply to comment #15)
> 
> I don't know what's the best policy here. Maybe Sam can pitch in.
> No matter how, I'd like to maintain the current behavior for iOS.

Sure, that's not a problem

[Chris Dumez]

(In reply to comment #14)
> Let me explain what I think it's the desired behavior here, at least for GTK:
> 
>  - For any BF navigation we always want stale data. If the page is in the
> page cache, that should always be used, if the page cache is disabled, then
> the memory cache, and if it's not in the memory cache, then stale data from
> the disk cache.
> 
>  - For a BF navigation of an item that was restored from the session as
> requested by the API, we always want stale data from the page cache or the
> memory cache, but not from the disk cache. So, it's considered a normal load
> only by the disk cache. This not only affects the BF navigation that happens
> right after restoring the session, but also to any other history item
> created by the session restore.

I don't think it is ever possible currently to have a PageCache entry for a HistoryItem after a session restore. On session restore, you essentially get a new HistoryItem and PageCache entries are associated to a specific HistoryItem.

Regarding the MemoryCache, I don't think there would be anything either, considering the memory cache is per WebProcess. On session restore, you get fresh tabs and normally new WebProcesses AFAIK.

> 
> The reasoning. We use the session restore for 3 different features in
> epiphany:
> 
>  1 - To restore a previous session at startup. In this case, there won't be
> page cache nor memory cache, so we will always end up in the disk cache. We
> don't want stale data in this case, users expect the browser to show updated
> contents. Not expired  resources are of course revalidated and loaded from
> the cache in any case.
> 
>  2 - To restore recently closed tabs. In this case we want to use stale data
> from any cache. It's not a problem if the history item is marked as normal
> load, because unless there's a web process crash, the resources will be in
> the page cache or memory cache (it's a recently closed tab)
> 
>  3 - In ephy, when you open a new a link in a new tab or window, the bf list
> is inherited, so that you can go back to the page from which the link was
> clicked. In this case, as in the previous one, we always want stale data
> from any cache, but we expect the resources to be in the page or memory
> cache. 
> 
> In the very unlikely case of not having the resources in the page or memory
> cache in cases 2 and 3, we would be doing a unnecessary revalidation, for
> resources that will very likely not be expired, so we will end up loading
> from the disk cache anyway. The unnecessary revalidation in these cases
> wouldn't be a problem for us at all.
> 
> Even id this is based on the particular cases of the epiphany browser, I
> think this is the desired behavior of any of our API users. If anybody
> complains we can always add a setting to enable/disable this from the API,
> but I don't think that's going to happen. 
> 
> Hope it's clearer now.

[Carlos Garcia Campos]

(In reply to comment #17)
> (In reply to comment #14)
> > Let me explain what I think it's the desired behavior here, at least for GTK:
> > 
> >  - For any BF navigation we always want stale data. If the page is in the
> > page cache, that should always be used, if the page cache is disabled, then
> > the memory cache, and if it's not in the memory cache, then stale data from
> > the disk cache.
> > 
> >  - For a BF navigation of an item that was restored from the session as
> > requested by the API, we always want stale data from the page cache or the
> > memory cache, but not from the disk cache. So, it's considered a normal load
> > only by the disk cache. This not only affects the BF navigation that happens
> > right after restoring the session, but also to any other history item
> > created by the session restore.
> 
> I don't think it is ever possible currently to have a PageCache entry for a
> HistoryItem after a session restore. On session restore, you essentially get
> a new HistoryItem and PageCache entries are associated to a specific
> HistoryItem.

Aha, good point.

> Regarding the MemoryCache, I don't think there would be anything either,
> considering the memory cache is per WebProcess. On session restore, you get
> fresh tabs and normally new WebProcesses AFAIK.

Unless you have a process limit of 1, but yes I get your point.

[Carlos Garcia Campos]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #14)
> > > Let me explain what I think it's the desired behavior here, at least for GTK:
> > > 
> > >  - For any BF navigation we always want stale data. If the page is in the
> > > page cache, that should always be used, if the page cache is disabled, then
> > > the memory cache, and if it's not in the memory cache, then stale data from
> > > the disk cache.
> > > 
> > >  - For a BF navigation of an item that was restored from the session as
> > > requested by the API, we always want stale data from the page cache or the
> > > memory cache, but not from the disk cache. So, it's considered a normal load
> > > only by the disk cache. This not only affects the BF navigation that happens
> > > right after restoring the session, but also to any other history item
> > > created by the session restore.
> > 
> > I don't think it is ever possible currently to have a PageCache entry for a
> > HistoryItem after a session restore. On session restore, you essentially get
> > a new HistoryItem and PageCache entries are associated to a specific
> > HistoryItem.
> 
> Aha, good point.

I remember now, the thing is that whether it was restored from the session only matters for the first time it's loaded. So, if you restore a page from the session, then you navigate, and then go back, the page is now in the page cache and you want to use it now. If we change the load type, we just need to unflag the history item after navigating to it.

> > Regarding the MemoryCache, I don't think there would be anything either,
> > considering the memory cache is per WebProcess. On session restore, you get
> > fresh tabs and normally new WebProcesses AFAIK.
> 
> Unless you have a process limit of 1, but yes I get your point.

Same here, in this case it would only happen if the page cache is disabled.

[Carlos Garcia Campos]

Another problem of converting the load type to standard for bf navigation after a session restore is that it adds a new entry to the bf list and you ends up with duplicated entries. So, if we don't want to flag the history item, we could add a new frame load type, something like FrameLoadType::SessionRestoredBackForward.

[Carlos Garcia Campos]

Created attachment 270741 [details]
A different approach

This is a hopefully simpler approach.

[Carlos Garcia Campos]

Created attachment 270743 [details]
Try to fix mac builds

[Chris Dumez]

Comment on attachment 270743 [details]
Try to fix mac builds

View in context: https://bugs.webkit.org/attachment.cgi?id=270743&action=review

I think Sam or Anders need to review this.

> Source/WebCore/ChangeLog:11
> +        policy cache to ensure it's revalidated by the disk cache if needed.

I would say "to ensure we do not use stale cached data in this case". As this seems to be the intent.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1199
> +    // iOS always want stale data for history navigation.

'wants'.

I don't quite get this comment because it is not just iOS, so does everyone else. The only case some platforms do not want stale data is for session restore (Which used to be considered as a history navigation but does not really have to).

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1218
> +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::Forward));

So we will use SessionRestoredBackForward on iOS even though this has nothing to do with session restore? This seems wrong.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1233
> +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::Back));

ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1248
> +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::IndexedBackForward));

ditto, this is not only used for session restore.

[Carlos Garcia Campos]

(In reply to comment #23)
> Comment on attachment 270743 [details]
> Try to fix mac builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270743&action=review
> 
> I think Sam or Anders need to review this.
> 
> > Source/WebCore/ChangeLog:11
> > +        policy cache to ensure it's revalidated by the disk cache if needed.
> 
> I would say "to ensure we do not use stale cached data in this case". As
> this seems to be the intent.

Not really, we might still use the cached data if the revalidation is not needed, which is what happens very often. If you close the browser, and then open it, most of the resources are used from the disk cache without revalidation (responseNeedsRevalidation returns false).

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1199
> > +    // iOS always want stale data for history navigation.
> 
> 'wants'.
> 
> I don't quite get this comment because it is not just iOS, so does everyone
> else. The only case some platforms do not want stale data is for session
> restore (Which used to be considered as a history navigation but does not
> really have to).

Well, that's what I meant with "always", but I agree it's not clear enough. When a session is restored, the back forward list is also restored, and it contains the current item as well. So, if you go back/forward after a session restore, it's indeed a history navigation. And the fact that we do a history navigation for the current item too is because it's also restored in the back forward list, if we do a normal navigation we duplicate the entry in the bf list, so going back after session restore would load the same page again.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1218
> > +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::Forward));
> 
> So we will use SessionRestoredBackForward on iOS even though this has
> nothing to do with session restore? This seems wrong.

No, on iOS frameLoadTypeForBackForwardItem always returns the proposed type, FrameLoadType::Forward in this case, so this patch doesn't affect iOS at all. For other ports SessionRestoredBackForward is only returned for history items created for a session restore. So, this has to do with session restore if the item we are about to navigate to was created from the session data.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1233
> > +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::Back));
> 
> ditto.

iOS will use FrameLoadType::Back here, not SessionRestoredBackForward.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1248
> > +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::IndexedBackForward));
> 
> ditto, this is not only used for session restore.

This is not about restoring the session, but about navigation to a history item that has been restored from the session. If we only care about the actual navigation that happens right after the session restore, we would show updated contents for the current page, but if then you go back, you could see very old contents again.

[Chris Dumez]

(In reply to comment #24)
> (In reply to comment #23)
> > Comment on attachment 270743 [details]
> > Try to fix mac builds
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=270743&action=review
> > 
> > I think Sam or Anders need to review this.
> > 
> > > Source/WebCore/ChangeLog:11
> > > +        policy cache to ensure it's revalidated by the disk cache if needed.
> > 
> > I would say "to ensure we do not use stale cached data in this case". As
> > this seems to be the intent.
> 
> Not really, we might still use the cached data if the revalidation is not
> needed, which is what happens very often. If you close the browser, and then
> open it, most of the resources are used from the disk cache without
> revalidation (responseNeedsRevalidation returns false).

I know we still want to use cached data if it is fresh ( does not need revalidation or revalidation succeeded). I still think my sentence is correct. Stale != cached. Stale data is data that has expired and is no longer fresh.

> > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1199
> > > +    // iOS always want stale data for history navigation.
> > 
> > 'wants'.
> > 
> > I don't quite get this comment because it is not just iOS, so does everyone
> > else. The only case some platforms do not want stale data is for session
> > restore (Which used to be considered as a history navigation but does not
> > really have to).
> 
> Well, that's what I meant with "always", but I agree it's not clear enough.
> When a session is restored, the back forward list is also restored, and it
> contains the current item as well. So, if you go back/forward after a
> session restore, it's indeed a history navigation. And the fact that we do a
> history navigation for the current item too is because it's also restored in
> the back forward list, if we do a normal navigation we duplicate the entry
> in the bf list, so going back after session restore would load the same page
> again.
> 
> > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1218
> > > +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::Forward));
> > 
> > So we will use SessionRestoredBackForward on iOS even though this has
> > nothing to do with session restore? This seems wrong.
> 
> No, on iOS frameLoadTypeForBackForwardItem always returns the proposed type,
> FrameLoadType::Forward in this case, so this patch doesn't affect iOS at
> all. For other ports SessionRestoredBackForward is only returned for history
> items created for a session restore. So, this has to do with session restore
> if the item we are about to navigate to was created from the session data.
> 
> > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1233
> > > +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::Back));
> > 
> > ditto.
> 
> iOS will use FrameLoadType::Back here, not SessionRestoredBackForward.
> 
> > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1248
> > > +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::IndexedBackForward));
> > 
> > ditto, this is not only used for session restore.
> 
> This is not about restoring the session, but about navigation to a history
> item that has been restored from the session. If we only care about the
> actual navigation that happens right after the session restore, we would
> show updated contents for the current page, but if then you go back, you
> could see very old contents again.

[Michael Catanzaro]

Although this behavior frustrated me at first, the advantage is that it makes pages load *really fast* when starting the browser. I noticed that I quite like Chrome's setting "When closing the browser... pick up where I left off" which works like what we have now.

Probably we should expose this as an option in our API.

[Carlos Garcia Campos]

(In reply to comment #26)
> Although this behavior frustrated me at first, the advantage is that it
> makes pages load *really fast* when starting the browser.

What's the advantage of loading very fast if contents are outdated? You have to manually load again to get the current contents, which is not only slower than revalidating the resources, but also very confusing. It happens to me every day, I have to manually reload all the bugzilla tabs I have opened to see new comments/reviews and post my comments. Fortunately I know there are new contents thanks to the bug mail.

> I noticed that I
> quite like Chrome's setting "When closing the browser... pick up where I
> left off" which works like what we have now.
> 
> Probably we should expose this as an option in our API.

I prefer to avoid adding an API option for this if possible.

[Michael Catanzaro]

OK, I agree at any rate that it's more important to avoid stale contents by default.

Let's ask Sam or Anders to review on IRC; we need this fixed soon for the next Epiphany release.

[Chris Dumez]

Comment on attachment 270743 [details]
Try to fix mac builds

View in context: https://bugs.webkit.org/attachment.cgi?id=270743&action=review

>>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1218
>>> +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::Forward));
>> 
>> So we will use SessionRestoredBackForward on iOS even though this has nothing to do with session restore? This seems wrong.
> 
> No, on iOS frameLoadTypeForBackForwardItem always returns the proposed type, FrameLoadType::Forward in this case, so this patch doesn't affect iOS at all. For other ports SessionRestoredBackForward is only returned for history items created for a session restore. So, this has to do with session restore if the item we are about to navigate to was created from the session data.

Right, I misread the function, sorry about that.

[Chris Dumez]

(In reply to comment #27)
> (In reply to comment #26)
> > Although this behavior frustrated me at first, the advantage is that it
> > makes pages load *really fast* when starting the browser.
> 
> What's the advantage of loading very fast if contents are outdated? You have
> to manually load again to get the current contents, which is not only slower
> than revalidating the resources, but also very confusing. It happens to me
> every day, I have to manually reload all the bugzilla tabs I have opened to
> see new comments/reviews and post my comments. Fortunately I know there are
> new contents thanks to the bug mail.

There are several advantages:
1. People often have tabs open they are no longer interested about. Doing network activity for them is wasteful (network, battery, startup time...)
2. Some people may have been reading a particular article on a page and may be surprised to get different content than the last time their browser was open (because we loaded fresh content from the web).
3. With the current behavior, users can reload only the tabs they are interested in getting fresh content for.

Your statement 'reloading' not revalidating the content seems inaccurate to me. Unless the client specifically requests a reload *from origin*, the regular reload merely revalidates cached data.

[Carlos Garcia Campos]

(In reply to comment #29)
> Comment on attachment 270743 [details]
> Try to fix mac builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270743&action=review
> 
> >>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1218
> >>> +    m_page->goToItem(*item, frameLoadTypeForBackForwardItem(backForwardItemID, FrameLoadType::Forward));
> >> 
> >> So we will use SessionRestoredBackForward on iOS even though this has nothing to do with session restore? This seems wrong.
> > 
> > No, on iOS frameLoadTypeForBackForwardItem always returns the proposed type, FrameLoadType::Forward in this case, so this patch doesn't affect iOS at all. For other ports SessionRestoredBackForward is only returned for history items created for a session restore. So, this has to do with session restore if the item we are about to navigate to was created from the session data.
> 
> Right, I misread the function, sorry about that.

No problem, I supposed it.

[Carlos Garcia Campos]

(In reply to comment #30)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > Although this behavior frustrated me at first, the advantage is that it
> > > makes pages load *really fast* when starting the browser.
> > 
> > What's the advantage of loading very fast if contents are outdated? You have
> > to manually load again to get the current contents, which is not only slower
> > than revalidating the resources, but also very confusing. It happens to me
> > every day, I have to manually reload all the bugzilla tabs I have opened to
> > see new comments/reviews and post my comments. Fortunately I know there are
> > new contents thanks to the bug mail.
> 
> There are several advantages:
> 1. People often have tabs open they are no longer interested about. Doing
> network activity for them is wasteful (network, battery, startup time...)
> 2. Some people may have been reading a particular article on a page and may
> be surprised to get different content than the last time their browser was
> open (because we loaded fresh content from the web).
> 3. With the current behavior, users can reload only the tabs they are
> interested in getting fresh content for.

I agree all this is probably what I expect in my mobile phone where the concept of starting/closing applications is not exactly the same than in the desktop. If you want it, I can make this behavior specific to GTK+, instead of making the exception for iOS. Then we can discuss whether Mac users want this or not, and whether to expose a setting to enable/disable it.

> Your statement 'reloading' not revalidating the content seems inaccurate to
> me. Unless the client specifically requests a reload *from origin*, the
> regular reload merely revalidates cached data.

Sure, you are right.

[Chris Dumez]

Comment on attachment 270743 [details]
Try to fix mac builds

View in context: https://bugs.webkit.org/attachment.cgi?id=270743&action=review

> Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:52
> +    bool restoredFromSession;

A thought just occurred to me. Don't we need to reset this flag at some point? Otherwise, it seems like we would *never* again use the back/forward cache policy to these history items (unless the HistoryItems get regenerated somehow?).

[Carlos Garcia Campos]

(In reply to comment #33)
> Comment on attachment 270743 [details]
> Try to fix mac builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270743&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:52
> > +    bool restoredFromSession;
> 
> A thought just occurred to me. Don't we need to reset this flag at some
> point? Otherwise, it seems like we would *never* again use the back/forward
> cache policy to these history items (unless the HistoryItems get regenerated
> somehow?).

That was my first idea, but it was not easy to decide when to reset it, and I realized that it was not actually needed, because after the first time, the resource will always hit the page cache or the memory cache if the page cache is disabled, but we won't reach the disk cache again.

[Chris Dumez]

(In reply to comment #34)
> (In reply to comment #33)
> > Comment on attachment 270743 [details]
> > Try to fix mac builds
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=270743&action=review
> > 
> > > Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:52
> > > +    bool restoredFromSession;
> > 
> > A thought just occurred to me. Don't we need to reset this flag at some
> > point? Otherwise, it seems like we would *never* again use the back/forward
> > cache policy to these history items (unless the HistoryItems get regenerated
> > somehow?).
> 
> That was my first idea, but it was not easy to decide when to reset it, and
> I realized that it was not actually needed, because after the first time,
> the resource will always hit the page cache or the memory cache if the page
> cache is disabled, but we won't reach the disk cache again.

It *may* use the PageCache or the memory cache but there is definitely no guarantee of this happening AFAIK. Relying on this seems wrong.

[Carlos Garcia Campos]

(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > Comment on attachment 270743 [details]
> > > Try to fix mac builds
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=270743&action=review
> > > 
> > > > Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:52
> > > > +    bool restoredFromSession;
> > > 
> > > A thought just occurred to me. Don't we need to reset this flag at some
> > > point? Otherwise, it seems like we would *never* again use the back/forward
> > > cache policy to these history items (unless the HistoryItems get regenerated
> > > somehow?).
> > 
> > That was my first idea, but it was not easy to decide when to reset it, and
> > I realized that it was not actually needed, because after the first time,
> > the resource will always hit the page cache or the memory cache if the page
> > cache is disabled, but we won't reach the disk cache again.
> 
> It *may* use the PageCache or the memory cache but there is definitely no
> guarantee of this happening AFAIK. Relying on this seems wrong.

Ok, I assumed it was correct, I'll update the patch to reset the value then. Do you know in which case we would en up in the disk cache again?

[Chris Dumez]

(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > (In reply to comment #33)
> > > > Comment on attachment 270743 [details]
> > > > Try to fix mac builds
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=270743&action=review
> > > > 
> > > > > Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:52
> > > > > +    bool restoredFromSession;
> > > > 
> > > > A thought just occurred to me. Don't we need to reset this flag at some
> > > > point? Otherwise, it seems like we would *never* again use the back/forward
> > > > cache policy to these history items (unless the HistoryItems get regenerated
> > > > somehow?).
> > > 
> > > That was my first idea, but it was not easy to decide when to reset it, and
> > > I realized that it was not actually needed, because after the first time,
> > > the resource will always hit the page cache or the memory cache if the page
> > > cache is disabled, but we won't reach the disk cache again.
> > 
> > It *may* use the PageCache or the memory cache but there is definitely no
> > guarantee of this happening AFAIK. Relying on this seems wrong.
> 
> Ok, I assumed it was correct, I'll update the patch to reset the value then.
> Do you know in which case we would en up in the disk cache again?

On memory pressure, we clear both the PageCache and the memory cache for example. These are caches and they get pruned / cleared so you cannot expect something specific to be in there for sure.

[Michael Catanzaro]

So, huge thread, what still needs to happen here?

Context for Apple folks: we need to land this soon to avoid reverting the session state work in Epiphany before our release next month.

[Carlos Garcia Campos]

I just need to find the time to update then patch to unflag the history item as loaded by session restore as I discussed with Chris. I would like to know also if Apple wants this behavior for non iOS too, because otherwise I'll change the condition to make this GTK specific.

[Chris Dumez]

(In reply to comment #39)
> I just need to find the time to update then patch to unflag the history item
> as loaded by session restore as I discussed with Chris. I would like to know
> also if Apple wants this behavior for non iOS too, because otherwise I'll
> change the condition to make this GTK specific.

I don't have strong feelings either way. I personally like the current behavior and we haven't received many (any?) complaints since this shipped in Safari 9.

However, since it seems some people here get frustrated with the new behavior I'd say we should probably be conservative and restore the old behavior on non-iOS as well, to be safe.

[Carlos Garcia Campos]

Created attachment 271879 [details]
Updated patch

Addressed review comments, and also reset the restoredFromSession flags after the first load.

[Carlos Garcia Campos]

Any other concern about this Chris? We really need a review here since we are preparing a new major release.

[Chris Dumez]

Comment on attachment 271879 [details]
Updated patch

Sorry, still not a huge fan of the complexity this adds. This seems intrusive and a bit complicated for what it does.

Would the following work?
1. We don't add a new load type (we already have too many)
2. We had a boolean flag to WebCore::HistoryItem to distinguish that the HistoryItem was restored from a previous session
3. We check this new WebCore::HistoryItem flag in FrameLoader::loadDifferentDocumentItem() to choose the right cache policy without requiring a new load type
4. We reset this flag right after it is read in FrameLoader::loadDifferentDocumentItem().

Let me know what you think.

[Carlos Garcia Campos]

(In reply to comment #43)
> Comment on attachment 271879 [details]
> Updated patch
> 
> Sorry, still not a huge fan of the complexity this adds. This seems
> intrusive and a bit complicated for what it does.
> 
> Would the following work?
> 1. We don't add a new load type (we already have too many)
> 2. We had a boolean flag to WebCore::HistoryItem to distinguish that the
> HistoryItem was restored from a previous session
> 3. We check this new WebCore::HistoryItem flag in
> FrameLoader::loadDifferentDocumentItem() to choose the right cache policy
> without requiring a new load type
> 4. We reset this flag right after it is read in
> FrameLoader::loadDifferentDocumentItem().
> 
> Let me know what you think.

This is pretty much what my first patch did :-) 

https://bug-153230-attachments.webkit.org/attachment.cgi?id=270655

I changed the approach because you were not sure about the need to add a member to HistoryItem, and I didn't like it either. See comment #12 :-)

But yes, I can go back to the initial approach, just with better names, with special case for iOS and resetting the flag. The patch was indeed a lot simpler.

[Carlos Garcia Campos]

Created attachment 272315 [details]
Go back to initial approach

[Chris Dumez]

Comment on attachment 272315 [details]
Go back to initial approach

View in context: https://bugs.webkit.org/attachment.cgi?id=272315&action=review

r=me with changes.

> Source/WebCore/history/HistoryItem.h:262
> +    // Item was created from a session restore.

I don't think this comment is useful.

> Source/WebCore/history/HistoryItem.h:263
> +    bool m_restoredFromSession { false };

we need a prefix for boolean members, I suggest m_wasRestoredFromSession.

Also, could you please move this new member next to the other boolean members above?

> Source/WebCore/loader/FrameLoader.cpp:3340
> +            if (!item.wasRestoredFromSession())

I would do the #ifdef iOS logic here.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2234
> +void WebPage::restoreSessionInternal(const Vector<BackForwardListItemState>& itemStates, bool restoredByAPIRequest)

Do we really need a new method? Maybe we could just add the extra parameter to restoreSession() ?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2238
> +#if !PLATFORM(IOS)

I would prefer if we moved the IOS ifdefing to FrameLoader::loadDifferentDocumentItem() and set the wasRestoredFromSession flag here no matter the platform. Otherwise, we end up with a boolean that has an inaccurate value on iOS.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2239
> +        // iOS always wants stale data after session restore.

s/wants/allows

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2248
> +    restoreSessionInternal(itemStates, true);

Could you please make the boolean argument an enum for readability? e.g.
enum class WasRestoredByAPIRequest { No, Yes };

[Carlos Garcia Campos]

(In reply to comment #46)
> Comment on attachment 272315 [details]
> Go back to initial approach
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272315&action=review
> 
> r=me with changes.

Thank you very much!

> > Source/WebCore/history/HistoryItem.h:262
> > +    // Item was created from a session restore.
> 
> I don't think this comment is useful.

Ok.

> > Source/WebCore/history/HistoryItem.h:263
> > +    bool m_restoredFromSession { false };
> 
> we need a prefix for boolean members, I suggest m_wasRestoredFromSession.

Ok.

> Also, could you please move this new member next to the other boolean
> members above?

Sure.

> > Source/WebCore/loader/FrameLoader.cpp:3340
> > +            if (!item.wasRestoredFromSession())
> 
> I would do the #ifdef iOS logic here.

Ok.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2234
> > +void WebPage::restoreSessionInternal(const Vector<BackForwardListItemState>& itemStates, bool restoredByAPIRequest)
> 
> Do we really need a new method? Maybe we could just add the extra parameter
> to restoreSession() ?

Yes, because it's a message handler called from generated code.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2238
> > +#if !PLATFORM(IOS)
> 
> I would prefer if we moved the IOS ifdefing to
> FrameLoader::loadDifferentDocumentItem() and set the wasRestoredFromSession
> flag here no matter the platform. Otherwise, we end up with a boolean that
> has an inaccurate value on iOS.

Makes sense.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2239
> > +        // iOS always wants stale data after session restore.
> 
> s/wants/allows

Ok.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2248
> > +    restoreSessionInternal(itemStates, true);
> 
> Could you please make the boolean argument an enum for readability? e.g.
> enum class WasRestoredByAPIRequest { No, Yes };

Sure, my original patch did that indeed, but I was not happy with the names I used.

[Carlos Garcia Campos]

Created attachment 272411 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r197244: <http://trac.webkit.org/changeset/197244>

[Sam Weinig]

I would rather this was done through a Setting rather than an #ifdef.  In addition, it would be really good for this to have a test case covering both options.

[Chris Dumez]

(In reply to comment #50)
> I would rather this was done through a Setting rather than an #ifdef.  In
> addition, it would be really good for this to have a test case covering both
> options.

Carlos, do you have spare cycles to make this change?

[Carlos Garcia Campos]

(In reply to comment #51)
> (In reply to comment #50)
> > I would rather this was done through a Setting rather than an #ifdef.  In
> > addition, it would be really good for this to have a test case covering both
> > options.
> 
> Carlos, do you have spare cycles to make this change?

Yes, I also thought it would be better to have a setting when you decided to use the same behavior for mac. I'm not sure about the test, though, since I don't think it's possible to test session restore from layout tests. I'll convert it to a setting for now.