1192394 - thumbnail tile may remain unchanged in a single newtab page after thumbnail file updates

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Oliver Henshaw]

I can reliably reproduce a newtab page showing an old thumbnail after
forcing the thumbnail to refresh and seeing the new thumbnail on all other newtab pages.

This happens both with and without the bug #1180715 fix.

STR:

Prepare by:

* Create a new profile
* Make sure browser.newtab.preload is still the default true
* Browse around a little to create at least one frecent site
* Find the file that stores the site thumbnail, e.g. with PageThumbsStorage.getFilePathForURL
* Make sure that this file is at least two days old and showing a different image (e.g. copy it from your regular profile)

Now,

1 Open a new tab page
=> See a tile showing the old thumbnail
2 Open the site in a new tab from the tile; wait for it to load; switch to it; wait a few more seconds
3 Switch back to the newtab page
=> The tile now shows a thumbnail for the current page
4 Open a new newtab page
=> The tile shows a thumbnail for the old page
5 Open a further newtab page
=> The tile shows a thumbnail for the current page

The reason for this is that the preloaded newtab page that will be opened in Step 4 happens to have the loadid that is stored in the image cache entry. So the old image will continue to be used by imgLoader regardless of the fact that hasExpired=true in imgLoader::ValidateEntry().

Comment 1

[Oliver Henshaw]

Attachment: Force an image load whenever the thumbnail file changes

Add a revision tag to moz-page-thumb URLs. Increment it whenever a new
thumbnail is captured and stored to disk. This prevents new loads from
re-using the old cached value even if the document context matches the
stored loadId of the cache entry.

Comment 2

[Oliver Henshaw]

Comment on attachment 8645157 [details] [diff] [review]
Force an image load whenever the thumbnail file changes

Note that this is patch is on top of bug #1180715 but doesn't really depend on it. Just they touch the same line and share some diff context.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8645157 [details] [diff] [review]
Force an image load whenever the thumbnail file changes

Review of attachment 8645157 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand how this patch works... The preloaded about:newtab page is sitting in the background, waiting to become visible. When a new tab is opened the old image is of course shown as nothing instructs the page to refresh the thumbnail.

I'm sure you tried the patch... so how does this work? I don't see the stale tile calling .getThumbnailURL() again and updating its screenshot?

Comment 4

[Oliver Henshaw]

