607117 - create GUIDs for all bookmark and history items automatically

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Mike Connor [:mconnor]]

Doing this after the fact won't be faster, and with Sync in Fx4, we'll want this in place without having to force it.  bug 607112 and bug 607115 both help us here, as well.

Comment 1

[Shawn Wilsher :sdwilsh]

Also need to add a test for migration v11->v10->v11 with this.

Comment 2

[Shawn Wilsher :sdwilsh]

I'm going to break this up like this:
Part 1 - Make bookmarks generate guids automagically
Part 2 - Make moz_places generate guids automagically
Part 3 - test from comment 1

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - make bookmarks generate guids automagically

Note: Sync will have to roll its own guid setting when it needs to change the guid for now.  We have no API for that until we get the new async one up and running.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 494210 [details] [diff] [review]
Part 1 - make bookmarks generate guids automagically

this was just too easy for you!

But I have comments!
You should also update other inserts, especially the one in PlacesDBUtils.jsm
http://mxr.mozilla.org/mozilla-central/search?string=insert%20into%20moz_bookmarks

>diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks.js b/toolkit/components/places/tests/bookmarks/test_bookmarks.js

>+    do_check_true(/^[a-zA-Z0-9\-_]{12}$/.test(stmt.row.guid));

nit: I'm starting thinking we could need a check_valid_places_guid() in head_common...

r=me with maintenance fixed.

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - make bookmarks generate guids automagically v1.1

Addresses comments, but I'd like you to look one more time.  Also added a shutdown assertion check to make sure we properly add guids always.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - make moz_places entries automagically get guids v1.0

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: Part 3 - migration test v1.0

Adds a test to make sure we properly handle migration from a v10 database that was formerly a v11 database back to v11.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 494458 [details] [diff] [review]
Part 2 - make moz_places entries automagically get guids v1.0

>diff --git a/toolkit/components/places/src/AsyncFaviconHelpers.cpp b/toolkit/components/places/src/AsyncFaviconHelpers.cpp
>--- a/toolkit/components/places/src/AsyncFaviconHelpers.cpp
>+++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp
>@@ -48,6 +48,7 @@
> #include "nsICacheVisitor.h"
> #include "nsICachingChannel.h"
> #include "nsIAsyncVerifyRedirectCallback.h"
>+#include "nsIRandomGenerator.h"
> 
> #include "nsNavHistory.h"
> #include "nsNavBookmarks.h"
>@@ -385,6 +386,12 @@ AsyncFetchAndSetIconForPage::start(nsIUR
>   NS_PRECONDITION(NS_IsMainThread(),
>                   "This should be called on the main thread");
> 
>+  // We need this service to be initialized on the main thread because it is not
>+  // threadsafe.  We are about to use it asynchronously, so initialize it now.
>+  nsCOMPtr<nsIRandomGenerator> rg =
>+    do_GetService("@mozilla.org/security/random-generator;1");
>+  NS_ENSURE_STATE(rg);

I'm thinking... could we make this call when we register the GENERATE_GUID() function?
Looks like a better centralized place to do it rather than doing it in every method using the function. Also because this start function is called for every icon, looks like a useless overhead just to check that a service is inited.

>diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp

>+    // We need this service to be initialized on the main thread because it is
>+    // not threadsafe.  We are about to use it asynchronously, so initialize it
>+    // now.
>+    nsCOMPtr<nsIRandomGenerator> rg =
>+      do_GetService("@mozilla.org/security/random-generator;1");
>+    NS_ENSURE_STATE(rg);
>+

ditto, this is called for each visit

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>@@ -1347,8 +1347,9 @@ nsNavHistory::GetStatement(const nsCOMPt
>   // will fetch the id of the existing entry.
>   RETURN_IF_STMT(mDBAddNewPage, NS_LITERAL_CSTRING(
>     "INSERT OR IGNORE INTO moz_places "
>-      "(url, title, rev_host, hidden, typed, frecency) "
>-    "VALUES (:page_url, :page_title, :rev_host, :hidden, :typed, :frecency) "
>+      "(url, title, rev_host, hidden, typed, frecency, guid) "
>+    "VALUES (:page_url, :page_title, :rev_host, :hidden, :typed, :frecency, "
>+             "GENERATE_GUID()) "
>   ));

I'm a bit worried here for the OR IGNORE if we generate a conflicting guid... but should be pretty rare and we will bail out trying to get the new place id immediately later, right?

I'd just evaluate moving the rg init, the rest looks fine.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 494553 [details] [diff] [review]
Part 3 - migration test v1.0

it's cool!

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> >+++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp
> I'm thinking... could we make this call when we register the GENERATE_GUID()
> function?
> Looks like a better centralized place to do it rather than doing it in every
> method using the function. Also because this start function is called for every
> icon, looks like a useless overhead just to check that a service is inited.
No, because we want NSS to be initialized as late as possible in the startup path.  I originally was going to do it there, but then recalled what bsmedberg had told me about wanting it to be late.

> I'm a bit worried here for the OR IGNORE if we generate a conflicting guid...
> but should be pretty rare and we will bail out trying to get the new place id
> immediately later, right?
ignoring it just means we ignore the failure, but still don't insert anything.  I can change that if you want.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #10)
> No, because we want NSS to be initialized as late as possible in the startup
> path.  I originally was going to do it there, but then recalled what bsmedberg
> had told me about wanting it to be late.

It won't make a difference, since as soon as we start history, it is to register the first visit or the first icon, that will call this code, so the current code is not delaying it. Did he say what's the reasoning for the delay?

> > I'm a bit worried here for the OR IGNORE if we generate a conflicting guid...
> > but should be pretty rare and we will bail out trying to get the new place id
> > immediately later, right?
> ignoring it just means we ignore the failure, but still don't insert anything. 

yes, I meant that we won't insert the required page. As I said I think it's fine for now, it should happen at the same frequency as a guid conflict, that I hope is really rare.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> It won't make a difference, since as soon as we start history, it is to
> register the first visit or the first icon, that will call this code, so the
> current code is not delaying it. Did he say what's the reasoning for the delay?
NSS is expensive to startup :(

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

hm, ok, can we make so that we try to init it only if we are going to add a new page or bookmark? often we add visits for pages that already have a moz_places entry, if so we should not init the service.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> hm, ok, can we make so that we try to init it only if we are going to add a new
> page or bookmark? often we add visits for pages that already have a moz_places
> entry, if so we should not init the service.
But how would we know that on the main thread?  For that matter, getting a service that has already been gotten before is quick, so it shouldn't really hurt us.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Well, as it is this is going to init that service at the first visit addition, always, this could affect Ts, you should get a try measure probably.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> I'd just evaluate moving the rg init, the rest looks fine.
Did this.

(In reply to comment #15)
> Well, as it is this is going to init that service at the first visit addition,
> always, this could affect Ts, you should get a try measure probably.
NSS is already in the startup path; we are just making harder to move it out of it.

Comment 17

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/projects/places/rev/d52263688514
http://hg.mozilla.org/projects/places/rev/790de2a5e9f5
http://hg.mozilla.org/projects/places/rev/b44a01549b60

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/d52263688514
http://hg.mozilla.org/mozilla-central/rev/790de2a5e9f5
http://hg.mozilla.org/mozilla-central/rev/b44a01549b60