Bug 1320481 - thumbnails cache not cleared with clear history on shutdown. r=Felipe a=jcristau
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 26 Jan 2017 17:54:32 +0100
changeset 378038 26f98ab7307248364557292295979f1452199ba3
parent 378037 4aa072b1110c8c2f2ccdd387d270d5115f488853
child 378039 87ff239bdeb8e12394a069cda2eec29c7765449c
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersFelipe, jcristau
bugs1320481
milestone53.0a2
Bug 1320481 - thumbnails cache not cleared with clear history on shutdown. r=Felipe a=jcristau The "places-shutdown" notification is not a good time to remove observers, since history notifications (and in particular onClearHistory) may happen at the same time (The sanitizer runs at "profile-change-teardown" that is basically the same as "places-shutdown"). We can't ensure proper order of these calls, the safest/simplest thing is just to keep a weak reference and continue observing. MozReview-Commit-ID: 9zHkDqozXup
browser/components/newtab/NewTabMessages.jsm
browser/components/newtab/PlacesProvider.jsm
browser/components/newtab/tests/xpcshell/test_PlacesProvider.js
browser/components/nsBrowserGlue.js
toolkit/components/thumbnails/PageThumbs.jsm
--- a/browser/components/newtab/NewTabMessages.jsm
+++ b/browser/components/newtab/NewTabMessages.jsm
@@ -230,14 +230,13 @@ let NewTabMessages = {
         NewTabWebChannel.off(action, this.handleContentRequest);
       }
 
       for (let pref of NewTabPrefsProvider.newtabPagePrefSet) {
         NewTabPrefsProvider.prefs.off(pref, this.handlePrefChange);
       }
     }
 
-    PlacesProvider.links.uninit();
     NewTabPrefsProvider.prefs.uninit();
     NewTabSearchProvider.search.uninit();
     NewTabWebChannel.uninit();
   }
 };
--- a/browser/components/newtab/PlacesProvider.jsm
+++ b/browser/components/newtab/PlacesProvider.jsm
@@ -89,43 +89,32 @@ Links.prototype = {
         gLinks.emit("linkChanged", {
           url: aURI.spec,
           title: aNewTitle
         });
       }
     },
 
     QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver,
