715355 - (placesGUIDsIO) Deprecate and remove old style GUIDs

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Places now uses new style 12 chars GUIDs, though many profiles still have old style ones, that are much longer and stored as annotations.
This may cause a lot of synchronous IO for users with sync add-ons like XMarks  or similar, and takes up lots of db space.

The old GUIDs are usually setup and retrieved through getItemGUID, setItemGUID, getItemIdForGUID APIs from the bookmarks service.

So this bug involves:
- contacting authors of major addons using old GUIDs
- figure out a migration path with them, eventually add new (async!) APIs to access the new GUIDs, if they need them
- remove the above deprecated GUID APIs from nsINavBookmarksService
- add a migration path to completely get rid of existing GUID annotations
- may require to fix bug 641671 and bug 627487, though Sync workarounded these, so it's not mandatory

Comment 1

[Marco Bonardo [:mak]]

We will do this as soon as the tree merges and reopens for Firefox 14.
http://blog.bonardo.net/2012/02/16/add-ons-devs-heads-up-we-are-killing-old-bookmarks-guids

Comment 2

[Marco Bonardo [:mak]]

in the steps to do here, I forgot adding a maintenance task to get rid of any potentially added guid annotation.

Comment 3

[:Paolo Amadini]

Attachment: Nuke GUID APIs and tests

Complete removal of old-style GUID APIs and tests. Some notes:

* Do we really need a new migration task to delete old GUIDs on update, or is
  a maintenance task enough? Old GUIDs could only have been added by add-ons
  at present, and removing the APIs makes them inaccessible in any case. While
  deleting immediately on update provides the performance benefits immediately,
  at the same time it slows down the update process immediately. In case a
  migration is needed, a brief overview of how we do and test that migration
  would be handy.
* There is code to preserve old-style GUIDs on undo/redo. This has never been
  done with new-style GUIDs. For the moment I've just removed the unused code,
  will we need a similar feature in the future? It probably depends on having
  the asynchronous bookmarks API and transactions code first.
* We might want to add a new test for the removal of both the "weave" and
  GUID annotations on maintenance.

Comment 4

[:Paolo Amadini]

Another note, this *requires* callers in comm-central to be updated first.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini [:paolo] from comment #3)
> * Do we really need a new migration task to delete old GUIDs on update, or is
>   a maintenance task enough? Old GUIDs could only have been added by add-ons
>   at present, and removing the APIs makes them inaccessible in any case.

Imo yes, the fact is that some user may never hit idle, plus the fact it's simpler for a user to connect a no more working add-on with an update, rather than seeing it break after a week (yes, you have a valid point about the APIs, but they may be doing direct queries). The migration stuff should be fast enough, it basically deletes from moz_items_annos table, based on the id taken from moz_anno_attributes.

> In case a
>   migration is needed, a brief overview of how we do and test that migration
>   would be handy.

We have a tests/migration folder for those tests, basically we have an old db there and we update it, checking expected has has been removed/added.

> * There is code to preserve old-style GUIDs on undo/redo. This has never been
>   done with new-style GUIDs. For the moment I've just removed the unused
> code,
>   will we need a similar feature in the future? It probably depends on having
>   the asynchronous bookmarks API and transactions code first.

We will need that in the future, but so far we lacking guids also in backups... there's lot of stuff to fix there yet.  Fine to remove the old code for now.

> * We might want to add a new test for the removal of both the "weave" and
>   GUID annotations on maintenance.

you can do that in test_preventive_maintenance.js

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 611272 [details] [diff] [review]
Nuke GUID APIs and tests

Review of attachment 611272 [details] [diff] [review]:
-----------------------------------------------------------------

these, plus previous comment. Btw looks fine.

::: toolkit/components/places/PlacesUtils.jsm
@@ +134,5 @@
>    LMANNO_FEEDURI: "livemark/feedURI",
>    LMANNO_SITEURI: "livemark/siteURI",
>    LMANNO_EXPIRATION: "livemark/expiration",
>    LMANNO_LOADFAILED: "livemark/loadfailed",
>    LMANNO_LOADING: "livemark/loading",

ugh, please file a bug to remove LMANNO_EXPIRATION, LMANNO_LOADFAILED, LMANNO_LOADING from PlacesUtils, looks like I forgot to do that, and they are unused.

::: toolkit/components/places/nsNavBookmarks.cpp
@@ -1675,5 @@
> -  }
> -
> -  // generate a new GUID base for this session
> -  nsCOMPtr<nsIUUIDGenerator> uuidgen =
> -    do_GetService("@mozilla.org/uuid-generator;1");

you can probably remove "nsIUUIDGenerator.h" from the included headers

Comment 7

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #5)
> We have a tests/migration folder for those tests, basically we have an old
> db there and we update it, checking expected has has been removed/added.

How are those old-version binary files generated?

Comment 8

[Marco Bonardo [:mak]]

well initially using the original version of the browser, I then made some change with SQLite manager.
Btw, you should just init a new browser profile before making the change. it will generate a 10MB DB due to chunked growth, add what you need. Then just open it in sqlite and vacuum to reshrink it.

Comment 9

[:Paolo Amadini]

Attachment: The patch

So I generated a v19 database on a new Nightly profile and added a legacy GUID
annotation manually, testing that it is removed on upgrade.

This patch should be pushed only after the related comm-central changes.

Comment 10

[Marco Bonardo [:mak]]

is there a bug for the comm-central stuff?

Comment 11

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #10)
> is there a bug for the comm-central stuff?

Now there is :-)

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 611774 [details] [diff] [review]
The patch

Review of attachment 611774 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/Database.cpp
@@ +764,5 @@
> +        rv = MigrateV20Up();
> +        NS_ENSURE_SUCCESS(rv, rv);
> +      }
> +
> +      // Firefox 14 uses schema version 20.

wooo, we are already at 20! I'm so happy now all of this is no more in nsNavHistory.cpp...

@@ +1853,5 @@
> +  // Remove obsolete bookmark GUID annotations.
> +  nsCOMPtr<mozIStorageStatement> deleteOldBookmarkGUIDAnnosStmt;
> +  nsresult rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "DELETE FROM moz_items_annos WHERE anno_attribute_id IN("
> +      "SELECT id FROM moz_anno_attributes "

there can be only one id, so that should not be an "IN(" but an "= ("

@@ +1856,5 @@
> +    "DELETE FROM moz_items_annos WHERE anno_attribute_id IN("
> +      "SELECT id FROM moz_anno_attributes "
> +      "WHERE name = :anno_guid"
> +    ")"
> +  ), getter_AddRefs(deleteOldBookmarkGUIDAnnosStmt));

you must NS_ENSURE_STATE the statement

::: toolkit/components/places/tests/migration/test_current_from_v19.js
@@ +33,5 @@
> +  + "WHERE anno_attribute_id = ( "
> +  +   "SELECT id "
> +  +   "FROM moz_anno_attributes "
> +  +   "WHERE name = :attr_name "
> +  + ") "

JOIN

@@ +37,5 @@
> +  + ") "
> +  );
> +  stmt.params.attr_name = kGuidAnnotationName;
> +  do_check_true(stmt.executeStep());
> +  do_check_false(stmt.executeStep());

hm, why not using a count(*) then? Or SELECT ... GROUP BY anno_attribute_id HAVING count(*) > 1 and do_check_false(executeStep())

@@ +50,5 @@
> +
> +add_test(function test_bookmark_guid_annotation_removed()
> +{
> +  let stmt = DBConn().createStatement(
> +    "SELECT COUNT(1) "

nit: we usually use lowercase count, using count(1) has no perf win vs count(*), so I tend to prefer the latter

@@ +56,5 @@
> +  + "WHERE anno_attribute_id = ( "
> +  +   "SELECT id "
> +  +   "FROM moz_anno_attributes "
> +  +   "WHERE name = :attr_name "
> +  + ") "

JOIN

::: toolkit/components/places/tests/unit/test_preventive_maintenance.js
@@ +122,5 @@
> +    let stmt = mDBConn.createStatement("INSERT INTO moz_anno_attributes (name) VALUES (:anno)");
> +    stmt.params['anno'] = this._obsoleteWeaveAttribute;
> +    stmt.execute();
> +    stmt.finalize();
> +    stmt = mDBConn.createStatement("INSERT INTO moz_annos (place_id, anno_attribute_id) VALUES(:place_id, (SELECT id FROM moz_anno_attributes WHERE name = :anno))");

needs indentation love

@@ +154,5 @@
> +    this._placeId = addPlace();
> +    // Add a bookmark.
> +    this._bookmarkId = addBookmark(this._placeId);
> +    // Add an obsolete attribute.
> +    let stmt = mDBConn.createStatement("INSERT INTO moz_anno_attributes (name) VALUES (:anno)");

in SQLite 3.7.11 (that we use in Nightly) you can now do multiple inserts at once using VALUES (value1),(value2),(value3), should simplify this code.

@@ +164,5 @@
> +    stmt.reset();
> +    stmt.params['anno'] = this._obsoleteWeaveAttribute;
> +    stmt.execute();
> +    stmt.finalize();
> +    stmt = mDBConn.createStatement("INSERT INTO moz_items_annos (item_id, anno_attribute_id) VALUES(:item_id, (SELECT id FROM moz_anno_attributes WHERE name = :anno))");

this needs indentation love

@@ +181,5 @@
> +  },
> +
> +  check: function() {
> +    // Check that the obsolete annotations have been removed.
> +    let stmt = mDBConn.createStatement("SELECT id FROM moz_anno_attributes WHERE name = :anno");

may as well use OR here and bind all of three, you care that none of those exists.

Comment 13

[:Paolo Amadini]

Attachment: Cleaned up

Maybe you can double-check indentation and such before I land the patch?

Comment 14

[:Paolo Amadini]

(In reply to Paolo Amadini [:paolo] from comment #13)
> Maybe you can double-check indentation and such before I land the patch?

Oh, and I also added a few missing rv checks and updated the migration test.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 613254 [details] [diff] [review]
Cleaned up

Review of attachment 613254 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/Database.cpp
@@ +1861,5 @@
> +      "SELECT id FROM moz_anno_attributes "
> +      "WHERE name = :anno_guid"
> +    ")"
> +  ), getter_AddRefs(deleteOldBookmarkGUIDAnnosStmt));
> +  NS_ENSURE_SUCCESS(rv, rv);

nit: we usually NS_ENSURE_STATE the stmt, and define rv later.

Comment 16

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #15)
> nit: we usually NS_ENSURE_STATE the stmt, and define rv later.

Hm, that masks the exception's real error code however, is it intentional?

In fact, I looked up NS_ENSURE_STATE before making the change and found it under
the header "Macros for checking state and arguments upon entering interface
boundaries". It looks like it's usually used like this in the code base
(typically, to check a member variable before entering a method), with just a
few exceptions:

http://mxr.mozilla.org/comm-central/search?string=NS_ENSURE_STATE

So even if we want to mask the error code, the originally intended way to do that
is probably doing NS_ENSURE_TRUE(stmt, NS_ERROR_UNEXPECTED) explicitly.

Comment 17

[:Paolo Amadini]

By the way, it would be invaluable to have a single go-to place (MDN page) for
all the debug-build assertions, abort conditions, and XPCOM argument/result/state
checks, both old and new.

Comment 18

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini [:paolo] from comment #16)
> (In reply to Marco Bonardo [:mak] from comment #15)
> > nit: we usually NS_ENSURE_STATE the stmt, and define rv later.
> 
> Hm, that masks the exception's real error code however, is it intentional?

Not completely intentional, but we are not really much interested in differentiating the failure there, it is just a bad failure and one has to debug it regardless.  Also there are rare cases where you may get a success rv and a null statement, that we are looking into.  Btw not a so specific reason (thus the nit), sometimes it's handy to avoid definiting a single nsresult.

Comment 19

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05aa7f1962a3

Comment 20

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/05aa7f1962a3