diff --git a/toolkit/components/places/History.jsm b/toolkit/components/places/History.jsm index 26e38fc28212..82d3b10cec24 100644 --- a/toolkit/components/places/History.jsm +++ b/toolkit/components/places/History.jsm @@ -650,27 +650,6 @@ var clear = Task.async(function* (db) { notify(observers, "onManyFrecenciesChanged"); }); -/** - * Remove a list of pages from `moz_places` by their id. - * - * @param db: (Sqlite connection) - * The database. - * @param idList: (Array of integers) - * The `moz_places` identifiers for the places to remove. - * @return (Promise) - */ -var removePagesById = Task.async(function*(db, idList) { - if (idList.length == 0) { - return; - } - // Note, we are already in a transaction, since callers create it. - yield db.execute(`DELETE FROM moz_places - WHERE id IN ( ${ sqlList(idList) } )`); - // Hosts accumulated during the places delete are updated through a trigger - // (see nsPlacesTriggers.h). - yield db.execute(`DELETE FROM moz_updatehosts_temp`); -}); - /** * Clean up pages whose history has been modified, by either * removing them entirely (if they are marked for removal, @@ -694,7 +673,25 @@ var removePagesById = Task.async(function*(db, idList) { */ var cleanupPages = Task.async(function*(db, pages) { yield invalidateFrecencies(db, pages.filter(p => p.hasForeign || p.hasVisits).map(p => p.id)); - yield removePagesById(db, pages.filter(p => !p.hasForeign && !p.hasVisits).map(p => p.id)); + + let pageIdsToRemove = pages.filter(p => !p.hasForeign && !p.hasVisits).map(p => p.id); + if (pageIdsToRemove.length > 0) { + let idsList = sqlList(pageIdsToRemove); + // Note, we are already in a transaction, since callers create it. + yield db.execute(`DELETE FROM moz_places WHERE id IN ( ${ idsList } )`); + // Hosts accumulated during the places delete are updated through a trigger + // (see nsPlacesTriggers.h). + yield db.executeCached(`DELETE FROM moz_updatehosts_temp`); + + // Expire orphans. + yield db.executeCached(` + DELETE FROM moz_favicons WHERE NOT EXISTS + (SELECT 1 FROM moz_places WHERE favicon_id = moz_favicons.id)`); + yield db.execute(`DELETE FROM moz_annos + WHERE place_id IN ( ${ idsList } )`); + yield db.execute(`DELETE FROM moz_inputhistory + WHERE place_id IN ( ${ idsList } )`); + } }); /** diff --git a/toolkit/components/places/tests/PlacesTestUtils.jsm b/toolkit/components/places/tests/PlacesTestUtils.jsm index b608985e7237..4dd940f29678 100644 --- a/toolkit/components/places/tests/PlacesTestUtils.jsm +++ b/toolkit/components/places/tests/PlacesTestUtils.jsm @@ -120,23 +120,15 @@ this.PlacesTestUtils = Object.freeze({ * this is a problem only across different connections. */ promiseAsyncUpdates() { - return new Promise(resolve => { - let db = PlacesUtils.history.DBConnection; - let begin = db.createAsyncStatement("BEGIN EXCLUSIVE"); - begin.executeAsync(); - begin.finalize(); - - let commit = db.createAsyncStatement("COMMIT"); - commit.executeAsync({ - handleResult: function () {}, - handleError: function () {}, - handleCompletion: function(aReason) - { - resolve(); - } - }); - commit.finalize(); - }); + return PlacesUtils.withConnectionWrapper("promiseAsyncUpdates", Task.async(function* (db) { + try { + yield db.executeCached("BEGIN EXCLUSIVE"); + yield db.executeCached("COMMIT"); + } catch (ex) { + // If we fail to start a transaction, it's because there is already one. + // In such a case we should not try to commit the existing transaction. + } + })); }, /** diff --git a/toolkit/components/places/tests/history/test_remove.js b/toolkit/components/places/tests/history/test_remove.js index 4706fc311611..e007f396e555 100644 --- a/toolkit/components/places/tests/history/test_remove.js +++ b/toolkit/components/places/tests/history/test_remove.js @@ -342,6 +342,20 @@ add_task(function* test_error_cases() { } }); -function run_test() { - run_next_test(); -} +add_task(function* test_orphans() { + let uri = NetUtil.newURI("http://moz.org/"); + yield PlacesTestUtils.addVisits({ uri }); + + PlacesUtils.favicons.setAndFetchFaviconForPage( + uri, SMALLPNG_DATA_URI, true, PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE, + null, Services.scriptSecurityManager.getSystemPrincipal()); + PlacesUtils.annotations.setPageAnnotation(uri, "test", "restval", 0, + PlacesUtils.annotations.EXPIRE_NEVER); + + yield PlacesUtils.history.remove(uri); + Assert.ok(!(yield PlacesTestUtils.isPageInDB(uri)), "Page should have been removed"); + let db = yield PlacesUtils.promiseDBConnection(); + let rows = yield db.execute(`SELECT (SELECT count(*) FROM moz_annos) + + (SELECT count(*) FROM moz_favicons) AS count`); + Assert.equal(rows[0].getResultByName("count"), 0, "Should not find orphans"); +}); diff --git a/toolkit/components/places/tests/history/test_removeVisitsByFilter.js b/toolkit/components/places/tests/history/test_removeVisitsByFilter.js index 64873a767acd..cda75bb2c283 100644 --- a/toolkit/components/places/tests/history/test_removeVisitsByFilter.js +++ b/toolkit/components/places/tests/history/test_removeVisitsByFilter.js @@ -12,10 +12,8 @@ Cu.import("resource://gre/modules/PromiseUtils.jsm", this); add_task(function* test_removeVisitsByFilter() { let referenceDate = new Date(1999, 9, 9, 9, 9); - /** - * Populate a database with 20 entries, remove a subset of entries, - * ensure consistency. - */ + // Populate a database with 20 entries, remove a subset of entries, + // ensure consistency. let remover = Task.async(function*(options) { do_print("Remover with options " + JSON.stringify(options)); let SAMPLE_SIZE = options.sampleSize; @@ -257,7 +255,21 @@ add_task(function* test_error_cases() { ); }); +add_task(function* test_orphans() { + let uri = NetUtil.newURI("http://moz.org/"); + yield PlacesTestUtils.addVisits({ uri }); -function run_test() { - run_next_test(); -} + PlacesUtils.favicons.setAndFetchFaviconForPage( + uri, SMALLPNG_DATA_URI, true, PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE, + null, Services.scriptSecurityManager.getSystemPrincipal()); + PlacesUtils.annotations.setPageAnnotation(uri, "test", "restval", 0, + PlacesUtils.annotations.EXPIRE_NEVER); + + yield PlacesUtils.history.removeVisitsByFilter({ beginDate: new Date(1999, 9, 9, 9, 9), + endDate: new Date() }); + Assert.ok(!(yield PlacesTestUtils.isPageInDB(uri)), "Page should have been removed"); + let db = yield PlacesUtils.promiseDBConnection(); + let rows = yield db.execute(`SELECT (SELECT count(*) FROM moz_annos) + + (SELECT count(*) FROM moz_favicons) AS count`); + Assert.equal(rows[0].getResultByName("count"), 0, "Should not find orphans"); +});