319455 - Expire annotations

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Brett Wilson]

Come up with a system for annotation expiration, and implement it.

Comment 1

[Brett Wilson]

Also, delete annotations when pages get deleted or expired from history.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

nsIAnnotationService looks like it has a system for expiring annotations now.  What that system doesn't yet have, however, is a way to specify that an annotation should expire when the bookmark or history entry for that annotation is deleted.  So there's still some more to do here.

Comment 3

[Brett Wilson]

The current system is not necessarily correct, I didn't put much thought into it, and it is 0% implemented in the service.

Comment 4

[Brett Wilson]

Wow, we should have a party if/when this actually gets implemented!

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: wip patch

implements the existing policy, w/ numbers pulled out of thin air.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Dietrich, will you be including the "delete annotations when pages get deleted or expired from history" policy in the patch for this bug, or should I file a separate bug on that?  FWIW, I think that's actually going to be the most popular policy, as extension developers will want their annotation to stick around as long as the user might access the history item.

Comment 7

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)
> Dietrich, will you be including the "delete annotations when pages get deleted
> or expired from history" policy in the patch for this bug, or should I file a
> separate bug on that?  FWIW, I think that's actually going to be the most
> popular policy, as extension developers will want their annotation to stick
> around as long as the user might access the history item.
> 

Hi Myk, are you asking about immediate removal, or eventual removal? This patch does enable removal of annotations that are not linked to a place id. However, it only occurs at shutdown and user-initiated clearing of history.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

I'm asking about immediate removal.  But perhaps eventual removal is sufficient.

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #8)
> I'm asking about immediate removal.  But perhaps eventual removal is
> sufficient.
> 

given that no dangling annotations should be accessible via the annotation apis, the effect should be equivalent.

however, if someone was querying the database directly, and not joining the proper tables, they would still be accessible.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> given that no dangling annotations should be accessible via the annotation
> apis, the effect should be equivalent.

Ah, cool, in that case eventual removal is fine.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: more

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: more

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: more

per myk's comment, this now removes annotations immediately if a moz_places entry is deleted.

Comment 14

[Asaf Romano (gone)]

Note moz_places entries are not removed for bookmarked urls. Maybe we should expire page-annotations when removing all visits for a given moz_place?

We also need another expiration type which wouldn't do that at all (EXPIRE_MANUAL_ONLY?), for annotations like "starred". We'll then need to keep  the the moz_places entry around as well (as we do for bookmarks).

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to comment #14)
> Note moz_places entries are not removed for bookmarked urls. Maybe we should
> expire page-annotations when removing all visits for a given moz_place?
> 
> We also need another expiration type which wouldn't do that at all
> (EXPIRE_MANUAL_ONLY?), for annotations like "starred". We'll then need to keep 
> the the moz_places entry around as well (as we do for bookmarks).
> 

right. currently, EXPIRE_NEVER doesn't actually mean *never*, it means zero visits, which is misleading.

maybe it would make sense to make EXPIRE_NEVER mean "manual removal", and have the default be EXPIRE_VISITS, which are cleared when history is cleared (eg: a place has zero visits). this would cover both of the issues you raised.

Comment 16

[Dietrich Ayala (:dietrich)]

this is not causing hangs, freezes or extreme-brokeness of critical features, so pushing to B1.

Comment 17

[(not reading, please use [e-mail] instead)]

Attachment: dietrich, can you work this in to your MigrateV6Up()?

Comment 18

[Dietrich Ayala (:dietrich)]

Attachment: more

- supports removePagesFromHost
- makes EXPIRE_NEVER require manual removal
- adds EXPIRE_WITH_HISTORY
- adds more tests

(doesn't have the index removal ride-along patch merged in yet)

Comment 19

[Marco Bonardo [:mak]]

sorry, don't know if you have already fixed that but

in nsNavHistoryExpire::EraseAnnotations

+  for (PRUint32 i = 0; i < aRecords.Length(); i ++) {
+    // delete annotations (except EXPIRE_NEVER)
+    nsCOMPtr<mozIStorageStatement> statement;
+    nsresult rv = aConnection->CreateStatement(NS_LITERAL_CSTRING(
+      "DELETE FROM moz_annos WHERE id in (SELECT a.id from moz_annos a JOIN moz_places p on a.place_id = p.id "

i think that the create statement should be out of the for cycle, only the bind inside


and in  nsNavHistory::RemovePagesFromHost

+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
+    "DELETE FROM moz_places WHERE id IN (") + deletedPlaceIds +
+      NS_LITERAL_CSTRING(") AND id NOT IN (") + deletedPlaceIdsBookmarked +
+      NS_LITERAL_CSTRING(") AND id NOT IN (") + deletedPlaceIdsWithAnno +
+      NS_LITERAL_CSTRING(")"), getter_AddRefs(statement));
   NS_ENSURE_SUCCESS(rv, rv);
   rv = statement->Execute();
   NS_ENSURE_SUCCESS(rv, rv);

this don't need to bind anything, so probably ExecuteSimpleSQL could be faster

Comment 20

[Marco Bonardo [:mak]]

Also, please check Bug 386711, i've rewritten ExpireVisits, ExpireHistory, ExpireFavicons to use a ExecuteSimpleSQL instead of a loop with statement.

My patch includes changes to use placeID in those functions instead of pageID, so you could remove that fix from your patch, and land my patch after yours... or you could merge that patch into your work. 
I've tried to follow your naming conventions.

Comment 21

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

- includes the index removal ride-along patch
- fixes comment #19

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 272860 [details] [diff] [review]
fix v1

Marco, could you also take a look at this patch if you are able?

Comment 23

[Marco Bonardo [:mak]]

what i still don't understand is the choice to not use sqlite datetime functions, it could be easy to use datatime('now') as default value for a column, or date('now','start of month') or date('now','-3 days') to compute expiration. 
Is it a mozilla development choice? i have not seen an explicit OMIT clause in sqlite defines, mozStorage could do the conversion between db and internal date formats with binding...


however, let's see the patch


on create table you do

+        "dateAdded INTEGER," 
+        "lastModified INTEGER)"));

then on MigrateV6Up

+    "ALTER TABLE moz_annos ADD dateAdded INTEGER DEFAULT 0"));
+    "ALTER TABLE moz_annos ADD lastModified INTEGER DEFAULT 0"));

should defaut values (0) be present or not? the same is for moz_annos and moz_items_annos



in  nsNavHistory::InitMemDB() there is

       "DELETE FROM moz_historyvisits WHERE place_id IN (SELECT id FROM moz_places WHERE url = ?1)"),

then you do

+  rv = GetUrlIdFor(aURI, &placeId, PR_TRUE);

if you put this before, you get the place_id, so you can call a 
DELETE FROM moz_historyvisits WHERE place_id = ?1
without the subquery. 
why GetUrlIdFor does contain a transaction? it makes a select and (in case of non existance) an insert... i can't see why use a transaction there with only one insert...



in nsNavHistory::RemovePagesFromHost

+  conditionString.AppendLiteral("AND (b.type = ?3 OR b.type IS NULL) ");
+  conditionString.AppendLiteral("AND (a.expiration = ?4 OR a.expiration IS 
NULL) ");

imho is better check existance against id (so b.id IS NULL, a.id IS NULL), id are always indexed so it could be faster, and id can never be null (a bug could instead create a b.type null on an existant bookmark)



+#define EXPIRATION_POLICY_DAYS    3 * 86400
why 3 days? why not 7 days?



nsNavHistoryExpire::ExpireAnnotations should get a transaction?



+    "DELETE FROM moz_anno_attributes WHERE " 
+    "id NOT IN (SELECT a.id FROM moz_anno_attributes a "
+      "JOIN moz_annos b ON b.anno_attribute_id = a.id "
+      "JOIN moz_places p ON b.place_id = p.id GROUP BY a.id) "
+    "AND "
+    "id NOT IN (SELECT a.id FROM moz_anno_attributes a "
+      "JOIN moz_items_annos c ON c.anno_attribute_id = a.id "
+      "JOIN moz_bookmarks p ON c.item_id = p.id GROUP BY a.id)"));

You can use SELECT DISTINCT here instead of group by clauses, because you select only 1 column


I can't see anything more here now, hope that helps

Comment 24

[(not reading, please use [e-mail] instead)]

some partial review comments, I'll contine my review tomorrow:

1)

+  // if dateAdded & lastModified cols are already there, then a partial update occurred.
+  // return, making no changes, and allowing db version to be updated.
+  nsCOMPtr<mozIStorageStatement> statement;
+  nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
+    "SELECT a.dateAdded, a.lastModified, b.dateAdded, b.lastModified "
+    "FROM moz_annos a, moz_items_annos b"), getter_AddRefs(statement));
+  if (NS_SUCCEEDED(rv))
+    return NS_OK;


I was hoping there was a easy way to see if a column exists, but I haven't figured one out.
You come up with something complicated using:

PRAGMA table_info(moz_annos)

...and then look at the second values for dateAdded or lastModified.  but this seems fragile and hackish.

Or

select sql from sqlite_master where type = "table" and name="moz_annos"

and look for dateAdded and lastModified in the string.  again, hackish.

I believe this should be faster than your query, however:

SELECT exists (SELECT a.dateAdded, a.lastModified, b.dateAdded, b.lastModified FROM moz_annos a, moz_items_annos b)

(Marco, do you agree?)

2)

+  // remove days annos
+  nsCAutoString days_query = NS_LITERAL_CSTRING(
+    "DELETE FROM moz_annos WHERE expiration = ") +
+      nsPrintfCString("%i", nsIAnnotationService::EXPIRE_DAYS) +
+      NS_LITERAL_CSTRING(" AND (") + nsPrintfCString("%lld", PR_Now()) + 
+      NS_LITERAL_CSTRING(" - dateAdded) > ") +
+      nsPrintfCString("%i", EXPIRATION_POLICY_DAYS);
+  nsresult rv = aConnection->ExecuteSimpleSQL(days_query);
+  NS_ENSURE_SUCCESS(rv, rv);

I am trying to find out where I read this, but it makes sense:  don't do calculations in queries if you can avoid them.

So intead of DELETE FROM moz_annos WHERE expiration = nsIAnnotationService::EXPIRE_DAYS and (PR_Now() - dateAdded) > EXPIRATION_POLICY_DAYS)

This should be:


DELETE FROM moz_annos WHERE expiration = nsIAnnotationService::EXPIRE_DAYS and (PR_Now() - dateAdded) > EXPIRATION_POLICY_DAYS)

PRTime now = PR_Now();  //  better than calling it 5 times, a trivial gain, more worth noting for future usage.
PRTime expireDate = now - EXPIRATION_POLICY_DAYS;

And then do "DELETE FROM moz_annos WHERE expiration = nsIAnnotationService::EXPIRE_DAYS and (dateAdded < expireDate)"

3)


+// Expiration policy amounts (in days)
+#define EXPIRATION_POLICY_DAYS    3 * 86400
+#define EXPIRATION_POLICY_WEEKS   30 * 86400
+#define EXPIRATION_POLICY_MONTHS  180 * 86400

wait, isn't 86400 the number of seconds in a day, but PR_Now() is in microseconds, right.  

Comment 25

[(not reading, please use [e-mail] instead)]

> I was hoping there was a easy way to see if a column exists, but I haven't
figured one out.
> ...
> (Marco, do you agree?)

actually, we could use sdwilsh's getColumnIndex() from bug #388059 to do this.

Comment 26

[(not reading, please use [e-mail] instead)]

> actually, we could use sdwilsh's getColumnIndex() from bug #388059 to do this.

actually, that would require a statement anyways, so I don't think that is any better than "SELECT exists (SELECT a.dateAdded, a.lastModified, b.dateAdded, b.lastModified FROM moz_annos a, moz_items_annos b)"

Comment 27

[Shawn Wilsher :sdwilsh]

It sounds like columnExists could be a useful addition to mozIStorageConnection, no?

Comment 28

[Marco Bonardo [:mak]]

if i'm not wrong to find if columns exists dietrich creates a statement, but it does not execute it. The engine should check that the query is valid without executing, and throws an error if it's not... it sounds faster than doing any query on the db, hwv some test could be done

Comment 29

[Shawn Wilsher :sdwilsh]

Yes, I'm 90% certain that if you try to create a statement that contains columns that don't exists, the creation will fail.

Comment 30

[Dietrich Ayala (:dietrich)]

fixed all the other issues not specifically responded to below.

(In reply to comment #23)
> what i still don't understand is the choice to not use sqlite datetime
> functions, it could be easy to use datatime('now') as default value for a
> column, or date('now','start of month') or date('now','-3 days') to compute
> expiration. 
> Is it a mozilla development choice? i have not seen an explicit OMIT clause in
> sqlite defines, mozStorage could do the conversion between db and internal date
> formats with binding...

the pre-existing places code did this, presumably to avoid any portability issues between moz cross-platform handling of PRTime/PRNow and sqlite. however, it'd be great if we could use the sqlite datetime functions, so worth following this up in another bug.

> 
> should defaut values (0) be present or not? the same is for moz_annos and
> moz_items_annos
> 

fixed, added defaults

> 
> 
> in  nsNavHistory::InitMemDB() there is
> 
>        "DELETE FROM moz_historyvisits WHERE place_id IN (SELECT id FROM
> moz_places WHERE url = ?1)"),
> 
> then you do
> 
> +  rv = GetUrlIdFor(aURI, &placeId, PR_TRUE);
> 
> if you put this before, you get the place_id, so you can call a 
> DELETE FROM moz_historyvisits WHERE place_id = ?1
> without the subquery. 
> why GetUrlIdFor does contain a transaction? it makes a select and (in case of
> non existance) an insert... i can't see why use a transaction there with only
> one insert...
> 

fixed. and i removed the transaction. if adding a new place entry ever does require more than one insert, the transaction should be added to InternalAddNewPage() anyways.

> 
> +#define EXPIRATION_POLICY_DAYS    3 * 86400
> why 3 days? why not 7 days?

sure, that makes sense given the other week-rounded periods. we can tweak these values through the betas, as we get feedback on usage.

> 
> 
> I can't see anything more here now, hope that helps
> 

very much, thanks!

Comment 31

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

fixed seth and marco's comments.

Comment 32

[Shawn Wilsher :sdwilsh]

Deitrich - you may want to drop any columns you don't need in this schema change as well.  I just did this in Bug 385875 for the download manager.

Comment 33

[Dietrich Ayala (:dietrich)]

(In reply to comment #32)
> Deitrich - you may want to drop any columns you don't need in this schema
> change as well.  I just did this in Bug 385875 for the download manager.
> 

thanks shawn, good suggestion. i'll fix bug 371800 as a ride-along. we especially need to do this before bug 332748 gets fixed, after which a table copy like this will take a lot longer (for those w/ default history settings anyway).

Comment 34

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

- revs anno service uuid
- includes fix to bug 371800

Comment 35

[(not reading, please use [e-mail] instead)]

Comment on attachment 273545 [details] [diff] [review]
fix v3

r=sspitzer, but a few comments / questions:


1)

for the moz_annos_items table, why are dateAdded and lastModified not defaulted to 0?

+        "type INTEGER DEFAULT 0,"
+        "dateAdded INTEGER," 
+        "lastModified INTEGER)"));

2)


-                              data->voidString,

You can also remove this from nsMorkHistoryImporter.cpp:

  // A voided string to use for the user title
  
  const nsString voidString;


3)

+  // we used to create an indexes on moz_favicons.url and
+  // moz_anno_attributes.name, but those indexes are not needed
+  // because those columns are UNIQUE, so remove them.
+  // see bug #386303 for more details
+  rv = aDBConn->ExecuteSimpleSQL(
+    NS_LITERAL_CSTRING("DROP INDEX IF EXISTS moz_favicons_url"));
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = aDBConn->ExecuteSimpleSQL(
+    NS_LITERAL_CSTRING("DROP INDEX IF EXISTS moz_anno_attributes_nameindex"));
+  NS_ENSURE_SUCCESS(rv, rv);

Thanks!

4)

+    // 2. create moz_places w/o user_title, and it's index


it's -> its

5)

+    if (NS_FAILED(rv)) {
+      nsCAutoString e;
+      mDBConn->GetLastErrorString(e);
+      printf("SQL ERROR: %s\n", PromiseFlatCString(e).get());
+    }

remove or wrap with #ifdef DEBUG

6)

