614790 - Bookmarks roots init could be locking with the first visit addition

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

roots are read from the database on startup, around the same time the first visit is added.
Since all database accessed are serialized, this can end up being a problem.

We cannot initialize roots asynchronously though, since not all the APIs are asynchronous. A possible solution is to clone the connection and read them from the cloned connection.

Comment 1

[Marco Bonardo [:mak]]

this blocks Places branche merge since it's one of the causes of our Ts regression.

it is already tested by bookmarks tests.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 3

[Marco Bonardo [:mak]]

I'm waiting try results regarding Ts on all platforms for this bug and bug 614787

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 493240 [details] [diff] [review]
patch v1.0

I prefer waiting for this review, while it's correct it's possible I have to do further enhancements for locking.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.1

This also delays the creation of the bookmarks service, since we don't need it on init.

I also have a patch to make history serve cloned connections, doesn't look like it's needed, but has some interesting side effect, like having a single point to handle creation, setup and closing of the clones (http://hg.mozilla.org/try/rev/63ff36bc8fba), do you think it's worth filing a bug or we don't want to follow such a way?

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> I also have a patch to make history serve cloned connections, doesn't look like
> it's needed, but has some interesting side effect, like having a single point
> to handle creation, setup and closing of the clones
> (http://hg.mozilla.org/try/rev/63ff36bc8fba), do you think it's worth filing a
> bug or we don't want to follow such a way?
We should file a bug for it, but I think we should punt on fixing it until after Firefox 4.0.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 493511 [details] [diff] [review]
patch v1.1

r=sdwilsh

Comment 8

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/c5c6f317e9b7

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> http://hg.mozilla.org/projects/places/rev/c5c6f317e9b7

http://hg.mozilla.org/mozilla-central/rev/c5c6f317e9b7