Bug 1452660 - Tag queries pointing to an invalid folder are being rewritten as empty urls. r=standard8
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 11 Apr 2018 15:45:23 +0200
changeset 412967 a061dfd9f1b14c2f5dd74f5c7d82fbfb76fd6f60
parent 412966 e4e3e42d46838df5d0702d2eab6273449902ee98
child 412968 4bdc8b9244705508152e90a05f746def85492733
push id33828
push userarchaeopteryx@coole-files.de
push dateThu, 12 Apr 2018 19:19:41 +0000
treeherdermozilla-central@6e22c4a726c2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstandard8
bugs1452660
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
@@ -3455,16 +3455,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]