(In reply to Tim Taubert [:ttaubert] (on PTO, back Aug 31st) from comment #3)
> Comment on attachment 8645157 [details] [diff] [review]
> Force an image load whenever the thumbnail file changes
> 
> Review of attachment 8645157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand how this patch works... The preloaded about:newtab page
> is sitting in the background, waiting to become visible. When a new tab is
> opened the old image is of course shown as nothing instructs the page to
> refresh the thumbnail.
> 
> I'm sure you tried the patch... so how does this work? I don't see the stale
> tile calling .getThumbnailURL() again and updating its screenshot?
site.refreshThumbnail() is called from browser/base/content/newtab/page.js:observe() when aTopic == "page-thumbnail:create"; refreshThumbnail() calls PageThumbs.getThumbnailURL(); and the notification comes from PageThumbs._store().

Comment 5

[Oliver Henshaw]

Comment on attachment 8645157 [details] [diff] [review]
Force an image load whenever the thumbnail file changes

(Adding feedback flag on the assumption that :ttaubert is going to be out of contact until the end of the month)

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8645157 [details] [diff] [review]
Force an image load whenever the thumbnail file changes

Review of attachment 8645157 [details] [diff] [review]:
-----------------------------------------------------------------

OK, sounds fine to me.

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +557,5 @@
> +  _revisionTable: {},
> +  _updateRevision(aURL) {
> +    this._revisionTable[aURL] = this.getRevision(aURL) + 1;
> +  },
> +  /**

Nit: Please add a blank line before this line.

::: toolkit/components/thumbnails/PageThumbsProtocol.js
@@ +12,5 @@
>   *
>   * moz-page-thumb://thumbnail/?url=http%3A%2F%2Fwww.mozilla.org%2F
>   *
>   * This URL requests an image for 'http://www.mozilla.org/'.
> + * There may also be other key=value pairs in the query string.

Would you mind documenting the keys while you're here?  url is the only existing one as far as I know.

Comment 7

[Oliver Henshaw]

Attachment: Force an image load whenever the thumbnail file changes

(In reply to Drew Willcoxon :adw from comment #6)
> Comment on attachment 8645157 [details] [diff] [review]
> Force an image load whenever the thumbnail file changes
> 
> Review of attachment 8645157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, sounds fine to me.
> 
> ::: toolkit/components/thumbnails/PageThumbs.jsm
> @@ +557,5 @@
> > +  _revisionTable: {},
> > +  _updateRevision(aURL) {
> > +    this._revisionTable[aURL] = this.getRevision(aURL) + 1;
> > +  },
> > +  /**
> 
> Nit: Please add a blank line before this line.
Done

> 
> ::: toolkit/components/thumbnails/PageThumbsProtocol.js
> @@ +12,5 @@
> >   *
> >   * moz-page-thumb://thumbnail/?url=http%3A%2F%2Fwww.mozilla.org%2F
> >   *
> >   * This URL requests an image for 'http://www.mozilla.org/'.
> > + * There may also be other key=value pairs in the query string.
> 
> Would you mind documenting the keys while you're here?  url is the only
> existing one as far as I know.

OK. I didn't want to be pinned down on the semantics or value of the revision
tag, but I've written something now. What do you think?

Are there any security/privacy concerns about the revision tag? Anything that
can see the moz-page-thumb URL can maybe infer which sites the user visits most
frecently. And tell whether the thumbnail has been updated in the lifetime of
the browser instance. But I guess anything that can load PageThumbs.jsm and
call getThumbnailURL() is trusted anyway?


Further changes to tests:
=========================

* Remove the wait from browser_thumbnails_update.js test as it's no longer
needed (introduced in bug 1180715).

* Return to using the top-level chrome document for retrieveImageDataForURL()
rather than creating a new document in a new tab for each caller (introduced
in bug 897880).
The tests are now slightly more sensitive to regressions for thumbnails that
should change but slightly less sensitive to regressions for thumbnails that
should not change.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8652922 [details] [diff] [review]
Force an image load whenever the thumbnail file changes

Review of attachment 8652922 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.  It's nice that we can remove all that stuff about dealing with the image cache.

The security question is interesting.  The patch never purges revisions from the revision table, which could lead to a couple of leaks.  Someone with chrome access could just look in the table to get its list of URLs, even if you had for example cleared your history after the table had been populated.  And someone with non-chrome access but who could see your page thumbs URLs (which should not happen, but let's say it does, because they're in the DOM of a page that gets compromised) could tell that you had visited the site N times in the session.

Should we worry about either of those things?  I'm not sure.  For the chrome part, there are places right now in about:newtab alone where we don't purge in-memory storage related to sites (blocked and pinned links, at least).  This isn't worse than that.

For the non-chrome part, non-chrome should not be able to see thumb URLs at all, so ideally it shouldn't be a problem.  And if non-chrome can see your thumb URLs, then it doesn't need the revision part to tell that the site is frecent.  But what does the revision tell it?  The only bad thing I can think of is if it also somehow knew that you had cleared your history earlier, after you visited some site.  In that case, if the site is visited again and causes a thumb URL to be generated, then revision > 0 (or 1?) implies that the site had been visited before (in the same session).

So I guess I'm inclined to say we shouldn't worry about it.  What do you think?

Comment 9

[Oliver Henshaw]

Attachment: Force an image load whenever the thumbnail file changes

Was thinking about this again recently. Almost everything you can discern from
the thumbnail URL you can discern from the content of the thumbnail itself.
e.g. if you can accesss either of them, you could poll every couple of days and
see whether they had changed, or force a visit to the site and see whether they
change as a a result.

But having the revision tag be a simple counter of updates is slightly more
information than that, as your history clearing example illustrates. This can
be avoided by initialising the revision tag with a random value - taking some
care to avoid generating thumbnail URLs that collide with a stale thumbnail in
the image cache.

Changes:
========

Randomise initial value of revision tag and increment (with wrap-around) on
update. Avoids encoding any information about frecency in the tag.

Minor revision to commit message: s/Increment it whenever/Change it whenever/

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8679591 [details] [diff] [review]
Force an image load whenever the thumbnail file changes

Review of attachment 8679591 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for coming back to this.

Comment 11

[Oliver Henshaw]

Attachment: Force an image load whenever the thumbnail file changes.

Semicolons++

Comment 12

[Oliver Henshaw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3fe5ada299e

Comment 13

[Oliver Henshaw]

All platforms but windows are finished, windows has quite a backlog and so these test might not start before Sunday. I've cancelled most remaining pending tests (on windowss) apart from xpcshell-test, and browser-chrome as I only expect this to have an impact on:

browser/base/content/test/newtab/
toolkit/components/thumbnails/test/
toolkit/modules/tests/xpcshell/test_NewTabUtils.js

Apart from that, it looks good to me. I hope all these https://bugzilla.mozilla.org/show_bug.cgi?id=1217681 are someone else's problem though.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3de20e823464

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3de20e823464

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3de20e823464

Comment 17

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion