Bug 1192394 - Force an image load whenever the thumbnail file changes…

…. r=adw

Add a revision tag to moz-page-thumb URLs. Change 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.

Also drop no-longer needed wait in browser_thumbnails_update.js test.

Restores behaviour of test/head.js:retrieveImageDataForURL to that of
before bug 897880 - i.e. load the thumbnail image in the context of the
top level chrome document (rather than a new tab every time). It's
likely that bug 1192394 is the reason for the test failures observed in
bug 897880 comment #11.

toolkit/components/thumbnails/PageThumbs.jsm

@@ -158,7 +158,8 @@ this.PageThumbs = {
    */
   getThumbnailURL: function PageThumbs_getThumbnailURL(aUrl) {
     return this.scheme + "://" + this.staticHost +
-           "/?url=" + encodeURIComponent(aUrl);
+           "/?url=" + encodeURIComponent(aUrl) +
+           "&revision=" + PageThumbsStorage.getRevision(aUrl);
   },
 
    /**
@@ -542,6 +543,44 @@ this.PageThumbsStorage = {
     return OS.Path.join(this.path, this.getLeafNameForURL(aURL));
   },
 
+  _revisionTable: {},
+
+  // Generate an arbitrary revision tag, i.e. one that can't be used to
+  // infer URL frecency.
+  _updateRevision(aURL) {
+    // Initialize with a random value and increment on each update. Wrap around
+    // modulo _revisionRange, so that even small values carry no meaning.
+    let rev = this._revisionTable[aURL];
+    if (rev == null)
+      rev = Math.floor(Math.random() * this._revisionRange);
+    this._revisionTable[aURL] = (rev + 1) % this._revisionRange;
+  },
+
+  // If two thumbnails with the same URL and revision are in cache at the
+  // same time, the image loader may pick the stale thumbnail in some cases.
+  // Therefore _revisionRange must be large enough to prevent this, e.g.
+  // in the pathological case image.cache.size (5MB by default) could fill
+  // with (abnormally small) 10KB thumbnail images if the browser session
+  // runs long enough (though this is unlikely as thumbnails are usually
+  // only updated every MAX_THUMBNAIL_AGE_SECS).
+  _revisionRange: 8192,
+
+  /**
+  * Return a revision tag for the thumbnail stored for a given URL.
+  *
+  * @param aURL The URL spec string
+  * @return A revision tag for the corresponding thumbnail. Returns a changed
+  * value whenever the stored thumbnail changes.
+  */
+  getRevision(aURL) {
+    let rev = this._revisionTable[aURL];
+    if (rev == null) {
+      this._updateRevision(aURL);
+      rev = this._revisionTable[aURL];
+    }
+    return rev;
+  },
+
   /**
    * Write the contents of a thumbnail, off the main thread.
    *
@@ -571,7 +610,7 @@ this.PageThumbsStorage = {
       msg /*we don't want that message garbage-collected,
            as OS.Shared.Type.void_t.in_ptr.toMsg uses C-level
            memory tricks to enforce zero-copy*/).
-      then(null, this._eatNoOverwriteError(aNoOverwrite));
+      then(() => this._updateRevision(aURL), this._eatNoOverwriteError(aNoOverwrite));
   },
 
   /**
@@ -590,7 +629,7 @@ this.PageThumbsStorage = {
     let targetFile = this.getFilePathForURL(aTargetURL);
     let options = { noOverwrite: aNoOverwrite };
     return PageThumbsWorker.post("copy", [sourceFile, targetFile, options]).
-      then(null, this._eatNoOverwriteError(aNoOverwrite));
+      then(() => this._updateRevision(aTargetURL), this._eatNoOverwriteError(aNoOverwrite));
   },
 
   /**

toolkit/components/thumbnails/PageThumbsProtocol.js

@@ -10,9 +10,10 @@
  *
  * URL structure:
  *
- * moz-page-thumb://thumbnail/?url=http%3A%2F%2Fwww.mozilla.org%2F
+ * moz-page-thumb://thumbnail/?url=http%3A%2F%2Fwww.mozilla.org%2F&revision=XX
  *
  * This URL requests an image for 'http://www.mozilla.org/'.
+ * The value of the revision key may change when the stored thumbnail changes.
  */
 
 "use strict";

toolkit/components/thumbnails/test/browser_thumbnails_update.js

@@ -88,11 +88,6 @@ function capIfStaleErrorResponseUpdateTest() {
   yield addTab(URL);
 
   yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
-
-  // image cache entry timestamps have second resolution
-  // so make sure the second part of this test takes part in a different second.
-  yield wait(2000);
-
   // update the thumbnail to be stale, then re-request it.  The server will
   // return a 400 response and a red thumbnail.
   // The service should not save the thumbnail - so we (a) check the thumbnail
@@ -124,11 +119,6 @@ function capIfStaleGoodResponseUpdateTest() {
   let browser = gBrowser.selectedBrowser;
 
   yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
-
-  // image cache entry timestamps have second resolution
-  // so make sure the second part of this test takes part in a different second.
-  yield wait(2000);
-
   // update the thumbnail to be stale, then re-request it.  The server will
   // return a 200 response and a red thumbnail - so that new thumbnail should
   // end up captured.
@@ -158,11 +148,6 @@ function regularCapErrorResponseUpdateTest() {
   yield addTab(URL);
   yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
   gBrowser.removeTab(gBrowser.selectedTab);
-
-  // image cache entry timestamps have second resolution
-  // so make sure the second part of this test takes part in a different second.
-  yield wait(2000);
-
   // do it again - the server will return a 400, so the foreground service
   // should not update it.
   yield addTab(URL);
@@ -177,11 +162,6 @@ function regularCapGoodResponseUpdateTest() {
   yield addTab(URL);
   yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
   gBrowser.removeTab(gBrowser.selectedTab);
-
-  // image cache entry timestamps have second resolution
-  // so make sure the second part of this test takes part in a different second.
-  yield wait(2000);
-
   // do it again - the server will return a 200, so the foreground service
   // should  update it.
   yield addTab(URL);

toolkit/components/thumbnails/test/head.js

@@ -145,33 +145,21 @@ function captureAndCheckColor(aRed, aGreen, aBlue, aMessage) {
 function retrieveImageDataForURL(aURL, aCallback) {
   let width = 100, height = 100;
   let thumb = PageThumbs.getThumbnailURL(aURL, width, height);
-  // create a tab with a chrome:// URL so it can host the thumbnail image.
-  // Note that we tried creating the element directly in the top-level chrome
-  // document, but this caused a strange problem:
-  // * call this with the url of an image.
-  // * immediately change the image content.
-  // * call this again with the same url (now holding different content)
-  // The original image data would be used.  Maybe the img hadn't been
-  // collected yet and the platform noticed the same URL, so reused the
-  // content?  Not sure - but this solves the problem.
-  addTab("chrome://global/content/mozilla.xhtml", () => {
-    let doc = gBrowser.selectedBrowser.contentDocument;
-    let htmlns = "http://www.w3.org/1999/xhtml";
-    let img = doc.createElementNS(htmlns, "img");
-    img.setAttribute("src", thumb);
-
-    whenLoaded(img, function () {
-      let canvas = document.createElementNS(htmlns, "canvas");
-      canvas.setAttribute("width", width);
-      canvas.setAttribute("height", height);
-
-      // Draw the image to a canvas and compare the pixel color values.
-      let ctx = canvas.getContext("2d");
-      ctx.drawImage(img, 0, 0, width, height);
-      let result = ctx.getImageData(0, 0, 100, 100).data;
-      gBrowser.removeTab(gBrowser.selectedTab);
-      aCallback(result);
-    });
+
+  let htmlns = "http://www.w3.org/1999/xhtml";
+  let img = document.createElementNS(htmlns, "img");
+  img.setAttribute("src", thumb);
+
+  whenLoaded(img, function () {
+    let canvas = document.createElementNS(htmlns, "canvas");
+    canvas.setAttribute("width", width);
+    canvas.setAttribute("height", height);
+
+    // Draw the image to a canvas and compare the pixel color values.
+    let ctx = canvas.getContext("2d");
+    ctx.drawImage(img, 0, 0, width, height);
+    let result = ctx.getImageData(0, 0, 100, 100).data;
+    aCallback(result);
   });
 }