+  // is bookmarked?
+  conditionString.AppendLiteral("AND (b.type = ?3 OR b.id IS NULL) ");
+
+  // has EXPIRES_NEVER annotations?
+  conditionString.AppendLiteral("AND (a.expiration = ?4 OR a.id IS NULL) ");


The "is NULL" part, a question (to make sure I understand):

This is because we are doing a LEFT OUTER JOIN with moz_bookmarks and moz_annos, right?

7)

+  nsCAutoString getURIsForDeletion = NS_LITERAL_CSTRING(
+    "SELECT h.id, h.url, b.id, a.id FROM moz_places h "

Why are we getting out h.url?  It seems like we only care about h.id, b.id, and a.id, no?

8)

+    if (deletedURIs.Count() > 1) {
+      deletedPlaceIds.AppendLiteral(", ");
+      deletedPlaceIdsBookmarked.AppendLiteral(", ");
+      deletedPlaceIdsWithAnno.AppendLiteral(", ");
+    }
 
+    PRInt64 placeId;
+    rv = statement->GetInt64(0, &placeId);
+    NS_ENSURE_SUCCESS(rv, rv);
+    deletedPlaceIds.AppendInt(placeId);
+    if (statement->AsInt64(2))
+      deletedPlaceIdsBookmarked.AppendInt(placeId);
+    if (statement->AsInt64(3))
+      deletedPlaceIdsWithAnno.AppendInt(placeId);

That looks like you are going to be appending ", " every time even if you aren't appending the placeId ever time, right?

8)

nsNavHistoryExpire::EraseAnnotations()

should you be using a transaction, like you do in nsAnnotationService::ExpireAnnotations()?

9)

+  // remove days item annos
...
+  // remove days item annos

first comment should be "// remove days annos"

10)

Do you think it would be worth writing a helper function CreateExpirationQuery() so we could do this:

// remove months annos
nsCAutoString months_query = CreateExpirationQuery("moz_annos", nsIAnnotationService::EXPIRE_MONTHS, now - EXPIRATION_POLICY_MONTHS);
rv = aConnection->ExecuteSimpleSQL(months_query);
NS_ENSURE_SUCCESS(rv, rv);

// remove months item annos
nsCAutoString item_months_query = CreateExpirationQuery("moz_annos_items", nsIAnnotationService::EXPIRE_MONTHS, now - EXPIRATION_POLICY_MONTHS);
rv = aConnection->ExecuteSimpleSQL(item_months_query);
NS_ENSURE_SUCCESS(rv, rv);

that seems better than copying the query generation code six times.

Comment 36

[Dietrich Ayala (:dietrich)]

> 6)
> 
> +  // is bookmarked?
> +  conditionString.AppendLiteral("AND (b.type = ?3 OR b.id IS NULL) ");
> +
> +  // has EXPIRES_NEVER annotations?
> +  conditionString.AppendLiteral("AND (a.expiration = ?4 OR a.id IS NULL) ");
> 
> 
> The "is NULL" part, a question (to make sure I understand):
> 
> This is because we are doing a LEFT OUTER JOIN with moz_bookmarks and
> moz_annos, right?

exactly. we want to match only items that:

- are bookmarked URIs or not bookmarked at all
- have DELETE_NEVER annotations, or are not annotated at all

these are sorted below, and then used to generate strings for in/exclusion in the delete queries.

> 
> 7)
> 
> +  nsCAutoString getURIsForDeletion = NS_LITERAL_CSTRING(
> +    "SELECT h.id, h.url, b.id, a.id FROM moz_places h "
> 
> Why are we getting out h.url?  It seems like we only care about h.id, b.id, and
> a.id, no?

it is required to send out notifications later in the method.

> 8)
> 
> nsNavHistoryExpire::EraseAnnotations()
> 
> should you be using a transaction, like you do in
> nsAnnotationService::ExpireAnnotations()?
> 

The Erase* methods are always called in the context of an existing transaction. ExpireAnnotations has various callers, not all wrapping it in a transaction.


