If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

create GUIDs for all bookmark and history items automatically

RESOLVED FIXED in mozilla2.0b9

Status

()

Toolkit
Places
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mconnor, Assigned: sdwilsh)

Tracking

unspecified
mozilla2.0b9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [fixed-in-places])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
blocking2.0: --- → betaN+
(Assignee)

Updated

7 years ago
Depends on: 607115
(Assignee)

Updated

7 years ago
Depends on: 607112
(Assignee)

Updated

7 years ago
No longer blocks: 607107
(Assignee)

Comment 1

7 years ago
Also need to add a test for migration v11->v10->v11 with this.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
(Assignee)

Comment 2

7 years ago
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
(Assignee)

Comment 3

7 years ago
Created attachment 494210 [details] [diff] [review]
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.
Attachment #494210 - Flags: review?(mak77)
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.
Attachment #494210 - Flags: review?(mak77) → review+
(Assignee)

Comment 5

7 years ago
Created attachment 494419 [details] [diff] [review]
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.
Attachment #494210 - Attachment is obsolete: true
Attachment #494419 - Flags: review?(mak77)
(Assignee)

Comment 6

7 years ago
Created attachment 494458 [details] [diff] [review]
Part 2 - make moz_places entries automagically get guids v1.0
Attachment #494458 - Flags: review?(mak77)
(Assignee)

Updated

7 years ago
Depends on: 616001
(Assignee)

Comment 7

7 years ago
Created attachment 494553 [details] [diff] [review]
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.
Attachment #494553 - Flags: review?(mak77)

Updated

7 years ago
Attachment #494419 - Flags: review?(mak77) → review+
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.
Attachment #494458 - Flags: review?(mak77) → review+
Comment on attachment 494553 [details] [diff] [review]
Part 3 - migration test v1.0

it's cool!
Attachment #494553 - Flags: review?(mak77) → review+
(Assignee)

Comment 10

7 years ago
(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.
(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.
(Assignee)

Comment 12

7 years ago
(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 :(
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.
(Assignee)

Comment 14

7 years ago
(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.
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.
(Assignee)

Comment 16

7 years ago
(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.
(Assignee)

Comment 17

7 years ago
http://hg.mozilla.org/projects/places/rev/d52263688514
http://hg.mozilla.org/projects/places/rev/790de2a5e9f5
http://hg.mozilla.org/projects/places/rev/b44a01549b60
Whiteboard: [fixed-in-places]
(Assignee)

Updated

7 years ago
Depends on: 617111
http://hg.mozilla.org/mozilla-central/rev/d52263688514
http://hg.mozilla.org/mozilla-central/rev/790de2a5e9f5
http://hg.mozilla.org/mozilla-central/rev/b44a01549b60
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.