GNOME Bugzilla – Bug 763184
Stale snapshots persist forever in overview
Last modified: 2016-05-09 13:40:16 UTC
We have in ephy-snapshot-service.c SNAPSHOT_UPDATE_THRESHOLD which implies that we refresh snapshots after one week, but it doesn't work. I'm not sure how our snapshots ever actually get refreshed. We have comparisons like this in ephy-snapshot-service.c: if (uri && current_time - mtime <= SNAPSHOT_UPDATE_THRESHOLD) Where current_time is the current time, and mtime is also the current time, so no chance to take new snapshots there. And we also have in web_view_check_snapshot, in ephy-web-view.c: if (ephy_snapshot_service_lookup_snapshot_path (service, url)) return FALSE; Which means we never take a new snapshot if a snapshot already exists for this url, regardless of age.
(In reply to Michael Catanzaro from comment #0) > We have comparisons like this in ephy-snapshot-service.c: > > if (uri && current_time - mtime <= SNAPSHOT_UPDATE_THRESHOLD) > > Where current_time is the current time, and mtime is also the current time, > so no chance to take new snapshots there. This was introduced in bug #725393.
The following fix has been pushed: fd68604 Revert "snapshot-service: Update snapshots after one week"
Created attachment 324983 [details] [review] Revert "snapshot-service: Update snapshots after one week" This reverts commit 80ba0ff169d472888389589b143407d56898ea62. This code has been broken since it was introduced. There are two cases where we use the snapshot service: * From EphyAboutHandler. Here we pass the a time taken from EphyHistoryService for the mtime. This check does not seem useful in this case, because it is only a query: it can never result in a new snapshot being taken to replace the old snapshot. * From EphyWebView itself, when a new page is loaded (or, until recently, when a favicon was acquired for that page). This is the only case in which a new snapshot can ever be taken (or updated). In this case, we pass the current time for mtime, then the comparison current_time - mtime <= SNAPSHOT_UPDATE_THRESHOLD is the same as if (0 <= SNAPSHOT_UPDATE_THRESHOLD), and so the snapshots always pass the check and are never expired. Ideally we would reimplement this properly, since one week seems like a good amount of time for which to persist snapshots. But this is far from straightforward to do. For now, let's just remove this code, causing snapshots to be updated at most once per browsing session. This mitigates the bug where, until recently, we would take snapshots of completely white web views, before the pages had rendered; even though we no longer take white snapshots, broken snapshots that were taken previously currently persist forever until ~/.cache/thumbnails is cleared somehow (e.g. by gnome-settings-daemon's housekeeping plugin). Now they will be cleared much sooner.
Nope, that's not enough; clearly it just removes a check that was always met anyway. It just happened that the first site I tested it on got a snapshot update by chance. :(
https://build.webkit.org/dashboard/ is an awesome test page for this; you can change the ports that appear on the dashboard to make obvious changes to the page, to check if the snapshot is updated in the overview.
Created attachment 324985 [details] [review] snapshot-service: Take new snapshots after a restart The previous commit was correct, but the commit message was not. It stated that removing the erroneous check would cause snapshots to be taken if the stored snapshot is too old. In fact, removing the check did not affect behavior at all -- the check was useless -- and so that did nothing to fix our stale snapshot issue. If the snapshot path is not in memory cache, this is an indication that the snapshot is relatively stale (older than the current browsing session) and should be taken again. But we cannot use the existing snapshot path memory cache as-is, because it is populated not when snapshots are taken, but when snapshots are looked up. Augment the cache to remember which paths were snapshotted since Epiphany was last restarted, and check it to determine whether we're due for a snapshot of the current page. Also, remove use of the cache from EphyWebView, since that would prevent snapshots from ever being taken. This really fixes the issue.
(In reply to Michael Catanzaro from comment #6) > Also, remove use of the cache from EphyWebView, since > that would prevent snapshots from ever being taken. To be clear: the current code prevents snapshots from ever being taken for pages in the overview after the overview is displayed for the first time (as displaying the overview causes these pages' snapshot paths to enter the memory cache).
Attachment 324985 [details] pushed as 9754735 - snapshot-service: Take new snapshots after a restart
Need to immediately return the OLD snapshot, and only schedule taking a new snapshot for future use, as snapshots are taken best-effort and regularly fail. We need to show the old snapshot rather than display a placeholder when this happens. This is an old bug, but it used to be extremely rare; unfortunately it's become obvious now that snapshots are refreshed regularly.
Well that was a good guess, and we should probably do that anyway, but not the problem I'm seeing. In gnome_desktop_thumbnail_is_valid in gnome-desktop-thumbnail.c, we hit this return: if (mtime != thumb_mtime) return FALSE; In my case today, 1461548112 != 1461548089. Crap, so the file's mtime is 13 milliseconds older than the mtime embedded in the PNG, and the thumbnail gets dropped from the overview. Still investigating.
(In reply to Michael Catanzaro from comment #10) > so the file's mtime is 13 milliseconds older than the mtime embedded in the PNG Ah, it's note the mtime of the file, it's the mtime of when we saved the page in history, from the history service, which of course can be a bit off of the actual mtime saved by gnome-desktop. We never noticed before because the snapshots were never refreshed ever, so once it worked once for a particular page, then that page would be good forever. But now it can break at any time. Since gnome-desktop requires we pass the EXACT mtime, because it expects us to be thumbnailing files on disk, it's hard to think of what to do about this, except adding API to gnome-desktop to avoid the mtime check (since we don't want that in Epiphany, it just gets in our way). This is quite unfortunate. Possibly would be better to bite the bullet and copy the portion of the code we need out of gnome-desktop; Epiphany is not a thumbnailer, after all.
Ugh, wrong again; sorry for the unnecessary CCs. Maybe I need some waiting period between thinking and posting a comment on Bugzilla. :) There is never any comparison to the actual mtime on disk, only to the mtime embedded in the PNG, which is controlled by Epiphany. The problem is that we just pass the current time when saving the snapshot; instead we just need to pass the time saved in history. So no need to modify gnome-desktop.
Initial patches... not ready to commit because: (a) Haven't thought about what happens if the same URL is loaded simultaneously in multiple web views (b) The mtime comparison in gnome_desktop_thumbnail_is_valid is still broken for some reason, e.g. even with these patches I just saw mtime 1461557996 != thumb_mtime 1461557948. Requires further investigation.
Created attachment 326649 [details] [review] snapshot-service: Always return snapshots immediately if available Return a stale snapshot, then schedule creation of a new snapshot. This way, we show a preexisting snapshot even if snapshot creation fails. The new snapshot will be used only for future requests.
Created attachment 326650 [details] [review] snapshot-service: Stop using const time_t We don't use const in C except for strings. And certainly not for pass-by-value.
Created attachment 326651 [details] [review] Always pass correct mtime to snapshot service The mtime we pass to the snapshot service when saving the snapshot needs to exactly match the mtime used when retrieving it, to the millisecond. Snapshots are retrieved by ephy-about-handler using the time saved in the history service, so the web view needs to use the history service time when saving the snapshot. Otherwise, the time could be off by a few milliseconds, causing snapshot invalidation. Worse, the invalid snapshot would be saved over a previously-valid snapshot, causing snapshots to disappear from the overview.
Created attachment 326652 [details] [review] web-view-test: Fix return type of visit_url_cb tartan would have caught this.
Created attachment 326653 [details] [review] web-view: Never save snapshots when loading If the user starts two loads in rapid succession, the first snapshot needs to be cancelled. We don't want to save an incomplete snapshot, or under the wrong URL.
Created attachment 326654 [details] [review] snapshot-service: Add a FIXME
Review of attachment 326649 [details] [review]: Good idea! ::: lib/ephy-snapshot-service.c @@ +585,3 @@ + + uri = webkit_web_view_get_uri (web_view); + if (ephy_snapshot_service_lookup_snapshot_freshness (service, uri) != SNAPSHOT_FRESH) { uri can be NULL here? @@ +639,3 @@ } + + ensure_snapshot_freshness_for_web_view (service, web_view); When uri is null, we have already scheduled a ephy_snapshot_service_take_from_webview(), I think we should do this inside the if (uri). @@ +882,3 @@ } + + ensure_snapshot_freshness_for_web_view (service, web_view); Same here.
(In reply to Carlos Garcia Campos from comment #20) > ::: lib/ephy-snapshot-service.c > @@ +585,3 @@ > + > + uri = webkit_web_view_get_uri (web_view); > + if (ephy_snapshot_service_lookup_snapshot_freshness (service, uri) != > SNAPSHOT_FRESH) { > > uri can be NULL here? No, it can never happen as the snapshot service is currently used; something will always be loaded in the web view before this function is called, and webkit_web_view_get_uri is documented to only return NULL if nothing has ever been loaded. The return value is not nullable, but that's a WebKit bug (see #152826). But I think that as currently-written, it kinda makes sense for the snapshot service to handle this case; it's currently very generic and has public interface to support many operations that Epiphany never uses. I will try to simplify it and remove this condition in the future, because we'll never want to pass a never-loaded web view to the snapshot service. > @@ +639,3 @@ > } > + > + ensure_snapshot_freshness_for_web_view (service, web_view); > > When uri is null, we have already scheduled a > ephy_snapshot_service_take_from_webview(), I think we should do this inside > the if (uri). Oops, you're right.
(In reply to Michael Catanzaro from comment #13) > Initial patches... not ready to commit because: > > (a) Haven't thought about what happens if the same URL is loaded > simultaneously in multiple web views > > (b) The mtime comparison in gnome_desktop_thumbnail_is_valid is still broken > for some reason, e.g. even with these patches I just saw mtime 1461557996 != > thumb_mtime 1461557948. Requires further investigation. Also it can never work reliably, because this would break whenever a page is loaded but a thumbnail is not created (e.g. if the web view is closed less than one second later). Sigh. I think we will have to bundle the thumbnail service to get rid of the mtime check.
(In reply to Michael Catanzaro from comment #22) > Also it can never work reliably It can, because the history service saves the thumbnail mtime separately, we're just not using it. I think we can just pass 0 everywhere instead.
(In reply to Michael Catanzaro from comment #23) > (In reply to Michael Catanzaro from comment #22) > > Also it can never work reliably > > It can, because the history service saves the thumbnail mtime separately, > we're just not using it. This dates to 0433ac9, when the call to ephy_history_service_set_url_thumbnail_time was moved from frecent_store_thumbnail_saved_cb to ephy_embed_shell_set_thumbnail_path (which is not the right place; ephy_history_service_set_url_thumbnail_time is only supposed to be called when a new thumbnail is saved).
Created attachment 327028 [details] [review] Save the correct thumbnail mtime in the history service Save the thumbnail mtime when saving thumbnails so it actually reflects the mtime embedded in the thumbnail, not the time the page was saved in the history service. This regressed in 0433ac9.
Created attachment 327029 [details] [review] snapshot-service: Clarify error message The snapshot service has two caches.
Created attachment 327031 [details] [review] snapshot-service: Always return snapshots immediately if available Return a stale snapshot, then schedule creation of a new snapshot. This way, we show a preexisting snapshot even if snapshot creation fails. The new snapshot will be used only for future requests.
Created attachment 327032 [details] [review] snapshot-service: Stop using const time_t We don't use const in C except for strings. And certainly not for pass-by-value.
Created attachment 327033 [details] [review] web-view-test: Fix return type of visit_url_cb tartan would have caught this.
Created attachment 327034 [details] [review] web-view: Never save snapshots when loading If the user starts two loads in rapid succession, the first snapshot needs to be cancelled. We don't want to save an incomplete snapshot, or under the wrong URL.
Created attachment 327035 [details] [review] Always pass correct mtime to snapshot service The mtime we pass to the snapshot service when saving the snapshot needs to exactly match the mtime used when retrieving it, to the millisecond. Snapshots are retrieved by ephy-about-handler using the time saved in the history service, so the web view needs to use the history service time when saving the snapshot. Otherwise, the time could be off by a few milliseconds, causing snapshot invalidation. Worse, the invalid snapshot would be saved over a previously-valid snapshot, causing snapshots to disappear from the overview.
Created attachment 327036 [details] [review] Save the correct thumbnail mtime in the history service Save the thumbnail mtime when saving thumbnails so it actually reflects the mtime embedded in the thumbnail, not the time the page was saved in the history service. This regressed in 0433ac9.
Created attachment 327037 [details] [review] snapshot-service: Clarify error message The snapshot service has two caches.
I don't have a reliable test for this, but I think it's working properly now with "Save the correct thumbnail mtime in the history service"... without these patches, I can reproduce the issue after relatively few attempts of restarting Epiphany and loading all the pages in the overview.
Comment on attachment 327035 [details] [review] Always pass correct mtime to snapshot service (Actually shouldn't need this one anymore. This was from before I realized thumbnail time was saved separately in the history service.)
Let's use bug #765863 for the miscellaneous patches. The following two patches are all that I intend to commit for 3.20.2. The first on its own should be sufficient to fix the regression introduced in 3.20.1.
Created attachment 327083 [details] [review] Save the correct thumbnail mtime in the history service Save the thumbnail mtime when saving thumbnails so it actually reflects the mtime embedded in the thumbnail, not the time the page was saved in the history service. This regressed in 0433ac9. It's only noticeable now due to 9754735, which has resulted in thumbnails regularly disappearing from the overview.
Created attachment 327084 [details] [review] snapshot-service: Always return snapshots immediately if available Return a stale snapshot, then schedule creation of a new snapshot. This way, we show a preexisting snapshot even if snapshot creation fails. The new snapshot will be used only for future requests.
Attachment 327083 [details] pushed as 4c4c719 - Save the correct thumbnail mtime in the history service Attachment 327084 [details] pushed as bc19302 - snapshot-service: Always return snapshots immediately if available