> 10)
> 
> Do you think it would be worth writing a helper function
> CreateExpirationQuery() so we could do this:
> 
> // remove months annos
> nsCAutoString months_query = CreateExpirationQuery("moz_annos",
> nsIAnnotationService::EXPIRE_MONTHS, now - EXPIRATION_POLICY_MONTHS);
> rv = aConnection->ExecuteSimpleSQL(months_query);
> NS_ENSURE_SUCCESS(rv, rv);
> 
> // remove months item annos
> nsCAutoString item_months_query = CreateExpirationQuery("moz_annos_items",
> nsIAnnotationService::EXPIRE_MONTHS, now - EXPIRATION_POLICY_MONTHS);
> rv = aConnection->ExecuteSimpleSQL(item_months_query);
> NS_ENSURE_SUCCESS(rv, rv);
> 
> that seems better than copying the query generation code six times.
> 

i replaced it with statements.

Comment 37

[Dietrich Ayala (:dietrich)]

Attachment: fix v4

- fixes seth's review comments
- adds the index for bug 389492

Comment 38

[Dietrich Ayala (:dietrich)]

Attachment: v5 - cleaned up

Comment 39

[(not reading, please use [e-mail] instead)]

Comment on attachment 273787 [details] [diff] [review]
v5 - cleaned up

r=sspitzer

to avoid prepended commas, instead of:

+    if (deletedURIs.Count() > 1)
+      deletedPlaceIds.AppendLiteral(", ");
 

+    deletedPlaceIds.AppendInt(placeId);
+    if (statement->AsInt64(2)) {
+      deletedPlaceIdsBookmarked.AppendLiteral(", ");
+      deletedPlaceIdsBookmarked.AppendInt(placeId);
+    }
+    if (statement->AsInt64(3)) {
+      deletedPlaceIdsWithAnno.AppendLiteral(", ");
+      deletedPlaceIdsWithAnno.AppendInt(placeId);
+    }

how about something like:

if (!deletedPlaceIds.Empty())
  deletedPlaceIds.AppendLiteral(", ");
deletedPlaceIds.AppendInt(placeId);

if (statement->AsInt64(2)) {
  if (!deletedPlaceIdsBookmarked.Empty())
    deletedPlaceIdsBookmarked.AppendLiteral(", ");
  deletedPlaceIdsBookmarked.AppendInt(placeId);
}
if (statement->AsInt64(3)) {
  if (!deletedPlaceIdsWithAnno.Empty())
    deletedPlaceIdsWithAnno.AppendLiteral(", ");
  deletedPlaceIdsWithAnno.AppendInt(placeId);
}

Comment 40

[Mike Connor [:mconnor]]

Missing the freeze, moving out.

Comment 41

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/public/nsIAnnotationService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsIAnnotationService.idl,v  <--  nsIAnnotationService.idl
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/components/places/public/nsINavHistoryService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v  <--  nsINavHistoryService.idl
new revision: 1.64; previous revision: 1.63
done
Checking in toolkit/components/places/src/nsAnnotationService.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsAnnotationService.cpp,v  <--  nsAnnotationService.cpp
new revision: 1.29; previous revision: 1.28
done
Checking in toolkit/components/places/src/nsAnnotationService.h;
/cvsroot/mozilla/toolkit/components/places/src/nsAnnotationService.h,v  <--  nsAnnotationService.h
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/components/places/src/nsMorkHistoryImporter.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsMorkHistoryImporter.cpp,v  <--  nsMorkHistoryImporter.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.114; previous revision: 1.113
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.145; previous revision: 1.144
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.87; previous revision: 1.86
done
Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/components/places/src/nsNavHistoryExpire.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.h,v  <--  nsNavHistoryExpire.h
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.108; previous revision: 1.107
done
Checking in toolkit/components/places/src/nsNavHistoryResult.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v  <--  nsNavHistoryResult.h
new revision: 1.44; previous revision: 1.43
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_browserhistory.js,v
done
Checking in toolkit/components/places/tests/unit/test_browserhistory.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_browserhistory.js,v  <--  test_browserhistory.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_expiration.js,v
done
Checking in toolkit/components/places/tests/unit/test_expiration.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_expiration.js,v  <--  test_expiration.js
initial revision: 1.1
done
Checking in toolkit/components/places/tests/unit/test_history.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_history.js,v  <--  test_history.js
new revision: 1.6; previous revision: 1.5
done

Comment 42

[Steve Lyon]

