diff --git a/browser/components/migration/AutoMigrate.jsm b/browser/components/migration/AutoMigrate.jsm index 2826fc4a60ad..eb0fcbe124b8 100644 --- a/browser/components/migration/AutoMigrate.jsm +++ b/browser/components/migration/AutoMigrate.jsm @@ -210,13 +210,33 @@ const AutoMigrate = { compression: "lz4", }); let stateData = this._dejsonifyUndoState(yield readPromise); - yield this._removeUnchangedBookmarks(stateData.get("bookmarks")); + histogram.add(12); + + this._errorMap = {bookmarks: 0, visits: 0, logins: 0}; + let browserId = Preferences.get(kAutoMigrateBrowserPref, "unknown"); + let reportErrorTelemetry = (type) => { + let histogramId = `FX_STARTUP_MIGRATION_UNDO_${type.toUpperCase()}_ERRORCOUNT`; + Services.telemetry.getKeyedHistogramById(histogramId).add(browserId, this._errorMap[type]); + }; + yield this._removeUnchangedBookmarks(stateData.get("bookmarks")).catch(ex => { + Cu.reportError("Uncaught exception when removing unchanged bookmarks!"); + Cu.reportError(ex); + }); + reportErrorTelemetry("bookmarks"); histogram.add(15); - yield this._removeSomeVisits(stateData.get("visits")); + yield this._removeSomeVisits(stateData.get("visits")).catch(ex => { + Cu.reportError("Uncaught exception when removing history visits!"); + Cu.reportError(ex); + }); + reportErrorTelemetry("visits"); histogram.add(20); - yield this._removeUnchangedLogins(stateData.get("logins")); + yield this._removeUnchangedLogins(stateData.get("logins")).catch(ex => { + Cu.reportError("Uncaught exception when removing unchanged logins!"); + Cu.reportError(ex); + }); + reportErrorTelemetry("logins"); histogram.add(25); // This is async, but no need to wait for it. @@ -419,7 +439,12 @@ const AutoMigrate = { let bmPromises = Array.from(guidToLMMap.keys()).map(guid => { // Ignore bookmarks where the promise doesn't resolve (ie that are missing) // Also check that the bookmark fetch returns isn't null before adding it. - return PlacesUtils.bookmarks.fetch(guid).then(bm => bm && bookmarksFromDB.push(bm), () => {}); + try { + return PlacesUtils.bookmarks.fetch(guid).then(bm => bm && bookmarksFromDB.push(bm), () => {}); + } catch (ex) { + // Ignore immediate exceptions, too. + } + return Promise.resolve(); }); // We can't use the result of Promise.all because that would include nulls // for bookmarks that no longer exist (which we're catching above). @@ -428,7 +453,7 @@ const AutoMigrate = { return bm.lastModified.getTime() == guidToLMMap.get(bm.guid).getTime(); }); - // We need to remove items with no ancestors first, followed by their + // We need to remove items without children first, followed by their // parents, etc. In order to do this, find out how many ancestors each item // has that also appear in our list of things to remove, and sort the items // by those numbers. This ensures that children are always removed before @@ -448,11 +473,16 @@ const AutoMigrate = { unchangedBookmarks.forEach(determineAncestorCount); unchangedBookmarks.sort((a, b) => b._ancestorCount - a._ancestorCount); for (let {guid} of unchangedBookmarks) { - yield PlacesUtils.bookmarks.remove(guid, {preventRemovalOfNonEmptyFolders: true}).catch(err => { + // Can't just use a .catch() because Bookmarks.remove() can throw (rather + // than returning rejected promises). + try { + yield PlacesUtils.bookmarks.remove(guid, {preventRemovalOfNonEmptyFolders: true}); + } catch (err) { if (err && err.message != "Cannot remove a non-empty folder.") { + this._errorMap.bookmarks++; Cu.reportError(err); } - }); + } } }), @@ -468,6 +498,7 @@ const AutoMigrate = { } catch (ex) { Cu.reportError("Failed to remove a login for " + foundLogins.hostname); Cu.reportError(ex); + this._errorMap.logins++; } } } @@ -490,10 +521,11 @@ const AutoMigrate = { }; try { yield PlacesUtils.history.removeVisitsByFilter(visitData); - } catch(ex) { + } catch (ex) { + this._errorMap.visits++; try { visitData.url = visitData.url.href; - } catch (ex) {} + } catch (ignoredEx) {} Cu.reportError("Failed to remove a visit: " + JSON.stringify(visitData)); Cu.reportError(ex); } diff --git a/browser/components/migration/tests/unit/test_automigration.js b/browser/components/migration/tests/unit/test_automigration.js index ed06d8e93f8c..4b03e934a455 100644 --- a/browser/components/migration/tests/unit/test_automigration.js +++ b/browser/components/migration/tests/unit/test_automigration.js @@ -188,6 +188,7 @@ add_task(function* checkUndoPreconditions() { */ add_task(function* checkUndoRemoval() { MigrationUtils.initializeUndoData(); + Preferences.set("browser.migrate.automigrate.browser", "automationbrowser"); // Insert a login and check that that worked. MigrationUtils.insertLoginWrapper({ hostname: "www.mozilla.org", @@ -257,6 +258,18 @@ add_task(function* checkUndoRemoval() { Assert.ok(AutoMigrate.canUndo(), "Should be possible to undo migration"); yield AutoMigrate.undo(); + let histograms = [ + "FX_STARTUP_MIGRATION_UNDO_BOOKMARKS_ERRORCOUNT", + "FX_STARTUP_MIGRATION_UNDO_LOGINS_ERRORCOUNT", + "FX_STARTUP_MIGRATION_UNDO_VISITS_ERRORCOUNT", + ]; + for (let histogramId of histograms) { + let keyedHistogram = Services.telemetry.getKeyedHistogramById(histogramId); + let histogramData = keyedHistogram.snapshot().automationbrowser; + Assert.equal(histogramData.sum, 0, `Should have reported 0 errors to ${histogramId}.`); + Assert.greaterOrEqual(histogramData.counts[0], 1, `Should have reported value of 0 one time to ${histogramId}.`); + } + // Check that the undo removed the history visits: visits = PlacesUtils.history.executeQuery(query, opts); visits.root.containerOpen = true; @@ -612,6 +625,27 @@ add_task(function* checkUndoVisitsState() { }); add_task(function* checkHistoryRemovalCompletion() { + AutoMigrate._errorMap = {bookmarks: 0, visits: 0, logins: 0}; yield AutoMigrate._removeSomeVisits([{url: "http://www.example.com/", limit: -1}]); ok(true, "Removing visits should complete even if removing some visits failed."); + Assert.equal(AutoMigrate._errorMap.visits, 1, "Should have logged the error for visits."); + + // Unfortunately there's not a reliable way to make removing bookmarks be + // unhappy unless the DB is messed up (e.g. contains children but has + // parents removed already). + yield AutoMigrate._removeUnchangedBookmarks([ + {guid: PlacesUtils.bookmarks, lastModified: new Date(0), parentGuid: 0}, + {guid: "gobbledygook", lastModified: new Date(0), parentGuid: 0}, + ]); + ok(true, "Removing bookmarks should complete even if some items are gone or bogus."); + Assert.equal(AutoMigrate._errorMap.bookmarks, 0, + "Should have ignored removing non-existing (or builtin) bookmark."); + + + yield AutoMigrate._removeUnchangedLogins([ + {guid: "gobbledygook", timePasswordChanged: new Date(0)}, + ]); + ok(true, "Removing logins should complete even if logins don't exist."); + Assert.equal(AutoMigrate._errorMap.logins, 0, + "Should have ignored removing non-existing logins."); }); diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json index f4bed112e072..a43074184c3b 100644 --- a/toolkit/components/telemetry/Histograms.json +++ b/toolkit/components/telemetry/Histograms.json @@ -5173,6 +5173,39 @@ "releaseChannelCollection": "opt-out", "description": "Indicates we showed a 'would you like to undo this automatic migration?' notification bar. The bucket indicates which nth day we're on (1st/2nd/3rd, by default - 0 would be indicative the pref didn't get set which shouldn't happen). After 3 days on which the notification gets shown, it will get disabled and never shown again." }, + "FX_STARTUP_MIGRATION_UNDO_BOOKMARKS_ERRORCOUNT": { + "bug_numbers": [1333233], + "alert_emails": ["gijs@mozilla.com"], + "expires_in_version": "58", + "keyed": true, + "kind": "exponential", + "n_buckets": 20, + "high": 100, + "releaseChannelCollection": "opt-out", + "description": "Indicates how many errors we find when trying to 'undo' bookmarks import. Keys are internal ids of browsers we import from, e.g. 'chrome' or 'ie', etc." + }, + "FX_STARTUP_MIGRATION_UNDO_LOGINS_ERRORCOUNT": { + "bug_numbers": [1333233], + "alert_emails": ["gijs@mozilla.com"], + "expires_in_version": "58", + "keyed": true, + "kind": "exponential", + "n_buckets": 20, + "high": 100, + "releaseChannelCollection": "opt-out", + "description": "Indicates how many errors we find when trying to 'undo' login (password) import. Keys are internal ids of browsers we import from, e.g. 'chrome' or 'ie', etc." + }, + "FX_STARTUP_MIGRATION_UNDO_VISITS_ERRORCOUNT": { + "bug_numbers": [1333233], + "alert_emails": ["gijs@mozilla.com"], + "expires_in_version": "58", + "keyed": true, + "kind": "exponential", + "n_buckets": 20, + "high": 100, + "releaseChannelCollection": "opt-out", + "description": "Indicates how many errors we find when trying to 'undo' history import. Keys are internal ids of browsers we import from, e.g. 'chrome' or 'ie', etc." + }, "FX_STARTUP_MIGRATION_DATA_RECENCY": { "bug_numbers": [1276694], "alert_emails": ["gijs@mozilla.com"],