-                        Ci.nsISupportsWeakReference])
+                                           Ci.nsISupportsWeakReference])
   },
 
   /**
    * Must be called before the provider is used.
    * Makes it easy to disable under pref
    */
   init: function PlacesProvider_init() {
     try {
       PlacesUtils.history.addObserver(this.historyObserver, true);
     } catch (e) {
       Cu.reportError(e);
     }
   },
 
   /**
-   * Must be called before the provider is unloaded.
-   */
-  uninit: function PlacesProvider_uninit() {
-    try {
-      PlacesUtils.history.removeObserver(this.historyObserver);
-    } catch (e) {
-      Cu.reportError(e);
-    }
-  },
-
-  /**
    * Gets the current set of links delivered by this provider.
    *
    * @returns {Promise} Returns a promise with the array of links as payload.
    */
   getLinks: Task.async(function*() {
     // Select a single page per host with highest frecency, highest recency.
     // Choose N top such pages. Note +rev_host, to turn off optimizer per :mak
     // suggestion.
--- a/browser/components/newtab/tests/xpcshell/test_PlacesProvider.js
+++ b/browser/components/newtab/tests/xpcshell/test_PlacesProvider.js
@@ -22,16 +22,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 
 XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
     "resource://gre/modules/NetUtil.jsm");
 
 // ensure a profile exists
 do_get_profile();
 
 function run_test() {
+  PlacesProvider.links.init();
   run_next_test();
 }
 
 // url prefix for test history population
 const TEST_URL = "https://mozilla.com/";
 // time when the test starts execution
 const TIME_NOW = (new Date()).getTime();
 
@@ -133,17 +134,16 @@ add_task(function* test_Links_getLinks_D
   links = yield provider.getLinks();
   equal(links.length, 2, "only two links must be left after deduplication");
   equal(links[0].url, visits[1].uri.spec, "earliest link is present");
   equal(links[1].url, visits[3].uri.spec, "most fresent link is present");
 });
 
 add_task(function* test_Links_onLinkChanged() {
   let provider = PlacesProvider.links;
-  provider.init();
 
   let url = "https://example.com/onFrecencyChanged1";
   let linkChangedMsgCount = 0;
 
   let linkChangedPromise = new Promise(resolve => {
     let handler = (_, link) => { // jshint ignore:line
       /* There are 3 linkChanged events:
        * 1. visit insertion (-1 frecency by default)
@@ -164,22 +164,20 @@ add_task(function* test_Links_onLinkChan
   });
 
   // add a visit
   let testURI = NetUtil.newURI(url);
   yield PlacesTestUtils.addVisits(testURI);
   yield linkChangedPromise;
 
   yield PlacesTestUtils.clearHistory();
-  provider.uninit();
 });
 
 add_task(function* test_Links_onClearHistory() {
   let provider = PlacesProvider.links;
-  provider.init();
 
   let clearHistoryPromise = new Promise(resolve => {
     let handler = () => {
       ok(true, `clearHistory event captured`);
       provider.off("clearHistory", handler);
       resolve();
     };
     provider.on("clearHistory", handler);
@@ -188,22 +186,20 @@ add_task(function* test_Links_onClearHis
   // add visits
   for (let i = 0; i <= 10; i++) {
     let url = `https://example.com/onClearHistory${i}`;
     let testURI = NetUtil.newURI(url);
     yield PlacesTestUtils.addVisits(testURI);
   }
   yield PlacesTestUtils.clearHistory();
   yield clearHistoryPromise;
-  provider.uninit();
 });
 
 add_task(function* test_Links_onDeleteURI() {
   let provider = PlacesProvider.links;
-  provider.init();
 
   let testURL = "https://example.com/toDelete";
 
   let deleteURIPromise = new Promise(resolve => {
     let handler = (_, {url}) => { // jshint ignore:line
       equal(testURL, url, "deleted url and expected url are the same");
       provider.off("deleteURI", handler);
       resolve();
@@ -211,22 +207,20 @@ add_task(function* test_Links_onDeleteUR
 
     provider.on("deleteURI", handler);
   });
 
   let testURI = NetUtil.newURI(testURL);
   yield PlacesTestUtils.addVisits(testURI);
   yield PlacesUtils.history.remove(testURL);
   yield deleteURIPromise;
-  provider.uninit();
 });
 
 add_task(function* test_Links_onManyLinksChanged() {
   let provider = PlacesProvider.links;
-  provider.init();
 
   let promise = new Promise(resolve => {
     let handler = () => {
       ok(true);
       provider.off("manyLinksChanged", handler);
       resolve();
     };
 
@@ -237,17 +231,16 @@ add_task(function* test_Links_onManyLink
   let testURI = NetUtil.newURI(testURL);
   yield PlacesTestUtils.addVisits(testURI);
 
   // trigger DecayFrecency
   PlacesUtils.history.QueryInterface(Ci.nsIObserver).
     observe(null, "idle-daily", "");
 
   yield promise;
-  provider.uninit();
 });
 
 add_task(function* test_Links_execute_query() {
   yield PlacesTestUtils.clearHistory();
   let provider = PlacesProvider.links;
 
   let visits = [
     makeVisit(0, 0, true), // frecency 200, today
--- a/browser/components/nsBrowserGlue.js
+++ b/browser/components/nsBrowserGlue.js
@@ -276,24 +276,16 @@ BrowserGlue.prototype = {
         break;
       case "places-database-locked":
         this._isPlacesDatabaseLocked = true;
         // Stop observing, so further attempts to load history service
         // will not show the prompt.
         Services.obs.removeObserver(this, "places-database-locked");
         this._isPlacesLockedObserver = false;
         break;
-      case "places-shutdown":
-        if (this._isPlacesShutdownObserver) {
-          Services.obs.removeObserver(this, "places-shutdown");
-          this._isPlacesShutdownObserver = false;
-        }
-        // places-shutdown is fired when the profile is about to disappear.
-        this._onPlacesShutdown();
-        break;
       case "idle":
         this._backupBookmarks();
         break;
       case "distribution-customization-complete":
         Services.obs.removeObserver(this, "distribution-customization-complete");
         // Customization has finished, we don't need the customizer anymore.
         delete this._distributionCustomizer;
         break;
@@ -414,18 +406,16 @@ BrowserGlue.prototype = {
     os.addObserver(this, "fxaccounts:device_disconnected", false);
     os.addObserver(this, "weave:engine:clients:display-uris", false);
     os.addObserver(this, "session-save", false);
     os.addObserver(this, "places-init-complete", false);
     this._isPlacesInitObserver = true;
     os.addObserver(this, "places-database-locked", false);
     this._isPlacesLockedObserver = true;
     os.addObserver(this, "distribution-customization-complete", false);
-    os.addObserver(this, "places-shutdown", false);
-    this._isPlacesShutdownObserver = true;
     os.addObserver(this, "handle-xul-text-link", false);
     os.addObserver(this, "profile-before-change", false);
     if (AppConstants.MOZ_TELEMETRY_REPORTING) {
       os.addObserver(this, "keyword-search", false);
     }
     os.addObserver(this, "browser-search-engine-modified", false);
     os.addObserver(this, "restart-in-safe-mode", false);
     os.addObserver(this, "flash-plugin-hang", false);
@@ -470,18 +460,16 @@ BrowserGlue.prototype = {
     if (this._bookmarksBackupIdleTime) {
       this._idleService.removeIdleObserver(this, this._bookmarksBackupIdleTime);
       delete this._bookmarksBackupIdleTime;
     }
     if (this._isPlacesInitObserver)
       os.removeObserver(this, "places-init-complete");
     if (this._isPlacesLockedObserver)
       os.removeObserver(this, "places-database-locked");
-    if (this._isPlacesShutdownObserver)
-      os.removeObserver(this, "places-shutdown");
     os.removeObserver(this, "handle-xul-text-link");
     os.removeObserver(this, "profile-before-change");
     if (AppConstants.MOZ_TELEMETRY_REPORTING) {
       os.removeObserver(this, "keyword-search");
     }
     os.removeObserver(this, "browser-search-engine-modified");
     os.removeObserver(this, "flash-plugin-hang");
     os.removeObserver(this, "xpi-signature-changed");
@@ -1000,18 +988,24 @@ BrowserGlue.prototype = {
     try {
       let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"]
                          .getService(Ci.nsIAppStartup);
       appStartup.trackStartupCrashEnd();
     } catch (e) {
       Cu.reportError("Could not end startup crash tracking in quit-application-granted: " + e);
     }
 
+    if (this._bookmarksBackupIdleTime) {
+      this._idleService.removeIdleObserver(this, this._bookmarksBackupIdleTime);
+      delete this._bookmarksBackupIdleTime;
+    }
+
     BrowserUsageTelemetry.uninit();
     SelfSupportBackend.uninit();
+    PageThumbs.uninit();
     NewTabMessages.uninit();
     AboutNewTab.uninit();
     webrtcUI.uninit();
     FormValidationHandler.uninit();
     AutoCompletePopup.uninit();
     DateTimePickerHelper.uninit();
     if (AppConstants.NIGHTLY_BUILD) {
       AddonWatcher.uninit();
@@ -1619,30 +1613,16 @@ BrowserGlue.prototype = {
     }).then(() => {
       // NB: deliberately after the catch so that we always do this, even if
       // we threw halfway through initializing in the Task above.
       Services.obs.notifyObservers(null, "places-browser-init-complete", "");
     });
   },
 
   /**
-   * Places shut-down tasks
-   * - finalize components depending on Places.
-   * - export bookmarks as HTML, if so configured.
-   */
-  _onPlacesShutdown: function BG__onPlacesShutdown() {
-    PageThumbs.uninit();
-
-    if (this._bookmarksBackupIdleTime) {
-      this._idleService.removeIdleObserver(this, this._bookmarksBackupIdleTime);
-      delete this._bookmarksBackupIdleTime;
-    }
-  },
-
-  /**
    * If a backup for today doesn't exist, this creates one.
    */
   _backupBookmarks: function BG__backupBookmarks() {
     return Task.spawn(function*() {
       let lastBackupFile = yield PlacesBackups.getMostRecentBackup();
       // Should backup bookmarks if there are no backups or the maximum
       // interval between backups elapsed.
       if (!lastBackupFile ||
--- a/toolkit/components/thumbnails/PageThumbs.jsm
+++ b/toolkit/components/thumbnails/PageThumbs.jsm
@@ -133,28 +133,27 @@ this.PageThumbs = {
    */
   get contentType() {
     return "image/png";
   },
 
   init: function PageThumbs_init() {
     if (!this._initialized) {
       this._initialized = true;
-      PlacesUtils.history.addObserver(PageThumbsHistoryObserver, false);
+      PlacesUtils.history.addObserver(PageThumbsHistoryObserver, true);
 
       // Migrate the underlying storage, if needed.
       PageThumbsStorageMigrator.migrate();
       PageThumbsExpiration.init();
     }
   },
 
   uninit: function PageThumbs_uninit() {
     if (this._initialized) {
       this._initialized = false;
-      PlacesUtils.history.removeObserver(PageThumbsHistoryObserver);
     }
   },
 
   /**
    * Gets the thumbnail image's url for a given web page's url.
    * @param aUrl The web page's url that is depicted in the thumbnail.
    * @return The thumbnail image's url.
    */
@@ -876,25 +875,26 @@ var PageThumbsExpiration = {
  * Interface to a dedicated thread handling I/O
  */
 var PageThumbsWorker = new BasePromiseWorker("resource://gre/modules/PageThumbsWorker.js");
 // As the PageThumbsWorker performs I/O, we can receive instances of
 // OS.File.Error, so we need to install a decoder.
 PageThumbsWorker.ExceptionHandlers["OS.File.Error"] = OS.File.Error.fromMsg;
 
 var PageThumbsHistoryObserver = {
-  onDeleteURI: function Thumbnails_onDeleteURI(aURI, aGUID) {
+  onDeleteURI(aURI, aGUID) {
     PageThumbsStorage.remove(aURI.spec);
   },
 
-  onClearHistory: function Thumbnails_onClearHistory() {
+  onClearHistory() {
     PageThumbsStorage.wipe();
   },
 
   onTitleChanged() {},
   onBeginUpdateBatch() {},
   onEndUpdateBatch() {},
   onVisit() {},
   onPageChanged() {},
   onDeleteVisits() {},
 
-  QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver])
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver,
+                                         Ci.nsISupportsWeakReference])
 };