From a799aea768aedb094014378cf908d6740df8b033 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Tue, 20 Aug 2013 09:26:33 +1000 Subject: [PATCH] Bug 897880 - Thumbnail service must not overwrite existing thumbnails if it gets an error response. r=adw --- .../thumbnails/BackgroundPageThumbs.jsm | 3 +- toolkit/components/thumbnails/PageThumbs.jsm | 64 +++++++- .../components/thumbnails/PageThumbsWorker.js | 24 ++- .../content/backgroundPageThumbsContent.js | 3 + .../components/thumbnails/test/Makefile.in | 1 + .../test/browser_thumbnails_update.js | 155 +++++++++++++++--- toolkit/components/thumbnails/test/head.js | 46 ++++-- .../thumbnails/test/thumbnails_update.sjs | 39 +++++ 8 files changed, 295 insertions(+), 40 deletions(-) create mode 100644 toolkit/components/thumbnails/test/thumbnails_update.sjs diff --git a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm b/toolkit/components/thumbnails/BackgroundPageThumbs.jsm index 87e6a6082a90..c00a8b2b5ff2 100644 --- a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm +++ b/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); }, }; diff --git a/toolkit/components/thumbnails/PageThumbs.jsm b/toolkit/components/thumbnails/PageThumbs.jsm index 91b581a179f4..090eefb861ba 100644 --- a/toolkit/components/thumbnails/PageThumbs.jsm +++ b/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); diff --git a/toolkit/components/thumbnails/PageThumbsWorker.js b/toolkit/components/thumbnails/PageThumbsWorker.js index a6ea6320e7f4..eeef77af59f2 100644 --- a/toolkit/components/thumbnails/PageThumbsWorker.js +++ b/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; + }, + }; diff --git a/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js b/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js index c40b7316e7ed..ee3006f45f9f 100644 --- a/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js +++ b/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)); diff --git a/toolkit/components/thumbnails/test/Makefile.in b/toolkit/components/thumbnails/test/Makefile.in index 4a38d68c548b..335c1dbe7f0d 100644 --- a/toolkit/components/thumbnails/test/Makefile.in +++ b/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 diff --git a/toolkit/components/thumbnails/test/browser_thumbnails_update.js b/toolkit/components/thumbnails/test/browser_thumbnails_update.js index d7a4bdba34e3..b37179b567fe 100644 --- a/toolkit/components/thumbnails/test/browser_thumbnails_update.js +++ b/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,"; - -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,"; + + 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,95 @@ 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); + // now() returns a higher-precision value than the modified time of a file. + // As we set the thumbnail very stale, allowing 1 second of "slop" here + // works around this while still keeping the test valid. + let now = Date.now() - 1000 ; + 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); + // now() returns a higher-precision value than the modified time of a file. + // As we set the thumbnail very stale, allowing 1 second of "slop" here + // works around this while still keeping the test valid. + let now = Date.now() - 1000 ; + 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"); } diff --git a/toolkit/components/thumbnails/test/head.js b/toolkit/components/thumbnails/test/head.js index 77047a38b051..61cf938adf38 100644 --- a/toolkit/components/thumbnails/test/head.js +++ b/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); + }); }); } diff --git a/toolkit/components/thumbnails/test/thumbnails_update.sjs b/toolkit/components/thumbnails/test/thumbnails_update.sjs new file mode 100644 index 000000000000..8ae4d79c9810 --- /dev/null +++ b/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(""); + // 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(""); + // reset the error state incase this ends up being reused for the + // same url and querystring. + setState(aRequest.queryString, ""); + } +}