659916 - Run expiration earlier after user-initiated removals

Status: RESOLVED INACTIVE

(Firefox :: Bookmarks & History, enhancement)

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 NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Clearing all history, removes favicon entries for all non bookmarked sites, as expected, but when clearing all history for a given site (either "delete" or "forget about this site" in the history sidebar) or clearing history for a time range (when all site visits fall within the range), corresponding favicon entries are not cleared.

Since a favicon entry is a record prior visitation, it should be cleared with history (when a clearing operation clears all site visits).  In more general terms, regardless of the path/mechanism, whenever the last moz_places entry for a site is removed the corresponding favicon should be removed.



Reproducible: Always

Comment 1

[Marco Bonardo [:mak]]

do you mean they are not removed from the database or the page somehow persist in the history UI?

Comment 2

[al_9x]

I am referring to moz_favicons records.

Comment 3

[Marco Bonardo [:mak]]

icons are expired by expiration, asynchronously, if your use-case is to check moz_favicons just after clearing history entries, doesn't seem that interesting.
If entries persist after minutes, then I'd need more information on the database entries.

Comment 4

[al_9x]

(In reply to comment #3)
> icons are expired by expiration, asynchronously

The normal expiration mechanism is not relevant here.  Fx should not inadvertently be keeping a stealth history, after clearing  When history is cleared the icons should be explicitly deleted as they are records of visitation.  That does happen when all history is deleted, but not per site or range.
 
> if your use-case is to
> check moz_favicons just after clearing history entries, doesn't seem that
> interesting.
> If entries persist after minutes, then I'd need more information on the
> database entries.

The icons definitely don't expire in minutes.  The max expiration time is a week, and for example on google.com that's what it ends up being.

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #4)
> The icons definitely don't expire in minutes.  The max expiration time is a
> week, and for example on google.com that's what it ends up being.

Expiration has nothing to do with icons expiration (That has indeed a 7 days value, but it's not relevant here). History expiration expires orphan icons, so icons that are no more referenced in moz_places.
I don't understand the rest, we are not inadvertently keeping anything, we are doing operations async rather than sync, and this is a design choice, when you delete we remove all visits, then async expiration takes care of removing orphans.
Are you absolutely sure those icons are unused, so there is no entry in moz_places with a favicon_id pointing to one of those?

Comment 6

[al_9x]

(In reply to comment #5)
> Are you absolutely sure those icons are unused, so there is no entry in
> moz_places with a favicon_id pointing to one of those?

Yes, I am sure.  There is nothing in moz_places other than 5 place: urls.
To repro:

xp sp3, Fx 4.0.1, new profile

1) start with a single about:home tab, clear everything, delete all bookmarks
2) navigate to http://www.google.com/
3) clear browsing history for the last hour (or delete www.google.com from the history sidebar), this removes google from moz_places, google favicon remains in moz_favicons and is not deleted after ~ 10 - 15 minutes or restarts.

Comment 7

[al_9x]

How many minutes should it take?  Does Fx need to be completely idle all that time?

Comment 8

[Marco Bonardo [:mak]]

3 to 9 minutes, depending on history status, the first cleanup after startup may delay a bit more. If a lots of removals did happen it will then start expiring every 3 minutes. Idle is not needed, but if it happens we also expire on idle and on shutdown.
Btw, I just tried, it took some time (slightly more than 10 mins) but entries were correctly removed.
May I get that database you generated in comment 6 to look at it?

Comment 9

[al_9x]

I tested it again, making it sure to give it enough time (15 minutes) and letting it idle and it did delete the favicon.  A similar async cleanup did not occur in Bug 659930, I'll post the exact steps there, please try to repro.

I guess it's working as designed, however I'd like to comment, this type of async, somewhat unpredictable cleanup makes sense for automated, background tasks, but not for an explicit user initiated, to all appearances synchronous, clearing operation.  One expects that when the dialog closes, everything that needed deleting has been deleted, there is no indication otherwise.

Why not finish everything synchronously?  Is it possible to force the cleanup now (via the error console)?

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> Why not finish everything synchronously?  Is it possible to force the
> cleanup now (via the error console)?

Because then the UI would lag badly, as it did in Firefox 3, it's matter of finding a compromise between speed and responsitivity.

What we may do is to ask for a sooner expiration after user actions, so that the wait time is reduced to the minimum.

Comment 11

[Marco Bonardo [:mak]]

confirming as enh request.

Comment 12

[al_9x]

(In reply to comment #10)
> (In reply to comment #9)
> > Why not finish everything synchronously?  Is it possible to force the
> > cleanup now (via the error console)?
> 
> Because then the UI would lag badly, as it did in Firefox 3, it's matter of
> finding a compromise between speed and responsitivity.
> 
> What we may do is to ask for a sooner expiration after user actions, so that
> the wait time is reduced to the minimum.

Also, closing Fx should complete the pending cleanups.  Delayed cleanup may be justifiable, but having data that's supposed to be deleted survive closing, can only be described as failure of the operation.

Can you post a snippet (for error console) that forces the cleanup?

Comment 13

[Marco Bonardo [:mak]]

Please don't confuse normal removals with Private Browsing, the latter is what prevents information leaking in real-time, normal history removals have obviously a lighter effect, they ensure you information is securely removed in an acceptable time. Users who care about their privacy that much, should use Private Browsing without any doubt.

Doing work at shutdown is a nice way of making users sad, the "I delete this and immediately close the browser" use-case is not that interesting, as it's not interesting real-time polling of the database data.
On shutdown the cleanup happens if the user removed a lot of pages and previous expiration step was unable to finish work.
That said, we could tweak things so that we may run an expiration at shutdown if a removal happened after previous expiration step.

Regarding the snippet, this should work:
Cc["@mozilla.org/places/expiration;1"].getService(Ci.nsIObserver).observe(null, "places-debug-start-expiration", null);

Comment 14

[al_9x]

(In reply to comment #13)
> That said, we could tweak things so that we may run an expiration at
> shutdown if a removal happened after previous expiration step.
> 

Would that guarantee that whatever is supposed to be deleted is cleaned up at least at closing, or would there still be situations when deletions would survive shutdown?  I think there should be such a guarantee, a deletion that leaves things behind in the underlying store after closing, feels flaky, broken. 

> Regarding the snippet, this should work:
> Cc["@mozilla.org/places/expiration;1"].getService(Ci.nsIObserver).
> observe(null, "places-debug-start-expiration", null);

Thanks, that works.

Components.classes["@mozilla.org/places/expiration;1"].getService(Components.interfaces.nsIObserver).observe(null, "places-debug-start-expiration", null);

Comment 15

[Marco Bonardo [:mak]]

(In reply to comment #14)
> Would that guarantee that whatever is supposed to be deleted is cleaned up
> at least at closing, or would there still be situations when deletions would
> survive shutdown?

May fail in case of large deletes.

I think there should be such a guarantee, a deletion that
> leaves things behind in the underlying store after closing, feels flaky,
> broken. 

It's just working as designed, so it's not broken. As I said the scope is not real-time privacy protection, since Private Browsing covers that need pretty well.
The scope here is to provide efficient-enough cleanup of the datastore, to remove useless stuff from UI, and protect privacy on a longer timeframe, but not real-time.

Comment 16

[al_9x]

(In reply to comment #13)
> Regarding the snippet, this should work:
> Cc["@mozilla.org/places/expiration;1"].getService(Ci.nsIObserver).
> observe(null, "places-debug-start-expiration", null);

Running this in Fx4 on a places.sqlite from Bug 656292, ends up deleting all history.  Is the issue form that bug, the loss of consistency between moz_places (visit_count, last_visit_date) and moz_historyvisits, sufficient to explain places-debug-start-expiration misbehaving?  If you can post the code to repair that particular inconsistency, I'll be able to answer that question. 

There should exist at least a manual full integrity check and repair for places, is Bug 476153 it?

Comment 17

[Marco Bonardo [:mak]]

This addon uses the maintenance code. It may be integrated in about:support in future.
https://addons.mozilla.org/firefox/addon/places-maintenance/

And no, expiration doesn't use visit_count, if it removes pages it's because they have no visits and no bookmarks associated.

Comment 18

[al_9x]

(In reply to comment #17)
> This addon uses the maintenance code. It may be integrated in about:support
> in future.
> https://addons.mozilla.org/firefox/addon/places-maintenance/
> 
> And no, expiration doesn't use visit_count, if it removes pages it's because
> they have no visits and no bookmarks associated.

But it definitely shouldn't be deleting all history?  The historyvisits table is empty after it runs.  So here's the situation, and it's the same with the addon, since it presumably calls the same code for expiration:

same places.sqlite injected into a new Fx4.0.1 profile, on one machine it deletes all history, on another it does not.  I also tried it in a clean xp vm on the second machine, and there it does delete.

Any ideas?  Obviously a db maintenance/repair tool should not be this flaky.  I suppose it could be the inconsistency in the db (though it's not clear why it would affect one system and not another), so can you post the repair code we discussed in Bug 656292, so I can test this hypothesis?

Comment 19

[al_9x]

(In reply to comment #18)
> so can you post the repair code
> we discussed in Bug 656292, so I can test this hypothesis?

After resyncing the tables with the code you posted in Bug 656292, executing places-debug-start-expiration still deletes all history.

What does that indicate to you?  Is there still something wrong with the db?  Is there a bug in expiration code?

Comment 20

[Marco Bonardo [:mak]]

no, expiration code has tests covering that thing and if it would do that thing we'd have far more complains than one, so there must be something other. what's the value of places.history.expiration.transient_current_max_pages in about:config? How many bookmarks are there? Do you have StumbleUpon or another add-on that may add tags?

Comment 21

[al_9x]

It's a new 4.0.1 profile, with an old places.sqlite dropped in.  No addons, 3250 moz_bookmarks, 33548 moz_places, 49877 history_visits, transient_current_max_pages == 32190.  So does max_pages being set close to the # of moz_places records explain the deletion?  Does it mean that if I let Fx idle for a while it will also autodelete all history?  That doesn't seem to be happening, only on manual invocation of debug expiration is all history deleted.

On the machine where history is not completely deleted by invoking debug expiration, max_pages is 96565.

Comment 22

[al_9x]

After setting places.history.expiration.max_pages to 2147483647, invoking debug expiration no longer deletes all history, so it seems the cause has been identified, however it's not clear to me if it's working as designed (seems more like a bug).

1) Is the deletion of all history normal for this db at transient_current_max_pages == 32190?  I don't see why it would be.  max_pages refers to history places records, not visits, right?

2) Why is there a difference between manual debug (deletes everything) and automatic (deletes nothing) expiration?

Comment 23

[al_9x]

Marco, please comment on the previous message.  There appears to be a bug in the manual expiration code.  The complete deletion of history does not seem justified, confirmed by the fact that automatic expiration does no such thing.

Comment 24

[al_9x]

I believe I ans(In reply to comment #20)
> no, expiration code has tests covering that thing and if it would do that
> thing we'd have far more complains than one, so there must be something
> other. what's the value of
> places.history.expiration.transient_current_max_pages in about:config? How
> many bookmarks are there? Do you have StumbleUpon or another add-on that may
> add tags?

I answered your questions, so is the complete deletion of history in this case, by manual expiration, a bug or not?

Comment 25

[Marco Bonardo [:mak]]

(In reply to comment #24)
> I answered your questions, so is the complete deletion of history in this
> case, by manual expiration, a bug or not?

Well, manual expiration is not something a user would ever see or use, we use it in tests, so it's not a bug. When I said
> Cc["@mozilla.org/places/expiration;1"].getService(Ci.nsIObserver).
> observe(null, "places-debug-start-expiration", null);
I forgot to specify that the third null indicates the limit for the expiration (null, undefined or such indicates to not limit).
Actually, this code is bogus http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesDBUtils.jsm#770 it should pass 0.

Comment 26

[Marco Bonardo [:mak]]

I filed bug 663344

Comment 27

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.