656292 - clearing history leaves empty site folders in the "by site" view of the history sidebar for many bookmarked sites

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[al_9x]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.17) Gecko/20110420 Firefox/3.6.17
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.17) Gecko/20110420 Firefox/3.6.17

I noticed when clearing all history as well as per site (deleting a site folder in the "by site" view of the history sidebar) that for many bookmarked sites, empty site folders were left behind in the history sidebar.

For all these sites, the visit_count is not cleared in the moz_places table, and for some, last_visit_date is not cleared.  These remain after multiple clearings.  Manually setting the visit_count to 0 causes the empty site folders to disappear.

I have not yet been able to repro in a new profile, but there is no indication that my places.sqlite has been corrupted, everything else seems to work, it passes the sqlite integrity_check  So what could cause this failure to clear visit_count and last_visit_date?

Reproducible: Always

Comment 1

[al_9x]

Fx 4.0.1 does clear all the visit_count (same places.sqlite) which removes the empty site folders from the history sidebar, but not all last_visit_date.

Comment 2

[al_9x]

(In reply to comment #0)
> I have not yet been able to repro in a new profile

I meant with a new places.sqlite.  I can repro in a new profile using the original problematic places.sqlite.

Comment 3

[Marco Bonardo [:mak]]

My guess is that visit_count was wrong for some reason, or you have a broken trigger. Could you send me that broken database by mail?

Comment 4

[al_9x]

(In reply to comment #3)
> My guess is that visit_count was wrong for some reason, or you have a broken
> trigger. Could you send me that broken database by mail?

I am not sharing my main profile history, but I can try to report what you need to know about the problematic entries or the db in general.

Comment 5

[al_9x]

When working normally, should both the visit_count and last_visit_date be cleared?

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)
> When working normally, should both the visit_count and last_visit_date be
> cleared?

they should, a trigger called moz_historyvisits_afterdelete_v2_trigger updates them everytime a visit is added or removed from moz_historyvisits

Comment 7

[al_9x]

(In reply to comment #6)
> (In reply to comment #5)
> > When working normally, should both the visit_count and last_visit_date be
> > cleared?
> 
> they should, a trigger called moz_historyvisits_afterdelete_v2_trigger
> updates them everytime a visit is added or removed from moz_historyvisits

According to sqlite manager, I have only one trigger moz_bookmarks_beforedelete_v1_trigger, same as in a new profile db.  Why doesn't a new db have this moz_historyvisits_afterdelete_v2_trigger?  Or am I not able to see it?

From what you've said I gather that if there is a mismatch between the visit_count in mozplaces and the number of entries for this place_id in moz_historyvisits, this problem would occur.

If that's the case, I have no idea how this would have happened, I know that I never manually edited either the visit_count or the moz_historyvisits entries.  But regardless of how this inconsistency crept in, would it not make sense, when clearing all history or all for a given site, to ensure that the remaining moz_places entries (matching the deleted site, if per site) have the visit_count and last_visit_date reset?

Can you post some code to resync moz_places (visit_count and last_visit_date) with moz_historyvisits?

Comment 8

[Marco Bonardo [:mak]]

(In reply to comment #7)
> According to sqlite manager, I have only one trigger
> moz_bookmarks_beforedelete_v1_trigger, same as in a new profile db.  Why
> doesn't a new db have this moz_historyvisits_afterdelete_v2_trigger?  Or am
> I not able to see it?

Good question, if the last time you used this db is with 3.6 it's usual to see _v1 triggers, Firefox 4 introduced the _v2 triggers (they are temp triggers though, don't remember if sqlite manager shows them)
Did you ever used nightlies or builds from the Places branch?
If you are using this profile only with FF4 you should remove the _v1 triggers, since otherwise you have double triggers an your visit_count is inflated.

> From what you've said I gather that if there is a mismatch between the
> visit_count in mozplaces and the number of entries for this place_id in
> moz_historyvisits, this problem would occur.

Also last_visit_date is important.
Regarding visit_count bug 476153 should implement a maintenance task to fix it.

> But regardless of how this inconsistency crept in, would it not
> make sense, when clearing all history or all for a given site, to ensure
> that the remaining moz_places entries (matching the deleted site, if per
> site) have the visit_count and last_visit_date reset?

It would be a useless complication, the trigger is there so we don't have to care.

> Can you post some code to resync moz_places (visit_count and
> last_visit_date) with moz_historyvisits?

there is an example for visit_count here http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesDBUtils.jsm#687 it can probably be rewritten as a single query and could also set last_visit_date

Comment 9

[al_9x]

(In reply to comment #8)
> (In reply to comment #7)
> > According to sqlite manager, I have only one trigger
> > moz_bookmarks_beforedelete_v1_trigger, same as in a new profile db.  Why
> > doesn't a new db have this moz_historyvisits_afterdelete_v2_trigger?  Or am
> > I not able to see it?
> 
> Good question, if the last time you used this db is with 3.6 it's usual to
> see _v1 triggers, Firefox 4 introduced the _v2 triggers (they are temp
> triggers though, don't remember if sqlite manager shows them)
> Did you ever used nightlies or builds from the Places branch?
> If you are using this profile only with FF4 you should remove the _v1
> triggers, since otherwise you have double triggers an your visit_count is
> inflated.

My places.sqlite is from a 3.6 profile, never been used in any higher build. The 3.6 places.sqlite does not have a permanent/visible moz_historyvisits_afterdelete trigger, is it temporary, hence not visible in sqlite manager?  Incidentally, a new 4.0 places.sqlite also has only a single permanent moz_bookmarks_beforedelete_v1_trigger.  You are not suggesting to delete it?

> 
> > From what you've said I gather that if there is a mismatch between the
> > visit_count in mozplaces and the number of entries for this place_id in
> > moz_historyvisits, this problem would occur.
> 
> Also last_visit_date is important.
> Regarding visit_count bug 476153 should implement a maintenance task to fix
> it.
> 
> > But regardless of how this inconsistency crept in, would it not
> > make sense, when clearing all history or all for a given site, to ensure
> > that the remaining moz_places entries (matching the deleted site, if per
> > site) have the visit_count and last_visit_date reset?
> 
> It would be a useless complication, the trigger is there so we don't have to
> care.

Whatever the mechanism is, it seems to have a bug, my places.sqlite somehow got this way through normal usage.  So it wouldn't be useless, as it would guarantee that regardless of any preexisting inconsistency, the clear operation would leave the db in a (more) proper state.  However, if you don't think this is worth doing, I would be happy with a workaround.

> 
> > Can you post some code to resync moz_places (visit_count and
> > last_visit_date) with moz_historyvisits?
> 
> there is an example for visit_count here
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> PlacesDBUtils.jsm#687 it can probably be rewritten as a single query and
> could also set last_visit_date

Could you please do that and post a snippet one can just run in the error console?

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> My places.sqlite is from a 3.6 profile, never been used in any higher build.
> The 3.6 places.sqlite does not have a permanent/visible
> moz_historyvisits_afterdelete trigger, is it temporary, hence not visible in
> sqlite manager?

> Incidentally, a new 4.0 places.sqlite also has only a
> single permanent moz_bookmarks_beforedelete_v1_trigger.  You are not
> suggesting to delete it?

wait, moz_bookmarks_beforedelete_v1_trigger is CORRECT, we are talking about history triggers, not bookmarks triggers.
4.0 has:
- moz_bookmarks_beforedelete_v1_trigger
- moz_historyvisits_afterinsert_v2_trigger (temp)
- moz_historyvisits_afterdelete_v2_trigger (temp)
- TRIGGER moz_openpages_temp_afterupdate_trigger (temp)
if you see any trigger that is not one of these it's wrong.

3.6 has much different triggers (the only one that is the same is probably moz_bookmarks_beforedelete_v1_trigger).

> Whatever the mechanism is, it seems to have a bug, my places.sqlite somehow
> got this way through normal usage.  So it wouldn't be useless, as it would
> guarantee that regardless of any preexisting inconsistency, the clear
> operation would leave the db in a (more) proper state.  However, if you
> don't think this is worth doing, I would be happy with a workaround.

Maybe, or maybe not. I think you are an advanced user moving across nightlies and versions (maybe also across branches?), thus it could be due to some temporary change we made that your profile collected wrongly.
As I said bug 476153 will do a coherence fix weekly on that data, once fixed.

> Could you please do that and post a snippet one can just run in the error
> console?

Yes, once I have time to do that (otherwise I'd already fixed the above bug in maintenance)

Comment 11

[al_9x]

(In reply to comment #10)
> (In reply to comment #9)
> > My places.sqlite is from a 3.6 profile, never been used in any higher build.
> > The 3.6 places.sqlite does not have a permanent/visible
> > moz_historyvisits_afterdelete trigger, is it temporary, hence not visible in
> > sqlite manager?
> 
> > Incidentally, a new 4.0 places.sqlite also has only a
> > single permanent moz_bookmarks_beforedelete_v1_trigger.  You are not
> > suggesting to delete it?
> 
> wait, moz_bookmarks_beforedelete_v1_trigger is CORRECT, we are talking about
> history triggers, not bookmarks triggers.
> 4.0 has:
> - moz_bookmarks_beforedelete_v1_trigger
> - moz_historyvisits_afterinsert_v2_trigger (temp)
> - moz_historyvisits_afterdelete_v2_trigger (temp)
> - TRIGGER moz_openpages_temp_afterupdate_trigger (temp)
> if you see any trigger that is not one of these it's wrong.
> 
> 3.6 has much different triggers (the only one that is the same is probably
> moz_bookmarks_beforedelete_v1_trigger).

But it must have something equivalent to moz_historyvisits_afterdelete_v2_trigger?

> 
> > Whatever the mechanism is, it seems to have a bug, my places.sqlite somehow
> > got this way through normal usage.  So it wouldn't be useless, as it would
> > guarantee that regardless of any preexisting inconsistency, the clear
> > operation would leave the db in a (more) proper state.  However, if you
> > don't think this is worth doing, I would be happy with a workaround.
> 
> Maybe, or maybe not. I think you are an advanced user moving across
> nightlies and versions (maybe also across branches?), thus it could be due
> to some temporary change we made that your profile collected wrongly.

I have used 4.0 betas and nightlies but never on my main profile.  This places.sqlite was created by 3.6 upgrading a 2.0 profile and never used with any nightly/alpha/beta builds.

Comment 12

[Marco Bonardo [:mak]]

(In reply to comment #11)
> > 3.6 has much different triggers (the only one that is the same is probably
> > moz_bookmarks_beforedelete_v1_trigger).
> 
> But it must have something equivalent to
> moz_historyvisits_afterdelete_v2_trigger?

there is a temporary trigger that is much more complicated since it has to update memory tables and such, we removed all that crazy stuff for 4.0.

> I have used 4.0 betas and nightlies but never on my main profile.  This
> places.sqlite was created by 3.6 upgrading a 2.0 profile and never used with
> any nightly/alpha/beta builds.

Ok, it's possible there was some bug in the temp tables management then. 4.0 doesn't have any crazy partitioning on the db.
Will try to find some time to make a query in the next days and let you know, ping me in case I forget.

Comment 13

[al_9x]

> > Can you post some code to resync moz_places (visit_count and
> > last_visit_date) with moz_historyvisits?
> 
> there is an example for visit_count here
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> PlacesDBUtils.jsm#687 it can probably be rewritten as a single query and
> could also set last_visit_date
>
> > Could you please do that and post a snippet one can just run in the error
> > console?
> 
> Yes, once I have time to do that (otherwise I'd already fixed the above bug
> in maintenance)

In case you've forgotten.

Comment 14

[Marco Bonardo [:mak]]

Try this one:

UPDATE moz_places SET visit_count = (SELECT count(*) FROM moz_historyvisits WHERE place_id = moz_places.id AND visit_type NOT IN (0,4,7,8)) WHERE id IN (SELECT id FROM moz_places h WHERE h.visit_count != (SELECT count(*) FROM moz_historyvisits WHERE place_id = h.id AND visit_type NOT IN (0,4,7,8)))

Comment 15

[al_9x]

(In reply to comment #14)
> Try this one:
> 
> UPDATE moz_places SET visit_count = (SELECT count(*) FROM moz_historyvisits
> WHERE place_id = moz_places.id AND visit_type NOT IN (0,4,7,8)) WHERE id IN
> (SELECT id FROM moz_places h WHERE h.visit_count != (SELECT count(*) FROM
> moz_historyvisits WHERE place_id = h.id AND visit_type NOT IN (0,4,7,8)))

Please update it to take care of the last_visit_date as we discussed:

> there is an example for visit_count here
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> PlacesDBUtils.jsm#687 it can probably be rewritten as a single query and
> could also set last_visit_date

So the actual command would be:

Components.classes["@mozilla.org/browser/nav-history-service;1"].getService(Components.interfaces.nsPIPlacesDatabase).DBConnection.executeSimpleSQL("UPDATE moz_places SET visit_count = (SELECT count(*) FROM moz_historyvisits WHERE place_id = moz_places.id AND visit_type NOT IN (0,4,7,8)) WHERE id IN (SELECT id FROM moz_places h WHERE h.visit_count != (SELECT count(*) FROM moz_historyvisits WHERE place_id = h.id AND visit_type NOT IN (0,4,7,8)))");

right?

Also, please comment on Bug 519830.

Comment 16

[Marco Bonardo [:mak]]

This may work, it will fix last_visit_date only if the visit_count is wrong though (since they are part of the same trigger, may be an acceptable rule)

"UPDATE moz_places SET visit_count = (SELECT count(*) FROM moz_historyvisits WHERE place_id = moz_places.id AND visit_type NOT IN (0,4,7,8)), last_visit_date = (SELECT visit_date FROM moz_historyvisits WHERE place_id = moz_places.id ORDER BY visit_date DESC LIMIT 1) WHERE id IN (SELECT id FROM moz_places h WHERE h.visit_count != (SELECT count(*) FROM moz_historyvisits WHERE place_id = h.id AND visit_type NOT IN (0,4,7,8)))"

Comment 17

[al_9x]

(In reply to comment #16)
> This may work, it will fix last_visit_date only if the visit_count is wrong
> though (since they are part of the same trigger, may be an acceptable rule)

Why only may? It does not have to be one statement, but please post the code to completely resync moz_places (visit_count and last_visit_date) with moz_historyvisits.  I don't think this will be a wasted effort for you as this operation will become part of the full integrity check of your addon or Fx.

Comment 18

[al_9x]

This seems to be working. After running the code and clearing history, visit_count is 0 and last_visit_date is null for all records and consequently there are no empty site folders in the sidebar.

Comment 19

[Marco Bonardo [:mak]]

Bug 692493 added checks on this coherence to weekly maintenance, so this should disappear from the wild.