Bug 1452660 - Tag queries pointing to an invalid folder are being rewritten as empty urls. r=standard8
MozReview-Commit-ID: FUmmYewl8Vt
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -760,24 +760,22 @@ var PlacesUIUtils = {
});
}
},
/**
* Helper for guessing scheme from an url string.
* Used to avoid nsIURI overhead in frequently called UI functions.
*
- * @param aUrlString the url to guess the scheme from.
- *
+ * @param {string} href The url to guess the scheme from.
* @return guessed scheme for this url string.
- *
* @note this is not supposed be perfect, so use it only for UI purposes.
*/
- guessUrlSchemeForUI: function PUIU_guessUrlSchemeForUI(aUrlString) {
- return aUrlString.substr(0, aUrlString.indexOf(":"));
+ guessUrlSchemeForUI(href) {
+ return href.substr(0, href.indexOf(":"));
},
getBestTitle: function PUIU_getBestTitle(aNode, aDoNotCutTitle) {
var title;
if (!aNode.title && PlacesUtils.nodeIsURI(aNode)) {
// if node title is empty, try to set the label using host and filename
// Services.io.newURI will throw if aNode.uri is not a valid URI
try {
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -1240,17 +1240,22 @@ Database::InitSchema(bool* aDatabaseMigr
NS_ENSURE_SUCCESS(rv, rv);
}
if (currentSchemaVersion < 46) {
rv = MigrateV46Up();
NS_ENSURE_SUCCESS(rv, rv);
}
- // Firefox 61 uses schema version 46.
+ if (currentSchemaVersion < 47) {
+ rv = MigrateV47Up();
+ NS_ENSURE_SUCCESS(rv, rv);
+ }
+
+ // Firefox 61 uses schema version 47.
// Schema Upgrades must add migration code here.
// >>> IMPORTANT! <<<
// NEVER MIX UP SYNC AND ASYNC EXECUTION IN MIGRATORS, YOU MAY LOCK THE
// CONNECTION AND CAUSE FURTHER STEPS TO FAIL.
// In case, set a bool and do the async work in the ScopeExit guard just
// before the migration steps.
}
@@ -2105,23 +2110,25 @@ Database::MigrateV45Up() {
}
nsresult
Database::MigrateV46Up() {
// Convert the existing queries. For simplicity we assume the user didn't
// edit these queries, and just do a 1:1 conversion.
nsresult rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
"UPDATE moz_places "
- "SET url = 'place:tag=' || ( "
- "SELECT title FROM moz_bookmarks "
- "WHERE id = CAST(get_query_param(substr(url, 7), 'folder') AS INT) "
- ") "
+ "SET url = IFNULL('place:tag=' || ( "
+ "SELECT title FROM moz_bookmarks "
+ "WHERE id = CAST(get_query_param(substr(url, 7), 'folder') AS INT) "
+ "), url) "
"WHERE url_hash BETWEEN hash('place', 'prefix_lo') AND "
"hash('place', 'prefix_hi') "
"AND url LIKE '%type=7%' "
+ "AND EXISTS(SELECT 1 FROM moz_bookmarks "
+ "WHERE id = CAST(get_query_param(substr(url, 7), 'folder') AS INT)) "
));
// Recalculate hashes for all tag queries.
rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
"UPDATE moz_places SET url_hash = hash(url) "
"WHERE url_hash BETWEEN hash('place', 'prefix_lo') AND "
"hash('place', 'prefix_hi') "
"AND url LIKE '%tag=%' "
@@ -2134,17 +2141,40 @@ Database::MigrateV46Up() {
"WHERE fk IN ( "
"SELECT id FROM moz_places "
"WHERE url_hash BETWEEN hash('place', 'prefix_lo') AND "
"hash('place', 'prefix_hi') "
"AND url LIKE '%tag=%' "
") "
));
NS_ENSURE_SUCCESS(rv, rv);
+ return NS_OK;
+}
+nsresult
+Database::MigrateV47Up() {
+ // v46 may have mistakenly set some url to NULL, we must fix those.
+ // Since the original url was an invalid query, we replace NULLs with an
+ // empty query.
+ nsresult rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+ "UPDATE moz_places "
+ "SET url = 'place:excludeItems=1', url_hash = hash('place:excludeItems=1') "
+ "WHERE url ISNULL "
+ ));
+ NS_ENSURE_SUCCESS(rv, rv);
+ // Update Sync fields for these queries.
+ rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+ "UPDATE moz_bookmarks SET syncChangeCounter = syncChangeCounter + 1 "
+ "WHERE fk IN ( "
+ "SELECT id FROM moz_places "
+ "WHERE url_hash = hash('place:excludeItems=1') "
+ "AND url = 'place:excludeItems=1' "
+ ") "
+ ));
+ NS_ENSURE_SUCCESS(rv, rv);
return NS_OK;
}
nsresult
Database::GetItemsWithAnno(const nsACString& aAnnoName, int32_t aItemType,
nsTArray<int64_t>& aItemIds)
{
nsCOMPtr<mozIStorageStatement> stmt;
@@ -2335,16 +2365,25 @@ Database::Shutdown()
// Sanity check unique urls
rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
"SELECT 1 FROM moz_places GROUP BY url HAVING count(*) > 1 "
), getter_AddRefs(stmt));
MOZ_ASSERT(NS_SUCCEEDED(rv));
rv = stmt->ExecuteStep(&hasResult);
MOZ_ASSERT(NS_SUCCEEDED(rv));
MOZ_ASSERT(!hasResult, "Found a duplicate url!");
+
+ // Sanity check NULL urls
+ rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
+ "SELECT 1 FROM moz_places WHERE url ISNULL "
+ ), getter_AddRefs(stmt));
+ MOZ_ASSERT(NS_SUCCEEDED(rv));
+ rv = stmt->ExecuteStep(&hasResult);
+ MOZ_ASSERT(NS_SUCCEEDED(rv));
+ MOZ_ASSERT(!hasResult, "Found a NULL url!");
}
#endif
mMainThreadStatements.FinalizeStatements();
mMainThreadAsyncStatements.FinalizeStatements();
RefPtr< FinalizeStatementCacheProxy<mozIStorageStatement> > event =
new FinalizeStatementCacheProxy<mozIStorageStatement>(
--- a/toolkit/components/places/Database.h
+++ b/toolkit/components/places/Database.h
@@ -14,17 +14,17 @@
#include "mozilla/storage/StatementCache.h"
#include "mozilla/Attributes.h"
#include "nsIEventTarget.h"
#include "Shutdown.h"
#include "nsCategoryCache.h"
// This is the schema version. Update it at any schema change and add a
// corresponding migrateVxx method below.
-#define DATABASE_SCHEMA_VERSION 46
+#define DATABASE_SCHEMA_VERSION 47
// Fired after Places inited.
#define TOPIC_PLACES_INIT_COMPLETE "places-init-complete"
// This topic is received when the profile is about to be lost. Places does
// initial shutdown work and notifies TOPIC_PLACES_SHUTDOWN to all listeners.
// Any shutdown work that requires the Places APIs should happen here.
#define TOPIC_PROFILE_CHANGE_TEARDOWN "profile-change-teardown"
// Fired when Places is shutting down. Any code should stop accessing Places
@@ -302,16 +302,17 @@ protected:
nsresult MigrateV39Up();
nsresult MigrateV40Up();
nsresult MigrateV41Up();
nsresult MigrateV42Up();
nsresult MigrateV43Up();
nsresult MigrateV44Up();
nsresult MigrateV45Up();
nsresult MigrateV46Up();
+ nsresult MigrateV47Up();
nsresult UpdateBookmarkRootTitles();
friend class ConnectionShutdownBlocker;
int64_t CreateMobileRoot();
nsresult GetItemsWithAnno(const nsACString& aAnnoName, int32_t aItemType,
nsTArray<int64_t>& aItemIds);
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -3434,16 +3434,22 @@ nsNavHistory::RowToResult(mozIStorageVal
nsNavHistoryResultNode** aResult)
{
NS_ASSERTION(aRow && aOptions && aResult, "Null pointer in RowToResult");
// URL
nsAutoCString url;
nsresult rv = aRow->GetUTF8String(kGetInfoIndex_URL, url);
NS_ENSURE_SUCCESS(rv, rv);
+ // In case of data corruption URL may be null, but our UI code prefers an
+ // empty string.
+ if (url.IsVoid()) {
+ MOZ_ASSERT(false, "Found a NULL url in moz_places");
+ url.SetIsVoid(false);
+ }
// title
nsAutoCString title;
bool isNull;
rv = aRow->GetIsNull(kGetInfoIndex_Title, &isNull);
NS_ENSURE_SUCCESS(rv, rv);
if (!isNull) {
rv = aRow->GetUTF8String(kGetInfoIndex_Title, title);
--- a/toolkit/components/places/tests/head_common.js
+++ b/toolkit/components/places/tests/head_common.js
@@ -1,14 +1,14 @@
/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-const CURRENT_SCHEMA_VERSION = 46;
+const CURRENT_SCHEMA_VERSION = 47;
const FIRST_UPGRADABLE_SCHEMA_VERSION = 30;
const NS_APP_USER_PROFILE_50_DIR = "ProfD";
// Shortcuts to transitions type.
const TRANSITION_LINK = Ci.nsINavHistoryService.TRANSITION_LINK;
const TRANSITION_TYPED = Ci.nsINavHistoryService.TRANSITION_TYPED;
const TRANSITION_BOOKMARK = Ci.nsINavHistoryService.TRANSITION_BOOKMARK;
--- a/toolkit/components/places/tests/migration/test_current_from_v45.js
+++ b/toolkit/components/places/tests/migration/test_current_from_v45.js
@@ -12,60 +12,71 @@ let gTags = [
{ folder: 234567,
url: "place:folder=234567&type=7&queryType=1&somethingelse",
title: "tag2",
hash: "268506675127932",
},
{ folder: 345678,
url: "place:type=7&folder=345678&queryType=1",
title: "tag3",
- hash: " 268506471927988",
+ hash: "268506471927988",
+ },
+ // This will point to an invalid folder id.
+ { folder: 456789,
+ url: "place:type=7&folder=456789&queryType=1",
+ title: "invalid",
+ hash: "268505972797836",
},
];
gTags.forEach(t => t.guid = t.title.padEnd(12, "_"));
add_task(async function setup() {
await setupPlacesDatabase("places_v43.sqlite");
// Setup database contents to be migrated.
let path = OS.Path.join(OS.Constants.Path.profileDir, DB_FILENAME);
let db = await Sqlite.openConnection({ path });
for (let tag of gTags) {
// We can reuse the same guid, it doesn't matter for this test.
await db.execute(`INSERT INTO moz_places (url, guid, url_hash)
VALUES (:url, :guid, :hash)
`, { url: tag.url, guid: tag.guid, hash: tag.hash });
- await db.execute(`INSERT INTO moz_bookmarks (id, fk, guid, title)
- VALUES (:id, (SELECT id FROM moz_places WHERE guid = :guid), :guid, :title)
- `, { id: tag.folder, guid: tag.guid, title: tag.title });
+ if (tag.title != "invalid") {
+ await db.execute(`INSERT INTO moz_bookmarks (id, fk, guid, title)
+ VALUES (:id, (SELECT id FROM moz_places WHERE guid = :guid), :guid, :title)
+ `, { id: tag.folder, guid: tag.guid, title: tag.title });
+ }
}
await db.close();
});
add_task(async function database_is_valid() {
// Accessing the database for the first time triggers migration.
Assert.equal(PlacesUtils.history.databaseStatus,
PlacesUtils.history.DATABASE_STATUS_UPGRADED);
let db = await PlacesUtils.promiseDBConnection();
Assert.equal((await db.getSchemaVersion()), CURRENT_SCHEMA_VERSION);
});
add_task(async function test_queries_converted() {
for (let tag of gTags) {
- let bm = await PlacesUtils.bookmarks.fetch(tag.guid);
- Assert.equal(bm.url.href, "place:tag=" + tag.title);
+ let url = tag.title == "invalid" ? tag.url : "place:tag=" + tag.title;
+ let page = await PlacesUtils.history.fetch(tag.guid);
+ Assert.equal(page.url.href, url);
}
});
add_task(async function test_sync_fields() {
let db = await PlacesUtils.promiseDBConnection();
for (let tag of gTags) {
- let rows = await db.execute(`
- SELECT syncChangeCounter
- FROM moz_bookmarks
- WHERE guid = :guid
- `, { guid: tag.guid });
- Assert.equal(rows[0].getResultByIndex(0), 2);
+ if (tag.title != "invalid") {
+ let rows = await db.execute(`
+ SELECT syncChangeCounter
+ FROM moz_bookmarks
+ WHERE guid = :guid
+ `, { guid: tag.guid });
+ Assert.equal(rows[0].getResultByIndex(0), 2);
+ }
}
});
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/migration/test_current_from_v46.js
@@ -0,0 +1,41 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+let guid = "null".padEnd(12, "_");
+
+add_task(async function setup() {
+ await setupPlacesDatabase("places_v43.sqlite");
+
+ // Setup database contents to be migrated.
+ let path = OS.Path.join(OS.Constants.Path.profileDir, DB_FILENAME);
+ let db = await Sqlite.openConnection({ path });
+ // We can reuse the same guid, it doesn't matter for this test.
+
+ await db.execute(`INSERT INTO moz_places (url, guid, url_hash)
+ VALUES (NULL, :guid, "123456")`, { guid });
+ await db.execute(`INSERT INTO moz_bookmarks (fk, guid)
+ VALUES ((SELECT id FROM moz_places WHERE guid = :guid), :guid)
+ `, { guid });
+ await db.close();
+});
+
+add_task(async function database_is_valid() {
+ // Accessing the database for the first time triggers migration.
+ Assert.equal(PlacesUtils.history.databaseStatus,
+ PlacesUtils.history.DATABASE_STATUS_UPGRADED);
+
+ let db = await PlacesUtils.promiseDBConnection();
+ Assert.equal((await db.getSchemaVersion()), CURRENT_SCHEMA_VERSION);
+
+ let page = await PlacesUtils.history.fetch(guid);
+ Assert.equal(page.url.href, "place:excludeItems=1");
+
+ let rows = await db.execute(`
+ SELECT syncChangeCounter
+ FROM moz_bookmarks
+ WHERE guid = :guid
+ `, { guid });
+ Assert.equal(rows[0].getResultByIndex(0), 2);
+});
--- a/toolkit/components/places/tests/migration/xpcshell.ini
+++ b/toolkit/components/places/tests/migration/xpcshell.ini
@@ -19,8 +19,9 @@ support-files =
[test_current_from_v34_no_roots.js]
[test_current_from_v35.js]
[test_current_from_v36.js]
[test_current_from_v38.js]
[test_current_from_v41.js]
[test_current_from_v42.js]
[test_current_from_v43.js]
[test_current_from_v45.js]
+[test_current_from_v46.js]