(In reply to comment #14)
> Note moz_places entries are not removed for bookmarked urls. 

I don't know if this falls into the realm of this update, but it seems to fit based on the comment above. There is an "issue" with trying to delete things from the URL bar autocomplete list. If the item has been bookmarked you can't get it removed from the autocomplete list. From reading the comments following what I quoted here, it seemed maybe this would fix that...but it doesn't. I just want to be able to maintain my autocomplete list independent of bookmarks as a quick way to go to websites.

Comment 43

[Ria Klaassen (not reading all bugmail)]

This bug caused my browser not to start up anymore. When I delete places.sqlite it starts up.

Comment 44

[Frederic Bezies]

Sorry to spam this bug, but I think this patch is also related to bug 389808, making personal toolbar blank.

Comment 45

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #43)
> This bug caused my browser not to start up anymore. When I delete places.sqlite
> it starts up.
>
Ehm, due to my "beautiful English" this sounds like an accusation, but it was not meant as an accusation nor as spam, more as a contribution, hopefully useful ;) 

Comment 46

[Dietrich Ayala (:dietrich)]

Ria, do you have the original places.sqlite or places.sqlite.corrupt file from that profile?

Comment 47

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/2007072608 Minefield/3.0a7pre ID:2007072608

FWIW I am seeing the same as Ria. Firefox starts up fine with 20070726_0850_firefox-3.0a7pre.en-US.win32.zip but not with 20070726_0939_firefox-3.0a7pre.en-US.win32.zip.

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1185465000&maxdate=1185467939

I'm trying to figure out what in my places.sqlite is causing this. (I thought it might be microsummaries, but apparently not)

Comment 48

[Dietrich Ayala (:dietrich)]

Do you have a lot of history? And by "not start up", do you mean that it hangs, or that the process starts and then exits?

Bug 371800 was fixed in this patch, and that does a full copy of the history table, which might take a while if you have a lot of history.

Comment 49

[Steve England [:stevee]]

I made a new profile and copied my normal places.sqlite into it. This new profile opens fine with 20070726_0850_firefox-3.0a7pre.en-US.win32.zip but not with 20070726_0939_firefox-3.0a7pre.en-US.win32.zip. (Firefox doesn't start up, CPU pegged at 99%, memory increasing by ~100k/s. After 5 minutes waiting i got bored and killed the process)

If I then open the new profile with 20070726_0850_firefox-3.0a7pre.en-US.win32.zip, clear my history (set to 45 days in my normal profile), then close firefox down and reopen the same profile with 20070726_0939_firefox-3.0a7pre.en-US.win32.zip, then it works fine.

So yes, it seems history-related. I will let the latest nightly build churn on my usual profile until i get really, really bored.

Comment 50

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/2007072705 Minefield/3.0a7pre ID:2007072705

18 minutes later and now my history has been processed. Firefox now starts up and shuts down like it used to. Perhaps this should be relnote'd for GP Alpha 7?

Comment 51

[Dietrich Ayala (:dietrich)]

Thanks Steve, it does sound like the history table-rebuild is the cause of the delay. Considering that bug 332748 changed the default history-length to 180 days, we decided it was better to do this schema fixup now than later, when this delay would be much worse. I'll get a mention in the release notes.

Comment 52

[Ria Klaassen (not reading all bugmail)]

I see also my history now. It took nearly 5 minutes to appear (about 16 MB file). 

Comment 53

[Ria Klaassen (not reading all bugmail)]

Oh well, haha, just like Frederic my bookmarks toolbar is empty now after a restart. Replaced the localstore.rdf file with a copy but that didn't help. Maybe influenced by an extension? I always suspect Custom Buttons for this kind of problems.
Anyways, I don't get it back. 

Comment 54

[(not reading, please use [e-mail] instead)]

> just like Frederic my bookmarks toolbar is empty now after a restart

a known issue, see bug #389808

Comment 55

[(not reading, please use [e-mail] instead)]

ria and steve:  let's take the performance issue of starting up after this change to bug #389876

Comment 56

[Al Billings [:abillings - ex-MoCo]]

Steve, did you verify the fix other than the performance issues?

Comment 57

[Steve England [:stevee]]

Al, I have verified nothing wrt this bug; I was only concerned that it appeared to cause my firefox to hang, but in reality it was processing my large history file.

Comment 58

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv