Expire annotations

RESOLVED FIXED in Firefox 3 alpha7

Status

()

Firefox
Bookmarks & History
P3
normal
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: Brett Wilson, Assigned: dietrich)

Tracking

Trunk
Firefox 3 alpha7
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

12 years ago
Come up with a system for annotation expiration, and implement it.
(Reporter)

Comment 1

12 years ago
Also, delete annotations when pages get deleted or expired from history.
(Reporter)

Updated

12 years ago
Priority: -- → P3
Target Milestone: --- → Firefox 2 beta1
(Reporter)

Updated

12 years ago
Priority: P3 → P4
(Reporter)

Updated

11 years ago
Priority: P4 → P2
Target Milestone: Firefox 2 beta1 → Firefox 3
(Reporter)

Updated

11 years ago
Priority: P2 → P3
Target Milestone: Firefox 3 → Firefox 3 alpha2
(Reporter)

Updated

11 years ago
Assignee: brettw → nobody
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.
(Reporter)

Comment 3

11 years ago
The current system is not necessarily correct, I didn't put much thought into it, and it is 0% implemented in the service.
Flags: blocking-firefox3?

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+

Updated

10 years ago
Target Milestone: Firefox 3 alpha2 → Firefox 3 alpha6
(Assignee)

Updated

10 years ago
Assignee: nobody → dietrich
(Reporter)

Comment 4

10 years ago
Wow, we should have a party if/when this actually gets implemented!
(Assignee)

Comment 5

10 years ago
Created attachment 268322 [details] [diff] [review]
wip patch

implements the existing policy, w/ numbers pulled out of thin air.
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.
(Assignee)

Comment 7

10 years ago
(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.
I'm asking about immediate removal.  But perhaps eventual removal is sufficient.
(Assignee)

Comment 9

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

Comment 11

10 years ago
Created attachment 269177 [details] [diff] [review]
more
Attachment #268322 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
Created attachment 269178 [details] [diff] [review]
more
Attachment #269177 - Attachment is obsolete: true
(Assignee)

Comment 13

10 years ago
Created attachment 269613 [details] [diff] [review]
more

per myk's comment, this now removes annotations immediately if a moz_places entry is deleted.
Attachment #269178 - Attachment is obsolete: true
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).
(Assignee)

Comment 15

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

Comment 16

10 years ago
this is not causing hangs, freezes or extreme-brokeness of critical features, so pushing to B1.
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
Created attachment 271272 [details] [diff] [review]
dietrich, can you work this in to your MigrateV6Up()?
Blocks: 387166
(Assignee)

Comment 18

10 years ago
Created attachment 271310 [details] [diff] [review]
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)
Attachment #269613 - Attachment is obsolete: true
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
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.
(Assignee)

Updated

10 years ago
Blocks: 386711
(Assignee)

Comment 21

10 years ago
Created attachment 272860 [details] [diff] [review]
fix v1

- includes the index removal ride-along patch
- fixes comment #19
Attachment #271272 - Attachment is obsolete: true
Attachment #271310 - Attachment is obsolete: true
Attachment #272860 - Flags: review?(sspitzer)
(Assignee)

Comment 22

10 years ago
Comment on attachment 272860 [details] [diff] [review]
fix v1

Marco, could you also take a look at this patch if you are able?
Attachment #272860 - Flags: review?(mak77)
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
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.  
> 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.
> 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)"
It sounds like columnExists could be a useful addition to mozIStorageConnection, no?
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
Yes, I'm 90% certain that if you try to create a statement that contains columns that don't exists, the creation will fail.
(Assignee)

Comment 30

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

Comment 31

10 years ago
Created attachment 273279 [details] [diff] [review]
fix v2

fixed seth and marco's comments.
Attachment #272860 - Attachment is obsolete: true
Attachment #273279 - Flags: review?(sspitzer)
Attachment #272860 - Flags: review?(sspitzer)
Attachment #272860 - Flags: review?(mak77)
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.
(Assignee)

Comment 33

10 years ago
(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).
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Attachment #273279 - Flags: review?(sspitzer)
(Assignee)

Updated

10 years ago
Blocks: 371800
(Assignee)

Comment 34

10 years ago
Created attachment 273545 [details] [diff] [review]
fix v3

- revs anno service uuid
- includes fix to bug 371800
Attachment #273279 - Attachment is obsolete: true
Attachment #273545 - Flags: review?(sspitzer)
(Assignee)

Updated

10 years ago
Blocks: 389492
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.
Attachment #273545 - Flags: review?(sspitzer) → review+
(Assignee)

Comment 36

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

Comment 37

10 years ago
Created attachment 273742 [details] [diff] [review]
fix v4

- fixes seth's review comments
- adds the index for bug 389492
Attachment #273545 - Attachment is obsolete: true
Attachment #273742 - Flags: review?(sspitzer)
(Assignee)

Comment 38

10 years ago
Created attachment 273787 [details] [diff] [review]
v5 - cleaned up
Attachment #273742 - Attachment is obsolete: true
Attachment #273787 - Flags: review?(sspitzer)
Attachment #273742 - Flags: review?(sspitzer)
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);
}
Attachment #273787 - Flags: review?(sspitzer) → review+
Missing the freeze, moving out.
Target Milestone: Firefox 3 M7 → Firefox 3 M8
(Assignee)

Comment 41

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3 M8 → Firefox 3 M7

Comment 42

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

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

Comment 44

10 years ago
Sorry to spam this bug, but I think this patch is also related to bug 389808, making personal toolbar blank.
(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 ;) 

(Assignee)

Comment 46

10 years ago
Ria, do you have the original places.sqlite or places.sqlite.corrupt file from that profile?
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)
(Assignee)

Comment 48

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

Comment 51

10 years ago
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.
I see also my history now. It took nearly 5 minutes to appear (about 16 MB file). 
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. 
Blocks: 389808
> just like Frederic my bookmarks toolbar is empty now after a restart

a known issue, see bug #389808
Blocks: 389876
ria and steve:  let's take the performance issue of starting up after this change to bug #389876
Steve, did you verify the fix other than the performance issues?
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.
Blocks: 408443
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.