Bug 897880 - Thumbnail service must not overwrite existing thumbnails if it gets an error response. r=adw

toolkit/components/thumbnails/BackgroundPageThumbs.jsm

@@ -354,7 +354,8 @@ Capture.prototype = {
       callOnDones();
       return;
     }
-    PageThumbs._store(this.url, data.finalURL, data.imageData).then(callOnDones);
+    PageThumbs._store(this.url, data.finalURL, data.imageData, data.wasErrorResponse)
+              .then(callOnDones);
   },
 };
 

toolkit/components/thumbnails/PageThumbs.jsm

@@ -203,18 +203,24 @@ this.PageThumbs = {
    * capture process will send a notification via the observer service on
    * capture, so consumers should watch for such observations if they want to
    * be notified of an updated thumbnail.
+   *
+   * @return {Promise} that's resolved on completion.
    */
   captureIfStale: function PageThumbs_captureIfStale(aUrl) {
+    let deferredResult = Promise.defer();
     let filePath = PageThumbsStorage.getFilePathForURL(aUrl);
-    PageThumbsWorker.post("isFileRecent", [filePath, MAX_THUMBNAIL_AGE_SECS]
+    PageThumbsWorker.post(
+      "isFileRecent",
+      [filePath, MAX_THUMBNAIL_AGE_SECS]
     ).then(result => {
       if (!result.ok) {
         // Sadly there is currently a circular dependency between this module
         // and BackgroundPageThumbs, so do the import locally.
         let BPT = Cu.import("resource://gre/modules/BackgroundPageThumbs.jsm", {}).BackgroundPageThumbs;
-        BPT.capture(aUrl);
+        BPT.capture(aUrl, {onDone: deferredResult.resolve});
       }
     });
+    return deferredResult.promise;
   },
 
   /**
@@ -315,12 +321,15 @@ this.PageThumbs = {
     let channel = aBrowser.docShell.currentDocumentChannel;
     let originalURL = channel.originalURI.spec;
 
+    // see if this was an error response.
+    let wasError = this._isChannelErrorResponse(channel);
+
     TaskUtils.spawn((function task() {
       let isSuccess = true;
       try {
         let blob = yield this.captureToBlob(aBrowser.contentWindow);
         let buffer = yield TaskUtils.readBlob(blob);
-        yield this._store(originalURL, url, buffer);
+        yield this._store(originalURL, url, buffer, wasError);
       } catch (_) {
         isSuccess = false;
       }
@@ -338,10 +347,27 @@ this.PageThumbs = {
    * @param aOriginalURL The URL with which the capture was initiated.
    * @param aFinalURL The URL to which aOriginalURL ultimately resolved.
    * @param aData An ArrayBuffer containing the image data.
+   * @param aWasErrorResponse A boolean indicating if the capture was for a
+   *                          response that returned an error code.
    * @return {Promise}
    */
-  _store: function PageThumbs__store(aOriginalURL, aFinalURL, aData) {
+  _store: function PageThumbs__store(aOriginalURL, aFinalURL, aData, aWasErrorResponse) {
     return TaskUtils.spawn(function () {
+      // If we got an error response, we only save it if we don't have an
+      // existing thumbnail.  If we *do* have an existing thumbnail we "touch"
+      // it so we consider the old version fresh.
+      if (aWasErrorResponse) {
+        let result = yield PageThumbsStorage.touchIfExists(aFinalURL);
+        let exists = result.ok;
+        if (exists) {
+          if (aFinalURL != aOriginalURL) {
+             yield PageThumbsStorage.touchIfExists(aOriginalURL);
+          }
+          return;
+        }
+        // was an error response, but no existing thumbnail - just store
+        // that error response as something is (arguably) better than nothing.
+      }
       let telemetryStoreTime = new Date();
       yield PageThumbsStorage.writeData(aFinalURL, aData);
       Services.telemetry.getHistogramById("FX_THUMBNAILS_STORE_TIME_MS")
@@ -463,6 +489,25 @@ this.PageThumbs = {
     return [this._thumbnailWidth, this._thumbnailHeight];
   },
 
+  /**
+   * Given a channel, returns true if it should be considered an "error
+   * response", false otherwise.
+   */
+  _isChannelErrorResponse: function(channel) {
+    // No valid document channel sounds like an error to me!
+    if (!channel)
+      return true;
+    if (!(channel instanceof Ci.nsIHttpChannel))
+      // it might be FTP etc, so assume it's ok.
+      return false;
+    try {
+      return !channel.requestSucceeded;
+    } catch (_) {
+      // not being able to determine success is surely failure!
+      return true;
+    }
+  },
+
   _prefEnabled: function PageThumbs_prefEnabled() {
     try {
       return !Services.prefs.getBoolPref("browser.pagethumbnails.capturing_disabled");
@@ -570,6 +615,17 @@ this.PageThumbsStorage = {
     return PageThumbsWorker.post("wipe", [this.path]);
   },
 
+  /**
+   * If the file holding the thumbnail for the given URL exists, update the
+   * modification time of the file to now and return true, otherwise return
+   * false.
+   *
+   * @return {Promise}
+   */
+  touchIfExists: function Storage_touchIfExists(aURL) {
+    return PageThumbsWorker.post("touchIfExists", [this.getFilePathForURL(aURL)]);
+  },
+
   _calculateMD5Hash: function Storage_calculateMD5Hash(aValue) {
     let hash = gCryptoHash;
     let value = gUnicodeConverter.convertToByteArray(aValue);

toolkit/components/thumbnails/PageThumbsWorker.js

@@ -176,6 +176,28 @@ let Agent = {
     } finally {
       iterator.close();
     }
-   }
+  },
+
+  touchIfExists: function Agent_touchIfExists(path) {
+    // No OS.File way to update the modification date of the file (bug 905509)
+    // so we open it for reading and writing, read 1 byte from the start of
+    // the file then write that byte back out.
+    // (Sadly it's impossible to use nsIFile here as we have no access to
+    // |Components|)
+    if (!File.exists(path)) {
+      return false;
+    }
+    let file = OS.File.open(path, { read: true, write: true });
+    try {
+      file.setPosition(0); // docs aren't clear on initial position, so seek to 0.
+      let byte = file.read(1);
+      file.setPosition(0);
+      file.write(byte);
+    } finally {
+      file.close();
+    }
+    return true;
+  },
+
 };
 

toolkit/components/thumbnails/content/backgroundPageThumbsContent.js

@@ -52,6 +52,8 @@ const backgroundPageThumbsContent = {
       PageThumbs._captureToCanvas(content, canvas);
       let captureTime = new Date() - captureDate;
 
+      let channel = docShell.currentDocumentChannel;
+      let isErrorResponse = PageThumbs._isChannelErrorResponse(channel);
       let finalURL = this._webNav.currentURI.spec;
       let fileReader = Cc["@mozilla.org/files/filereader;1"].
                        createInstance(Ci.nsIDOMFileReader);
@@ -64,6 +66,7 @@ const backgroundPageThumbsContent = {
             CAPTURE_PAGE_LOAD_TIME_MS: pageLoadTime,
             CAPTURE_CANVAS_DRAW_TIME_MS: captureTime,
           },
+          wasErrorResponse: isErrorResponse,
         });
       };
       canvas.toBlob(blob => fileReader.readAsArrayBuffer(blob));

toolkit/components/thumbnails/test/Makefile.in

@@ -28,6 +28,7 @@ MOCHITEST_BROWSER_FILES := \
 	background_red_redirect.sjs \
 	privacy_cache_control.sjs \
 	thumbnails_background.sjs \
+	thumbnails_update.sjs \
 	$(NULL)
 
 include $(topsrcdir)/config/rules.mk

toolkit/components/thumbnails/test/browser_thumbnails_update.js

@@ -5,20 +5,56 @@
  * These tests check the auto-update facility of the thumbnail service.
  */
 
-let numNotifications = 0;
-const URL = "data:text/html;charset=utf-8,<body%20bgcolor=ff0000></body>";
-
-function observe(subject, topic, data) {
-  is(topic, "page-thumbnail:create", "got expected topic");
-  is(data, URL, "data is our test URL");
-  if (++numNotifications == 2) {
-    // This is the final notification and signals test success...
-    Services.obs.removeObserver(observe, "page-thumbnail:create");
-    next();
+function runTests() {
+  // A "trampoline" - a generator that iterates over sub-iterators
+  let tests = [
+    simpleCaptureTest,
+    errorResponseUpdateTest,
+    goodResponseUpdateTest,
+    foregroundErrorResponseUpdateTest,
+    foregroundGoodResponseUpdateTest
+  ];
+  for (let test of tests) {
+    info("Running subtest " + test.name);
+    for (let iterator of test())
+      yield iterator;
   }
 }
 
-function runTests() {
+function ensureThumbnailStale(url) {
+  // We go behind the back of the thumbnail service and change the
+  // mtime of the file to be in the past.
+  let fname = PageThumbsStorage.getFilePathForURL(url);
+  let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
+  file.initWithPath(fname);
+  ok(file.exists(), fname + " should exist");
+  // Set it as very stale...
+  file.lastModifiedTime = Date.now() - 1000000000;
+}
+
+function getThumbnailModifiedTime(url) {
+  let fname = PageThumbsStorage.getFilePathForURL(url);
+  let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
+  file.initWithPath(fname);
+  return file.lastModifiedTime;
+}
+
+// The tests!
+/* Check functionality of a normal "captureIfStale" request */
+function simpleCaptureTest() {
+  let numNotifications = 0;
+  const URL = "data:text/html;charset=utf-8,<body%20bgcolor=ff0000></body>";
+
+  function observe(subject, topic, data) {
+    is(topic, "page-thumbnail:create", "got expected topic");
+    is(data, URL, "data is our test URL");
+    if (++numNotifications == 2) {
+      // This is the final notification and signals test success...
+      Services.obs.removeObserver(observe, "page-thumbnail:create");
+      next();
+    }
+  }
+
   Services.obs.addObserver(observe, "page-thumbnail:create", false);
   // Create a tab with a red background.
   yield addTab(URL);
@@ -26,6 +62,8 @@ function runTests() {
 
   // Capture the screenshot.
   PageThumbs.captureAndStore(browser, function () {
+    // done with the tab.
+    gBrowser.removeTab(gBrowser.selectedTab);
     // We've got a capture so should have seen the observer.
     is(numNotifications, 1, "got notification of item being created.");
     // The capture is now "fresh" - so requesting the URL should not cause
@@ -33,18 +71,89 @@ function runTests() {
     PageThumbs.captureIfStale(URL);
     is(numNotifications, 1, "still only 1 notification of item being created.");
 
-    // Now we will go behind the back of the thumbnail service and change the
-    // mtime of the file to be in the past.
-    let fname = PageThumbsStorage.getFilePathForURL(URL);
-    let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
-    file.initWithPath(fname);
-    ok(file.exists(), fname + " doesn't exist");
-    // Set it as very stale...
-    file.lastModifiedTime = Date.now() - 1000000000;
+    ensureThumbnailStale(URL);
     // Ask for it to be updated.
     PageThumbs.captureIfStale(URL);
     // But it's async, so wait - our observer above will call next() when
     // the notification comes.
   });
-  yield undefined; // wait for callbacks to call 'next'...
+  yield undefined // wait for callbacks to call 'next'...
+}
+
+/* Check functionality of a background capture when there is an error response
+   from the server.
+ */
+function errorResponseUpdateTest() {
+  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?fail";
+  yield addTab(URL);
+
+  yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
+  gBrowser.removeTab(gBrowser.selectedTab);
+  // update the thumbnail to be stale, then re-request it.  The server will
+  // return a 400 response and a red thumbnail.
+  // The b/g service should (a) not save the thumbnail and (b) update the file
+  // to have an mtime of "now" - so we (a) check the thumbnail remains green
+  // and (b) check the mtime of the file is >= now.
+  ensureThumbnailStale(URL);
+  let now = Date.now();
+  PageThumbs.captureIfStale(URL).then(() => {
+    ok(getThumbnailModifiedTime(URL) >= now, "modified time should be >= now");
+    retrieveImageDataForURL(URL, function ([r, g, b]) {
+      is("" + [r,g,b], "" + [0, 255, 0], "thumbnail is still green");
+      next();
+    });
+  }).then(null, err => {ok(false, "Error in captureIfStale: " + err)});
+  yield undefined; // wait for callback to call 'next'...
+}
+
+/* Check functionality of a background capture when there is a non-error
+   response from the server.  This test is somewhat redundant - although it is
+   using a http:// URL instead of a data: url like most others.
+ */
+function goodResponseUpdateTest() {
+  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?ok";
+  yield addTab(URL);
+  let browser = gBrowser.selectedBrowser;
+
+  yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
+  // 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.
+  ensureThumbnailStale(URL);
+  let now = Date.now();
+  PageThumbs.captureIfStale(URL).then(() => {
+    ok(getThumbnailModifiedTime(URL) >= now, "modified time should be >= now");
+    // the captureIfStale request saw a 200 response with the red body, so we
+    // expect to see the red version here.
+    retrieveImageDataForURL(URL, function ([r, g, b]) {
+      is("" + [r,g,b], "" + [255, 0, 0], "thumbnail is now red");
+      next();
+    });
+  }).then(null, err => {ok(false, "Error in captureIfStale: " + err)});
+  yield undefined; // wait for callback to call 'next'...
+}
+
+/* Check functionality of a foreground capture when there is an error response
+   from the server.
+ */
+function foregroundErrorResponseUpdateTest() {
+  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?fail";
+  yield addTab(URL);
+  yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
+  gBrowser.removeTab(gBrowser.selectedTab);
+  // do it again - the server will return a 400, so the foreground service
+  // should not update it.
+  yield addTab(URL);
+  yield captureAndCheckColor(0, 255, 0, "we still have a green thumbnail");
+}
+
+function foregroundGoodResponseUpdateTest() {
+  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?ok";
+  yield addTab(URL);
+  yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
+  gBrowser.removeTab(gBrowser.selectedTab);
+  // do it again - the server will return a 200, so the foreground service
+  // should  update it.
+  yield addTab(URL);
+  yield captureAndCheckColor(255, 0, 0, "we now  have a red thumbnail");
 }

toolkit/components/thumbnails/test/head.js

@@ -52,8 +52,13 @@ let TestRunner = {
   next: function () {
     try {
       let value = TestRunner._iter.next();
-      if (value && typeof value.then == "function")
-        value.then(next);
+      if (value && typeof value.then == "function") {
+        value.then(result => {
+          next(result);
+        }, error => {
+          ok(false, error + "\n" + error.stack);
+        });
+      }
     } catch (e if e instanceof StopIteration) {
       finish();
     }
@@ -129,20 +134,33 @@ 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);
 
-  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);
 
-  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);
-    aCallback(ctx.getImageData(0, 0, 100, 100).data);
+      // 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);
+    });
   });
 }
 

toolkit/components/thumbnails/test/thumbnails_update.sjs

@@ -0,0 +1,39 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// This server-side script primarily must return different *content* for the
+// second request than it did for the first.
+// Also, it should be able to return an error response when requested for the
+// second response.
+// So the basic tests will be to grab the thumbnail, then request it to be
+// grabbed again:
+// * If the second request succeeded, the new thumbnail should exist.
+// * If the second request is an error, the new thumbnail should be ignored.
+
+function handleRequest(aRequest, aResponse) {
+  aResponse.setHeader("Content-Type", "text/html;charset=utf-8", false);
+  // we want to disable gBrowserThumbnails on-load capture for these responses,
+  // so set as a "no-store" response.
+  aResponse.setHeader("Cache-Control", "no-store");
+
+  let doneError = getState(aRequest.queryString);
+  if (!doneError) {
+    // first request - return a response with a green body and 200 response.
+    aResponse.setStatusLine(aRequest.httpVersion, 200, "OK - It's green");
+    aResponse.write("<html><body bgcolor=00ff00></body></html>");
+    // set the  state so the next request does the "second request" thing below.
+    setState(aRequest.queryString, "yep");
+  } else {
+    // second request - this will be done by the b/g service.
+    // We always return a red background, but depending on the query string we
+    // return either a 200 or 401 response.
+    if (aRequest.queryString == "fail")
+      aResponse.setStatusLine(aRequest.httpVersion, 401, "Oh no you don't");
+    else
+      aResponse.setStatusLine(aRequest.httpVersion, 200, "OK - It's red");
+    aResponse.write("<html><body bgcolor=ff0000></body></html>");
+    // reset the error state incase this ends up being reused for the
+    // same url and querystring.
+    setState(aRequest.queryString, "");
+  }
+}