Bug 1452660 - Tag queries pointing to an invalid folder are being rewritten as empty urls. r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 11 Apr 2018 15:45:23 +0200
changeset 780621 6f5d940bd1ae519ee54f3acf5b93a244d12b0db9
parent 780394 f6c3a0a19d82db25750d8badccd5cf37e79d028e
push id106043
push usermak77@bonardo.net
push dateWed, 11 Apr 2018 16:55:19 +0000
reviewersstandard8
bugs1452660
milestone61.0a1
Bug 1452660 - Tag queries pointing to an invalid folder are being rewritten as empty urls. r=standard8 MozReview-Commit-ID: FUmmYewl8Vt
browser/components/places/PlacesUIUtils.jsm
toolkit/components/places/Database.cpp
toolkit/components/places/Database.h
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/tests/head_common.js
toolkit/components/places/tests/migration/test_current_from_v45.js
toolkit/components/places/tests/migration/test_current_from_v46.js
toolkit/components/places/tests/migration/xpcshell.ini
--- 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]